diff --git a/.env.example b/.env.example index b59476ffcd..31c8b0883e 100644 --- a/.env.example +++ b/.env.example @@ -68,32 +68,32 @@ GITTENSORY_REVIEW_ENRICHMENT=false # commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot # blameLink,approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch,commitHygiene # pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker,magicNumber -# conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting,errorSwallow,unsafeAny,a11y -# i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak,deprecatedDep,revertRecurrence -# coverageDelta,callerImpact +# conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting,errorSwallow,complexity +# unsafeAny,a11y,i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak,deprecatedDep +# revertRecurrence,coverageDelta,callerImpact # # Profile defaults: # fast: dependency,dependencyDiff,lockfileDrift,secret,license,installScript,heavyDependency # hardcodedUrl,actionPin,eol,redos,provenance,secretLog,typosquat,iacMisconfig,nativeBuild # testRatio,migrationSafety,looseRange,terminology,todoMarker,magicNumber,conflictMarker -# debugLeftover,sizeSmell,floatingPromise,deepNesting,errorSwallow,unsafeAny,a11y,i18n,apiBreak -# deprecatedDep +# debugLeftover,sizeSmell,floatingPromise,deepNesting,errorSwallow,complexity,unsafeAny,a11y +# i18n,apiBreak,deprecatedDep # balanced (default): dependency,dependencyDiff,lockfileDrift,secret,license,installScript # heavyDependency,hardcodedUrl,actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight # typosquat,commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift,duplication # churnHotspot,blameLink,approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch # commitHygiene,pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology # todoMarker,magicNumber,conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting -# errorSwallow,unsafeAny,a11y,i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak -# deprecatedDep,revertRecurrence,coverageDelta,callerImpact +# errorSwallow,complexity,unsafeAny,a11y,i18n,unusedExport,exhaustiveness,flakyTest,commitLint +# apiBreak,deprecatedDep,revertRecurrence,coverageDelta,callerImpact # deep: dependency,dependencyDiff,lockfileDrift,secret,license,installScript,heavyDependency # hardcodedUrl,actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat # commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot # blameLink,approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch,commitHygiene # pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker,magicNumber -# conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting,errorSwallow,unsafeAny,a11y -# i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak,deprecatedDep,revertRecurrence -# coverageDelta,callerImpact +# conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting,errorSwallow,complexity +# unsafeAny,a11y,i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak,deprecatedDep +# revertRecurrence,coverageDelta,callerImpact # END GENERATED REES ANALYZERS # Submitter-reputation spend control (internal-only): downgrades new/burst/low-rep diff --git a/apps/gittensory-ui/src/lib/rees-analyzers.ts b/apps/gittensory-ui/src/lib/rees-analyzers.ts index feca564a90..e0b8407c2c 100644 --- a/apps/gittensory-ui/src/lib/rees-analyzers.ts +++ b/apps/gittensory-ui/src/lib/rees-analyzers.ts @@ -1041,12 +1041,37 @@ export const REES_ANALYZERS = [ }, docs: { summary: - "Flags newly-added catch/except blocks that swallow the error — empty body, unused binding, or a bare `return null`.", - looksAt: "Added lines in changed non-test JS/TS/Python source files.", - reports: "File, line, and kind: empty-catch, unused-binding, or return-null.", + "Flags newly-added catch/except blocks (and Go if-err checks) that swallow or mishandle the error — empty body, unused binding, a bare `return null`/`nil`, or a Python bare `except:` naming no exception type.", + looksAt: "Added lines in changed non-test JS/TS/Python/Go source files.", + reports: "File, line, and kind: empty-catch, unused-binding, return-null, or bare-except.", network: "Pure local analyzer. No external network call.", notes: - "Multiline catch bodies are collected with brace balance. Catches that log, rethrow, or reference the binding are not flagged. Brace counting is character-level (string literals are not stripped).", + "Multiline catch/if-err bodies are collected with brace balance (Go's `if err != nil { … }`, including the if-with-initializer form, is treated the same as a JS/TS catch). Handlers that log, rethrow/panic, or reference the checked binding are not flagged. Python's bare `except:` is flagged regardless of body — it catches SystemExit/KeyboardInterrupt too. Brace counting is character-level (string literals are not stripped).", + }, + }, + { + name: "complexity", + title: "Approximate cyclomatic complexity", + category: "quality", + cost: "local", + defaultEnabled: true, + profiles: ["fast", "balanced", "deep"], + requires: ["files"], + limits: { + maxFindings: 25, + maxComplexity: 10, + maxLineChars: 2000, + }, + docs: { + summary: + "Flags a newly-added function whose approximate cyclomatic complexity (branch/loop/logical-operator density, computed on the diff-visible lines) exceeds a threshold.", + looksAt: + "Added lines in changed non-test TS/JS source files, starting from a named function declaration or a const/let/var-assigned arrow function whose opening line is part of the diff.", + reports: + "File, line, the detected function name, the measured complexity, and the configured threshold.", + network: "Pure local analyzer. No external network call.", + notes: + "Diff-hunk approximation, not a whole-function true McCabe count: REES has no full-file content, so this counts if/for/while/case/catch/&&/||/?? token occurrences across the function's ADDED body lines only (1 + count), the same function-boundary detection size-smell.ts (#2019) uses for 'big-function'. A function whose signature line is not part of the diff is not scored. Distinct from deep-nesting (#2030), which measures brace NESTING depth, a readability smell, not decision-point density. Ternary (`? :`) is intentionally excluded — see the analyzer source header for why.", }, }, { diff --git a/packages/gittensory-engine/src/review/enrichment-analyzer-names.ts b/packages/gittensory-engine/src/review/enrichment-analyzer-names.ts index 3a8b7d47a1..e6580b36cf 100644 --- a/packages/gittensory-engine/src/review/enrichment-analyzer-names.ts +++ b/packages/gittensory-engine/src/review/enrichment-analyzer-names.ts @@ -46,6 +46,7 @@ export const REES_ANALYZER_NAMES = [ "floatingPromise", "deepNesting", "errorSwallow", + "complexity", "unsafeAny", "a11y", "i18n", diff --git a/review-enrichment/Dockerfile b/review-enrichment/Dockerfile index e06eb5c158..6e68312fb1 100644 --- a/review-enrichment/Dockerfile +++ b/review-enrichment/Dockerfile @@ -1,4 +1,6 @@ -# Gittensory review-enrichment service (REES). Lean two-stage Node build; analyzers add CLI tools later (#1477). +# Gittensory review-enrichment service (REES). Lean two-stage Node build; analyzers are pure-JS diff-hunk +# heuristics with no external CLI tools (#1477 scoped this to the cheap, no-checkout path -- see +# review-enrichment/src/analyzers/complexity.ts's header for why a real linter/AST toolchain is out of scope). # Build context = the review-enrichment/ directory (Railway "Root Directory" = review-enrichment). FROM node:22-slim AS build WORKDIR /app diff --git a/review-enrichment/analyzer-metadata.json b/review-enrichment/analyzer-metadata.json index 6482e449c9..40ca05a34f 100644 --- a/review-enrichment/analyzer-metadata.json +++ b/review-enrichment/analyzer-metadata.json @@ -1180,11 +1180,38 @@ "maxLineChars": 2000 }, "docs": { - "summary": "Flags newly-added catch/except blocks that swallow the error — empty body, unused binding, or a bare `return null`.", - "looksAt": "Added lines in changed non-test JS/TS/Python source files.", - "reports": "File, line, and kind: empty-catch, unused-binding, or return-null.", + "summary": "Flags newly-added catch/except blocks (and Go if-err checks) that swallow or mishandle the error — empty body, unused binding, a bare `return null`/`nil`, or a Python bare `except:` naming no exception type.", + "looksAt": "Added lines in changed non-test JS/TS/Python/Go source files.", + "reports": "File, line, and kind: empty-catch, unused-binding, return-null, or bare-except.", "network": "Pure local analyzer. No external network call.", - "notes": "Multiline catch bodies are collected with brace balance. Catches that log, rethrow, or reference the binding are not flagged. Brace counting is character-level (string literals are not stripped)." + "notes": "Multiline catch/if-err bodies are collected with brace balance (Go's `if err != nil { … }`, including the if-with-initializer form, is treated the same as a JS/TS catch). Handlers that log, rethrow/panic, or reference the checked binding are not flagged. Python's bare `except:` is flagged regardless of body — it catches SystemExit/KeyboardInterrupt too. Brace counting is character-level (string literals are not stripped)." + } + }, + { + "name": "complexity", + "title": "Approximate cyclomatic complexity", + "category": "quality", + "cost": "local", + "defaultEnabled": true, + "profiles": [ + "fast", + "balanced", + "deep" + ], + "requires": [ + "files" + ], + "limits": { + "maxFindings": 25, + "maxComplexity": 10, + "maxLineChars": 2000 + }, + "docs": { + "summary": "Flags a newly-added function whose approximate cyclomatic complexity (branch/loop/logical-operator density, computed on the diff-visible lines) exceeds a threshold.", + "looksAt": "Added lines in changed non-test TS/JS source files, starting from a named function declaration or a const/let/var-assigned arrow function whose opening line is part of the diff.", + "reports": "File, line, the detected function name, the measured complexity, and the configured threshold.", + "network": "Pure local analyzer. No external network call.", + "notes": "Diff-hunk approximation, not a whole-function true McCabe count: REES has no full-file content, so this counts if/for/while/case/catch/&&/||/?? token occurrences across the function's ADDED body lines only (1 + count), the same function-boundary detection size-smell.ts (#2019) uses for 'big-function'. A function whose signature line is not part of the diff is not scored. Distinct from deep-nesting (#2030), which measures brace NESTING depth, a readability smell, not decision-point density. Ternary (`? :`) is intentionally excluded — see the analyzer source header for why." } }, { diff --git a/review-enrichment/src/analyzers/complexity.ts b/review-enrichment/src/analyzers/complexity.ts new file mode 100644 index 0000000000..6645bd68e0 --- /dev/null +++ b/review-enrichment/src/analyzers/complexity.ts @@ -0,0 +1,203 @@ +// Approximate cyclomatic-complexity analyzer (#1477). REES has no full-file content -- only diff hunks -- so +// this is deliberately NOT a whole-function true McCabe count (that needs a real parser reading the ENTIRE +// function, including any part outside the diff, and a new AST-parser dependency this service does not carry). +// Instead it approximates: for each newly-added function whose OPENING line is visible in the diff (named +// `function` declarations and arrow functions assigned to const/let/var -- the same structural detection +// size-smell.ts (#2019) already uses for "big-function"), it counts branch/loop/logical-operator tokens across +// the function's ADDED body lines only and reports `1 + that count`, the standard McCabe formula computed on +// the visible slice. A function whose signature line is NOT part of the diff (only its body was edited) is not +// attributed a complexity score, the same accepted scope limit size-smell.ts already carries for "big-function". +// +// Distinct from deep-nesting.ts (#2030), which measures brace NESTING depth -- a readability smell that +// analyzer's own header explicitly disclaims as a complexity metric. This analyzer counts DECISION POINTS +// instead: a flat function (nesting depth 1) can still have high complexity from many sibling `if`/`&&` checks, +// and a deeply-nested function can have low complexity if each level has only one predicate. The two analyzers +// intentionally measure different axes of the same diff. +// +// Ternary (`? :`) is deliberately EXCLUDED from the decision-point count: distinguishing a conditional +// expression's `?` from TypeScript's optional-property/parameter marker (`foo?: T`) or optional chaining +// (`?.`) is not reliably decidable per-line by regex without a false-positive rate this precision-first +// heuristic rejects. if/for/while/case/catch/&&/||/?? are unambiguous token shapes that cover the bulk of +// realistic branching. +// +// Pure compute over added diff lines, no network, no new dependency. churn-hotspot (#1513) is not precedent for +// a broader one-time fetch here: it fetches commit METADATA that cannot exist in a diff in any form at all, so a +// fetch is its only option; complexity is partially approximable from the diff text itself, so the cheap +// in-hunk approximation -- not a full-file fetch -- is the right scope for this analyzer. +import type { ComplexityFinding, EnrichRequest } from "../types.js"; +import { codeOnly } from "./secret-log.js"; +import { isTestPath } from "./test-ratio.js"; + +export const DEFAULT_MAX_COMPLEXITY = 10; +const MAX_FINDINGS = 25; +const MAX_LINE_CHARS = 2000; + +const JS_TS_PATH_RE = /\.(?:tsx?|jsx?|mts|cts|cjs|mjs)$/i; + +const FUNCTION_OPEN_RE = + /\bfunction\s+(\w+)\s*\([^)]*\)\s*\{|\b(?:const|let|var)\s+(\w+)\s*=\s*(?:async\s*)?(?:function\s*)?\([^)]*\)\s*=>\s*\{/; + +// Decision-point token classes, each counted as +1 branch. `if` also matches the "if" inside "else if" (correct: +// only the branch "else if" itself introduces should add 1; a bare "else" with no "if" adds 0, matching McCabe +// semantics). for/for-of/for-in/for-await, while (do-while is counted once via its trailing `while(...)`), a +// switch `case` label (never `default`, which is not an additional predicate), `catch`, and the `&&`/`||`/`??` +// short-circuit operators (each occurrence is its own branch). All patterns are flat (no group is itself +// quantified), so none can backtrack catastrophically. +const DECISION_RES: RegExp[] = [ + /\bif\s*\(/g, + /\bfor\s*(?:await\s*)?\(/g, + /\bwhile\s*\(/g, + /\bcatch\s*[({]/g, + /\bcase\s+/g, + /&&/g, + /\|\|/g, + /\?\?/g, +]; + +function isJsTsPath(path: string): boolean { + return JS_TS_PATH_RE.test(path) && !isTestPath(path); +} + +function isCommentLine(line: string): boolean { + const trimmed = line.trimStart(); + return /^(?:\/\/|\/\*|\*)/.test(trimmed); +} + +/** Count decision-point tokens (if/for/while/case/catch/&&/||/??) in one code fragment. Pure. */ +export function countDecisionPoints(code: string): number { + let total = 0; + for (const re of DECISION_RES) { + const matches = code.match(re); + if (matches) total += matches.length; + } + return total; +} + +/** The declared/assigned name when a line opens a named function declaration or an arrow function assigned to a + * const/let/var -- the same structural scope size-smell.ts's function detection uses. Pure. */ +export function functionNameFromLine(line: string): string | undefined { + if (isCommentLine(line)) return undefined; + const match = FUNCTION_OPEN_RE.exec(codeOnly(line)); + return match?.[1] ?? match?.[2]; +} + +function braceDepthDelta(code: string): number { + let depth = 0; + for (const ch of code) { + if (ch === "{") depth++; + else if (ch === "}") depth--; + } + return depth; +} + +type ScanLimits = { + maxComplexity?: number; + maxFindings?: number; + signal?: AbortSignal; +}; + +type PendingFunction = { + name: string; + startLine: number; + complexity: number; + depth: number; +}; + +/** Scan one file patch's added lines for a newly-added function whose approximate complexity exceeds a + * threshold, line-cited via hunk headers. Pure. */ +export function scanPatchForComplexity( + path: string, + patch: string, + limits: ScanLimits = {}, +): ComplexityFinding[] { + const configured = limits.maxComplexity ?? DEFAULT_MAX_COMPLEXITY; + const maxComplexity = configured > 0 ? configured : DEFAULT_MAX_COMPLEXITY; + const maxFindings = limits.maxFindings ?? MAX_FINDINGS; + if (maxFindings <= 0 || !isJsTsPath(path)) return []; + + const findings: ComplexityFinding[] = []; + let newLine = 0; + let inHunk = false; + let pending: PendingFunction | null = null; + + const flushFunction = () => { + if (!pending) return; + if (pending.complexity > maxComplexity) { + findings.push({ + file: path, + line: pending.startLine, + name: pending.name, + complexity: pending.complexity, + threshold: maxComplexity, + }); + } + pending = null; + }; + + for (const line of patch.split("\n")) { + if (limits.signal?.aborted) throw new Error("analyzer_aborted"); + const hunk = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(line); + if (hunk) { + flushFunction(); + newLine = Number(hunk[1]); + inHunk = true; + continue; + } + if (!inHunk) continue; + + if (line.startsWith("+")) { + const body = line.slice(1); + if (body.length <= MAX_LINE_CHARS) { + const commented = isCommentLine(body); + const code = codeOnly(body); + if (pending) { + if (!commented) pending.complexity += countDecisionPoints(code); + pending.depth += braceDepthDelta(code); + if (pending.depth <= 0) flushFunction(); + } else { + const name = functionNameFromLine(body); + if (name) { + pending = { + name, + startLine: newLine, + complexity: 1 + (commented ? 0 : countDecisionPoints(code)), + depth: braceDepthDelta(code), + }; + if (pending.depth <= 0) flushFunction(); + } + } + } + newLine++; + } else { + flushFunction(); + if (!line.startsWith("-") && !line.startsWith("\\")) { + newLine++; + } + } + + if (findings.length >= maxFindings) return findings; + } + + flushFunction(); + return findings; +} + +/** Analyzer entrypoint: scan every changed TS/JS file's added lines for high approximate complexity. */ +export async function scanComplexity( + req: EnrichRequest, + signal?: AbortSignal, +): Promise { + const findings: ComplexityFinding[] = []; + for (const file of req.files ?? []) { + if (signal?.aborted) throw new Error("analyzer_aborted"); + if (!file.patch) continue; + for (const finding of scanPatchForComplexity(file.path, file.patch, { + maxFindings: MAX_FINDINGS - findings.length, + signal, + })) { + findings.push(finding); + if (findings.length >= MAX_FINDINGS) return findings; + } + } + return findings; +} diff --git a/review-enrichment/src/analyzers/error-swallow.ts b/review-enrichment/src/analyzers/error-swallow.ts index 4917665a52..b42cc57698 100644 --- a/review-enrichment/src/analyzers/error-swallow.ts +++ b/review-enrichment/src/analyzers/error-swallow.ts @@ -1,214 +1,244 @@ -// Empty-catch / error-swallow analyzer (#2014). Flags newly-added catch/except blocks that swallow the error -// (empty body, unused binding, or a bare `return null`) — a top source of silent failures. Pure compute over -// added diff lines, no network. Scoped to JS/TS/Python source files; Python `except: pass` is included. -import type { EnrichRequest, ErrorSwallowFinding } from "../types.js"; -import { isTestPath } from "./test-ratio.js"; - -const MAX_FINDINGS = 25; -const MAX_LINE_CHARS = 2000; - -const SOURCE_EXTS = new Set(["ts", "tsx", "js", "jsx", "mjs", "cjs", "mts", "cts", "py"]); - -const CATCH_OPEN_RE = /catch\s*(?:\(\s*([\w$]+)?\s*\))?\s*\{/; -const PY_EXCEPT_PASS_RE = /^\s*except(?:\s+[\w.]+\s*(?:as\s+(\w+))?)?\s*:\s*pass\s*(?:#.*)?$/; - -function isScannablePath(path: string): boolean { - const ext = /\.([^.]+)$/.exec(path)?.[1]?.toLowerCase(); - return Boolean(ext && SOURCE_EXTS.has(ext) && !isTestPath(path)); -} - -function escapeRegExp(value: string): string { - return value.replace(/[$.*+?^{}()|[\]\\]/g, "\\$&"); -} - -function referencesBinding(body: string, binding: string): boolean { - const escaped = escapeRegExp(binding); - const bindingRe = new RegExp(`(? { - findings.push({ file: path, line, kind }); - }; - - for (const line of patch.split("\n")) { - if (limits.signal?.aborted) throw new Error("analyzer_aborted"); - const hunk = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(line); - if (hunk) { - newLine = Number(hunk[1]); - inHunk = true; - pending = null; - continue; - } - if (!inHunk) continue; - - if (line.startsWith("+")) { - const body = line.slice(1); - if (body.length <= MAX_LINE_CHARS) { - if (pending) { - pending = updatePending(pending, body); - if (pending.depth <= 0) { - const kind = flushPending(pending); - if (kind) { - pushFinding(pending.startLine, kind); - if (findings.length >= maxFindings) return findings; - } - pending = null; - } - } else { - const kind = detectErrorSwallow(body); - if (kind) { - pushFinding(newLine, kind); - if (findings.length >= maxFindings) return findings; - } else { - const open = CATCH_OPEN_RE.exec(body); - if (open) { - const braceIndex = body.indexOf("{", open.index ?? 0); - if (braceIndex >= 0) { - const depth = braceBalanceFrom(body, braceIndex); - if (depth > 0) { - pending = { - startLine: newLine, - binding: open[1] ?? null, - body: body.slice(braceIndex), - depth, - }; - } - } - } - } - } - } - newLine++; - } else if (!line.startsWith("-") && !line.startsWith("\\")) { - pending = null; - newLine++; - } else { - pending = null; - } - - if (findings.length >= maxFindings) return findings; - } - - return findings; -} - -/** Analyzer entrypoint: scan every changed scannable file's added lines for swallowed errors. */ -export async function scanErrorSwallow( - req: EnrichRequest, - signal?: AbortSignal, -): Promise { - const findings: ErrorSwallowFinding[] = []; - for (const file of req.files ?? []) { - if (signal?.aborted) throw new Error("analyzer_aborted"); - if (!file.patch) continue; - for (const finding of scanPatchForErrorSwallow(file.path, file.patch, { - maxFindings: MAX_FINDINGS - findings.length, - signal, - })) { - findings.push(finding); - if (findings.length >= MAX_FINDINGS) return findings; - } - } - return findings; -} +// Empty-catch / error-swallow analyzer (#2014, extended for Go + Python bare-except by #1477). Flags +// newly-added catch/except blocks (and Go `if err != nil` checks) that swallow or mishandle the error — empty +// body, unused binding, a bare `return null`/`nil`, or (Python-only) a bare `except:` naming no exception type, +// which catches everything (including SystemExit/KeyboardInterrupt) regardless of its body — all top sources +// of silent failures. Pure compute over added diff lines, no network. Scoped to JS/TS/Python/Go source files. +import type { EnrichRequest, ErrorSwallowFinding } from "../types.js"; +import { isTestPath } from "./test-ratio.js"; + +const MAX_FINDINGS = 25; +const MAX_LINE_CHARS = 2000; + +const SOURCE_EXTS = new Set(["ts", "tsx", "js", "jsx", "mjs", "cjs", "mts", "cts", "py", "go"]); + +const CATCH_OPEN_RE = /catch\s*(?:\(\s*([\w$]+)?\s*\))?\s*\{/; +// Go's `if err != nil { ... }` check, in both its bare form (`if err != nil {`) and its very common +// if-with-initializer form (`if err := f(); err != nil {`, where only the text after the `;` is the actual +// check). The captured identifier must itself look like an error variable (contains "err"/"error", +// case-insensitively on the leading letter) so an unrelated nil-pointer check like `if node != nil {` is never +// mistaken for error handling. Parens are deliberately NOT part of the match: JS/TS require parens around an +// `if` condition and Go idiomatic (gofmt) style never adds them, so this shape cannot occur in valid JS/TS. +const GO_ERR_CHECK_OPEN_RE = /(?:\bif\s+|;\s*)(\w*[Ee]rr(?:or)?\d*)\s*!=\s*nil\s*\{/; +const OPEN_RES: RegExp[] = [CATCH_OPEN_RE, GO_ERR_CHECK_OPEN_RE]; +const PY_EXCEPT_PASS_RE = /^\s*except(?:\s+[\w.]+\s*(?:as\s+(\w+))?)?\s*:\s*pass\s*(?:#.*)?$/; +// A bare Python `except:` naming no exception type at all — flake8's E722. This is a defect independent of the +// handler body (which may log or re-raise perfectly well): a bare except also catches SystemExit, +// KeyboardInterrupt, and GeneratorExit, which should almost never be swallowed alongside ordinary exceptions. +// Anchored so `except Exception:` / `except (A, B):` (a real type named) never match. +const PY_BARE_EXCEPT_RE = /^\s*except\s*:\s*(?:#.*)?$/; + +/** Try each recognized error-handling opener (JS/TS `catch`, Go `if err != nil`) against `line`, in order. + * Returns the first match, with the checked/bound identifier always in capture group 1. Pure. */ +function matchErrorOpen(line: string): RegExpExecArray | null { + for (const re of OPEN_RES) { + const match = re.exec(line); + if (match) return match; + } + return null; +} + +function isScannablePath(path: string): boolean { + const ext = /\.([^.]+)$/.exec(path)?.[1]?.toLowerCase(); + return Boolean(ext && SOURCE_EXTS.has(ext) && !isTestPath(path)); +} + +function escapeRegExp(value: string): string { + return value.replace(/[$.*+?^{}()|[\]\\]/g, "\\$&"); +} + +function referencesBinding(body: string, binding: string): boolean { + const escaped = escapeRegExp(binding); + const bindingRe = new RegExp(`(? { + findings.push({ file: path, line, kind }); + }; + + for (const line of patch.split("\n")) { + if (limits.signal?.aborted) throw new Error("analyzer_aborted"); + const hunk = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(line); + if (hunk) { + newLine = Number(hunk[1]); + inHunk = true; + pending = null; + continue; + } + if (!inHunk) continue; + + if (line.startsWith("+")) { + const body = line.slice(1); + if (body.length <= MAX_LINE_CHARS) { + if (pending) { + pending = updatePending(pending, body); + if (pending.depth <= 0) { + const kind = flushPending(pending); + if (kind) { + pushFinding(pending.startLine, kind); + if (findings.length >= maxFindings) return findings; + } + pending = null; + } + } else { + const kind = detectErrorSwallow(body); + if (kind) { + pushFinding(newLine, kind); + if (findings.length >= maxFindings) return findings; + } else { + const open = matchErrorOpen(body); + if (open) { + const braceIndex = body.indexOf("{", open.index ?? 0); + if (braceIndex >= 0) { + const depth = braceBalanceFrom(body, braceIndex); + if (depth > 0) { + pending = { + startLine: newLine, + binding: open[1] ?? null, + body: body.slice(braceIndex), + depth, + }; + } + } + } + } + } + } + newLine++; + } else if (!line.startsWith("-") && !line.startsWith("\\")) { + pending = null; + newLine++; + } else { + pending = null; + } + + if (findings.length >= maxFindings) return findings; + } + + return findings; +} + +/** Analyzer entrypoint: scan every changed scannable file's added lines for swallowed errors. */ +export async function scanErrorSwallow( + req: EnrichRequest, + signal?: AbortSignal, +): Promise { + const findings: ErrorSwallowFinding[] = []; + for (const file of req.files ?? []) { + if (signal?.aborted) throw new Error("analyzer_aborted"); + if (!file.patch) continue; + for (const finding of scanPatchForErrorSwallow(file.path, file.patch, { + maxFindings: MAX_FINDINGS - findings.length, + signal, + })) { + findings.push(finding); + if (findings.length >= MAX_FINDINGS) return findings; + } + } + return findings; +} diff --git a/review-enrichment/src/analyzers/registry.ts b/review-enrichment/src/analyzers/registry.ts index 152b6fd202..2d26e6a987 100644 --- a/review-enrichment/src/analyzers/registry.ts +++ b/review-enrichment/src/analyzers/registry.ts @@ -35,6 +35,7 @@ import { scanDebugLeftover } from "./debug-leftover.js"; import { scanDeepNesting } from "./deep-nesting.js"; import { scanI18nRegression } from "./i18n-regression.js"; import { scanErrorSwallow } from "./error-swallow.js"; +import { scanComplexity } from "./complexity.js"; import { scanFloatingPromise } from "./floating-promise.js"; import { scanSizeSmell } from "./size-smell.js"; import { scanA11yRegression } from "./a11y-regression.js"; @@ -1128,25 +1129,65 @@ export const ANALYZER_DESCRIPTORS = [ limits: { maxFindings: 25, maxLineChars: 2000 }, docs: { summary: - "Flags newly-added catch/except blocks that swallow the error — empty body, unused binding, or a bare `return null`.", - looksAt: "Added lines in changed non-test JS/TS/Python source files.", - reports: "File, line, and kind: empty-catch, unused-binding, or return-null.", + "Flags newly-added catch/except blocks (and Go if-err checks) that swallow or mishandle the error — empty body, unused binding, a bare `return null`/`nil`, or a Python bare `except:` naming no exception type.", + looksAt: "Added lines in changed non-test JS/TS/Python/Go source files.", + reports: "File, line, and kind: empty-catch, unused-binding, return-null, or bare-except.", network: "Pure local analyzer. No external network call.", notes: - "Multiline catch bodies are collected with brace balance. Catches that log, rethrow, or reference the binding are not flagged. Brace counting is character-level (string literals are not stripped).", + "Multiline catch/if-err bodies are collected with brace balance (Go's `if err != nil { … }`, including the if-with-initializer form, is treated the same as a JS/TS catch). Handlers that log, rethrow/panic, or reference the checked binding are not flagged. Python's bare `except:` is flagged regardless of body — it catches SystemExit/KeyboardInterrupt too. Brace counting is character-level (string literals are not stripped).", }, render: (findings, helpers) => { if (!findings.length) return []; - const lines = ["### Swallowed errors (empty catch / unused binding / return null)"]; + const explain = (kind: (typeof findings)[number]["kind"]): string => { + switch (kind) { + case "empty-catch": + return "empty-catch — the error is checked/caught but the handling block is empty"; + case "unused-binding": + return "unused-binding — the error is checked/caught but never referenced, logged, or returned"; + case "return-null": + return "return-null — returns null/nil instead of propagating the error"; + case "bare-except": + return "bare-except — catches every exception (including SystemExit/KeyboardInterrupt), no type named"; + } + }; + const lines = ["### Swallowed errors (empty catch / unused binding / return null / bare except)"]; for (const item of findings) { - lines.push( - `- ${helpers.safeCodeSpan(`${item.file}:${item.line}`)} — ${helpers.safeCodeSpan(item.kind)}`, - ); + lines.push(`- ${helpers.safeCodeSpan(`${item.file}:${item.line}`)} — ${explain(item.kind)}`); } return lines; }, run: (req, { signal }) => scanErrorSwallow(req, signal), }), + descriptor({ + name: "complexity", + title: "Approximate cyclomatic complexity", + category: "quality", + cost: "local", + defaultEnabled: true, + requires: ["files"], + limits: { maxFindings: 25, maxComplexity: 10, maxLineChars: 2000 }, + docs: { + summary: + "Flags a newly-added function whose approximate cyclomatic complexity (branch/loop/logical-operator density, computed on the diff-visible lines) exceeds a threshold.", + looksAt: + "Added lines in changed non-test TS/JS source files, starting from a named function declaration or a const/let/var-assigned arrow function whose opening line is part of the diff.", + reports: "File, line, the detected function name, the measured complexity, and the configured threshold.", + network: "Pure local analyzer. No external network call.", + notes: + "Diff-hunk approximation, not a whole-function true McCabe count: REES has no full-file content, so this counts if/for/while/case/catch/&&/||/?? token occurrences across the function's ADDED body lines only (1 + count), the same function-boundary detection size-smell.ts (#2019) uses for 'big-function'. A function whose signature line is not part of the diff is not scored. Distinct from deep-nesting (#2030), which measures brace NESTING depth, a readability smell, not decision-point density. Ternary (`? :`) is intentionally excluded — see the analyzer source header for why.", + }, + render: (findings, helpers) => { + if (!findings.length) return []; + const lines = ["### Approximate cyclomatic complexity (diff-visible branch/loop/logical-operator density)"]; + for (const item of findings) { + const location = helpers.safeCodeSpan(`${item.file}:${item.line}`); + const name = helpers.safeCodeSpan(item.name); + lines.push(`- ${location} — ${name}: approx. complexity ${item.complexity} (threshold ${item.threshold})`); + } + return lines; + }, + run: (req, { signal }) => scanComplexity(req, signal), + }), descriptor({ name: "unsafeAny", title: "Unsafe any (TS)", diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index bd88b25137..b450e75efa 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -496,6 +496,7 @@ export function renderBrief( lines.push(...renderDescriptorSection("floatingPromise", findings.floatingPromise)); lines.push(...renderDescriptorSection("deepNesting", findings.deepNesting)); lines.push(...renderDescriptorSection("errorSwallow", findings.errorSwallow)); + lines.push(...renderDescriptorSection("complexity", findings.complexity)); lines.push(...renderDescriptorSection("unsafeAny", findings.unsafeAny)); lines.push(...renderDescriptorSection("a11y", findings.a11y)); lines.push(...renderDescriptorSection("i18n", findings.i18n)); diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index cb9431bfd4..983a8b2428 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -530,12 +530,30 @@ export interface I18nFinding { line: number; } -/** A swallowed-error catch/except block newly added in the diff (#2014, part of #1499). - * Reports file, line, and kind only — never catch body content. */ +/** A swallowed or mishandled error/exception newly added in the diff (#2014, extended for Go + Python + * bare-except by #1477). Reports file, line, and kind only — never catch/handler body content. `empty-catch`: + * an empty JS/TS `catch`, Go `if err != nil {}`, or Python `except: pass` body. `unused-binding`: the bound + * exception/checked error variable is never referenced, logged, or re-thrown. `return-null`: the handler + * returns a bare `null`/`nil` instead of propagating the error. `bare-except`: a Python `except:` naming no + * exception type, which catches everything (including `SystemExit`/`KeyboardInterrupt`) regardless of body. */ export interface ErrorSwallowFinding { file: string; line: number; - kind: "empty-catch" | "unused-binding" | "return-null"; + kind: "empty-catch" | "unused-binding" | "return-null" | "bare-except"; +} + +/** An approximate cyclomatic complexity for a newly-added function, computed from diff-visible added lines only + * (#1477) — `1 + a count of if/for/while/case/catch/&&/||/?? tokens` within the function's added body lines, + * not a whole-function true McCabe count (REES has no full-file content to walk). Distinct from deep-nesting + * (#2030), which measures brace NESTING depth, a readability smell, not decision-point density. Reports file, + * line, the detected function name, the measured complexity, and the configured threshold — never source + * content. */ +export interface ComplexityFinding { + file: string; + line: number; + name: string; + complexity: number; + threshold: number; } /** Deep control-flow nesting newly added in the diff (#2030, part of #1499). @@ -698,6 +716,7 @@ export interface BriefFindings { floatingPromise?: FloatingPromiseFinding[]; deepNesting?: DeepNestingFinding[]; errorSwallow?: ErrorSwallowFinding[]; + complexity?: ComplexityFinding[]; unsafeAny?: UnsafeAnyFinding[]; a11y?: A11yFinding[]; i18n?: I18nFinding[]; diff --git a/review-enrichment/test/analyzer-registry.test.ts b/review-enrichment/test/analyzer-registry.test.ts index 1042b62161..1ccfec0b71 100644 --- a/review-enrichment/test/analyzer-registry.test.ts +++ b/review-enrichment/test/analyzer-registry.test.ts @@ -52,6 +52,7 @@ const EXPECTED_ANALYZERS = [ "floatingPromise", "deepNesting", "errorSwallow", + "complexity", "unsafeAny", "a11y", "i18n", diff --git a/review-enrichment/test/complexity.test.ts b/review-enrichment/test/complexity.test.ts new file mode 100644 index 0000000000..64762f9ac8 --- /dev/null +++ b/review-enrichment/test/complexity.test.ts @@ -0,0 +1,199 @@ +// Units for the approximate cyclomatic-complexity analyzer (#1477). Own file so concurrent analyzer PRs don't collide. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + countDecisionPoints, + DEFAULT_MAX_COMPLEXITY, + functionNameFromLine, + scanComplexity, + scanPatchForComplexity, +} from "../dist/analyzers/complexity.js"; +import { renderBrief } from "../dist/render.js"; + +const patchOf = (lines: string[]) => + `@@ -1,0 +1,${lines.length} @@\n${lines.map((l) => `+${l}`).join("\n")}`; + +test("countDecisionPoints: counts if/for/while/catch/case and logical operators", () => { + assert.equal(countDecisionPoints(""), 0); + assert.equal(countDecisionPoints("if (a) {"), 1); + assert.equal(countDecisionPoints("for (const x of y) {"), 1); + assert.equal(countDecisionPoints("for await (const x of y) {"), 1); + assert.equal(countDecisionPoints("while (true) {"), 1); + assert.equal(countDecisionPoints("catch (e) {"), 1); + assert.equal(countDecisionPoints("catch {"), 1); + assert.equal(countDecisionPoints("case 1:"), 1); + assert.equal(countDecisionPoints("a && b || c ?? d"), 3); + assert.equal(countDecisionPoints("if (a) { if (b) {} }"), 2); +}); + +test("countDecisionPoints: does not count default, ternary, or optional chaining", () => { + assert.equal(countDecisionPoints("default:"), 0); + assert.equal(countDecisionPoints("const x = a ? b : c;"), 0); + assert.equal(countDecisionPoints("a?.b ?? c"), 1); + assert.equal(countDecisionPoints("function f(x?: number) {"), 0); +}); + +test("countDecisionPoints: does not falsely match identifiers containing the token as a substring", () => { + assert.equal(countDecisionPoints("const testCase1 = getCase();"), 0); + assert.equal(countDecisionPoints("const forecast = 1;"), 0); +}); + +test("functionNameFromLine: detects named functions and arrow-assigned functions", () => { + assert.equal(functionNameFromLine("function run() {"), "run"); + assert.equal(functionNameFromLine("export function run() {"), "run"); + assert.equal(functionNameFromLine("const run = () => {"), "run"); + assert.equal(functionNameFromLine("export const run = async () => {"), "run"); + assert.equal(functionNameFromLine("const run = (x: number) => {"), "run"); +}); + +test("functionNameFromLine: a plain (non-arrow) function expression assigned to a const is out of scope", () => { + // Same structural scope as size-smell.ts's function detection: only named `function` declarations and + // arrow functions are recognized, not a bare `function` expression assigned via const/let/var. + assert.equal(functionNameFromLine("const run = function () {"), undefined); +}); + +test("functionNameFromLine: returns undefined for non-function lines and comments", () => { + assert.equal(functionNameFromLine("if (a) {"), undefined); + assert.equal(functionNameFromLine(" return x;"), undefined); + assert.equal(functionNameFromLine("// function run() {"), undefined); + assert.equal(functionNameFromLine(" * function run() {"), undefined); +}); + +test("scanPatchForComplexity: flags a function whose approximate complexity exceeds the threshold", () => { + const ifCount = DEFAULT_MAX_COMPLEXITY + 1; + const lines = [ + "function big() {", + ...Array.from({ length: ifCount }, (_, i) => ` if (cond${i}) {}`), + "}", + ]; + const findings = scanPatchForComplexity("src/widget.ts", patchOf(lines)); + assert.equal(findings.length, 1); + assert.deepEqual(findings[0], { + file: "src/widget.ts", + line: 1, + name: "big", + complexity: 1 + ifCount, + threshold: DEFAULT_MAX_COMPLEXITY, + }); +}); + +test("scanPatchForComplexity: does not flag a function at or under the threshold", () => { + const ifCount = DEFAULT_MAX_COMPLEXITY - 1; + const lines = [ + "function ok() {", + ...Array.from({ length: ifCount }, (_, i) => ` if (cond${i}) {}`), + "}", + ]; + assert.deepEqual(scanPatchForComplexity("src/widget.ts", patchOf(lines)), []); +}); + +test("scanPatchForComplexity: scores sibling functions independently in the same hunk", () => { + const ifCount = DEFAULT_MAX_COMPLEXITY + 1; + const lines = [ + "function complicated() {", + ...Array.from({ length: ifCount }, (_, i) => ` if (cond${i}) {}`), + "}", + "function simple() {", + " if (a) {}", + "}", + ]; + const findings = scanPatchForComplexity("src/widget.ts", patchOf(lines)); + assert.equal(findings.length, 1); + assert.equal(findings[0]?.name, "complicated"); +}); + +test("scanPatchForComplexity: arrow functions are scored the same as named functions", () => { + const ifCount = DEFAULT_MAX_COMPLEXITY + 1; + const lines = [ + "export const big = () => {", + ...Array.from({ length: ifCount }, (_, i) => ` if (cond${i}) {}`), + "};", + ]; + const findings = scanPatchForComplexity("src/widget.ts", patchOf(lines)); + assert.equal(findings.length, 1); + assert.equal(findings[0]?.name, "big"); +}); + +test("scanPatchForComplexity: comment-only added lines do not inflate complexity", () => { + const ifCount = DEFAULT_MAX_COMPLEXITY - 1; + const lines = [ + "function ok() {", + ...Array.from({ length: ifCount }, (_, i) => ` if (cond${i}) {}`), + " // if (extra) { would push this over the threshold if counted }", + "}", + ]; + assert.deepEqual(scanPatchForComplexity("src/widget.ts", patchOf(lines)), []); +}); + +test("scanPatchForComplexity: an edit to an existing function's body (signature not in the diff) is not scored", () => { + const patch = [ + "@@ -5,4 +5,6 @@", + " function existing() {", + "+ if (a) {}", + "+ if (b) {}", + " return x;", + " }", + ].join("\n"); + assert.deepEqual(scanPatchForComplexity("src/widget.ts", patch), []); +}); + +test("scanPatchForComplexity: a context line flushes an in-progress function using its partial count", () => { + const ifCount = DEFAULT_MAX_COMPLEXITY + 1; + const patch = [ + "@@ -1,0 +1,3 @@", + "+function big() {", + ...Array.from({ length: ifCount }, (_, i) => `+ if (cond${i}) {}`), + " // unchanged context line interrupts the run before the closing brace", + ].join("\n"); + const findings = scanPatchForComplexity("src/widget.ts", patch); + assert.equal(findings.length, 1); + assert.equal(findings[0]?.name, "big"); + assert.equal(findings[0]?.complexity, 1 + ifCount); +}); + +test("scanPatchForComplexity: skips non-TS/JS files and test files", () => { + const ifCount = DEFAULT_MAX_COMPLEXITY + 1; + const lines = [ + "function big() {", + ...Array.from({ length: ifCount }, (_, i) => ` if (cond${i}) {}`), + "}", + ]; + assert.deepEqual(scanPatchForComplexity("src/widget.py", patchOf(lines)), []); + assert.deepEqual(scanPatchForComplexity("src/widget.test.ts", patchOf(lines)), []); +}); + +test("scanPatchForComplexity: respects a custom maxComplexity limit", () => { + const lines = ["function f() {", " if (a) {}", " if (b) {}", "}"]; + assert.deepEqual(scanPatchForComplexity("src/widget.ts", patchOf(lines), { maxComplexity: 2 }), [ + { file: "src/widget.ts", line: 1, name: "f", complexity: 3, threshold: 2 }, + ]); + assert.deepEqual(scanPatchForComplexity("src/widget.ts", patchOf(lines), { maxComplexity: 3 }), []); +}); + +test("scanPatchForComplexity: respects the findings cap across many functions", () => { + const ifCount = DEFAULT_MAX_COMPLEXITY + 1; + const functionBlock = (i: number) => [ + `function big${i}() {`, + ...Array.from({ length: ifCount }, (_, j) => ` if (cond${j}) {}`), + "}", + ]; + const lines = Array.from({ length: 30 }, (_, i) => functionBlock(i)).flat(); + assert.equal(scanPatchForComplexity("src/widget.ts", patchOf(lines), { maxFindings: 3 }).length, 3); +}); + +test("scanComplexity: aggregates across files and renders a public-safe brief", async () => { + const ifCount = DEFAULT_MAX_COMPLEXITY + 1; + const findings = await scanComplexity({ + files: [ + { + path: "src/a.ts", + patch: patchOf(["function big() {", ...Array.from({ length: ifCount }, (_, i) => ` if (cond${i}) {}`), "}"]), + }, + ], + }); + assert.equal(findings.length, 1); + const { promptSection } = renderBrief({ complexity: findings }); + assert.match(promptSection, /Approximate cyclomatic complexity/); + assert.match(promptSection, /src\/a\.ts:1/); + assert.match(promptSection, /big/); +}); diff --git a/review-enrichment/test/error-swallow.test.ts b/review-enrichment/test/error-swallow.test.ts index 8bd3645184..c71fa9a1f9 100644 --- a/review-enrichment/test/error-swallow.test.ts +++ b/review-enrichment/test/error-swallow.test.ts @@ -1,93 +1,152 @@ -// Units for the error-swallow analyzer (#2014). Own file so concurrent analyzer PRs don't collide. -import { test } from "node:test"; -import assert from "node:assert/strict"; -import { - detectErrorSwallow, - scanErrorSwallow, - scanPatchForErrorSwallow, -} from "../dist/analyzers/error-swallow.js"; -import { renderBrief } from "../dist/render.js"; - -const patchOf = (lines: string[]) => - `@@ -1,0 +1,${lines.length} @@\n${lines.map((l) => `+${l}`).join("\n")}`; - -test("detectErrorSwallow: flags empty catches and return-null handlers", () => { - assert.equal(detectErrorSwallow("try { f(); } catch (e) {}"), "empty-catch"); - assert.equal(detectErrorSwallow("try { f(); } catch {}"), "empty-catch"); - assert.equal(detectErrorSwallow("try { f(); } catch (e) { return null; }"), "return-null"); - assert.equal(detectErrorSwallow("except ValueError: pass"), "empty-catch"); - assert.equal(detectErrorSwallow("except ValueError as err: pass"), "unused-binding"); -}); - -test("detectErrorSwallow: does not flag catches that log, rethrow, or use the binding", () => { - assert.equal(detectErrorSwallow("try { f(); } catch (e) { console.error(e); }"), null); - assert.equal(detectErrorSwallow("try { f(); } catch (e) { throw e; }"), null); - assert.equal(detectErrorSwallow("try { f(); } catch (e) { cleanup(e); }"), null); - assert.equal(detectErrorSwallow("try { f(); } catch ($err) { handle($err); }"), null); -}); - -test("detectErrorSwallow: brace-balances nested blocks on one line", () => { - assert.equal(detectErrorSwallow("try { f(); } catch (e) { if (x) {} handle(e); }"), null); -}); - -test("detectErrorSwallow: flags unused bindings on single-line catches", () => { - assert.equal(detectErrorSwallow("try { f(); } catch (err) { cleanup(); }"), "unused-binding"); -}); - -test("scanPatchForErrorSwallow: flags added lines with correct locations", () => { - const findings = scanPatchForErrorSwallow( - "src/worker.ts", - patchOf([ - "export async function run() {", - " try {", - " await load();", - " } catch (e) {}", - "}", - ]), - ); - assert.deepEqual(findings, [{ file: "src/worker.ts", line: 4, kind: "empty-catch" }]); -}); - -test("scanPatchForErrorSwallow: supports multi-line catch blocks on added lines", () => { - const patch = [ - "@@ -1,0 +1,5 @@", - "+try {", - "+ await load();", - "+} catch (err) {", - "+ return null;", - "+}", - ].join("\n"); - assert.deepEqual(scanPatchForErrorSwallow("src/worker.ts", patch), [ - { file: "src/worker.ts", line: 3, kind: "return-null" }, - ]); -}); - -test("scanPatchForErrorSwallow: skips test files", () => { - assert.deepEqual( - scanPatchForErrorSwallow("src/worker.test.ts", patchOf(["catch (e) {}"])), - [], - ); -}); - -test("scanPatchForErrorSwallow: respects the findings cap", () => { - const lines = Array.from({ length: 30 }, () => "catch (e) {}"); - assert.equal(scanPatchForErrorSwallow("src/a.ts", patchOf(lines), { maxFindings: 3 }).length, 3); -}); - -test("scanErrorSwallow: aggregates across files and renders a public-safe brief", async () => { - const findings = await scanErrorSwallow({ - files: [ - { path: "src/a.ts", patch: patchOf(["catch (e) {}"]) }, - { path: "lib/b.py", patch: patchOf(["except RuntimeError: pass"]) }, - ], - }); - assert.deepEqual(findings, [ - { file: "src/a.ts", line: 1, kind: "empty-catch" }, - { file: "lib/b.py", line: 1, kind: "empty-catch" }, - ]); - - const { promptSection } = renderBrief({ errorSwallow: findings }); - assert.match(promptSection, /Swallowed errors/); - assert.match(promptSection, /src\/a\.ts:1/); - assert.doesNotMatch(promptSection, /catch \(e\)/); -}); +// Units for the error-swallow analyzer (#2014). Own file so concurrent analyzer PRs don't collide. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + detectErrorSwallow, + scanErrorSwallow, + scanPatchForErrorSwallow, +} from "../dist/analyzers/error-swallow.js"; +import { renderBrief } from "../dist/render.js"; + +const patchOf = (lines: string[]) => + `@@ -1,0 +1,${lines.length} @@\n${lines.map((l) => `+${l}`).join("\n")}`; + +test("detectErrorSwallow: flags empty catches and return-null handlers", () => { + assert.equal(detectErrorSwallow("try { f(); } catch (e) {}"), "empty-catch"); + assert.equal(detectErrorSwallow("try { f(); } catch {}"), "empty-catch"); + assert.equal(detectErrorSwallow("try { f(); } catch (e) { return null; }"), "return-null"); + assert.equal(detectErrorSwallow("except ValueError: pass"), "empty-catch"); + assert.equal(detectErrorSwallow("except ValueError as err: pass"), "unused-binding"); +}); + +test("detectErrorSwallow: does not flag catches that log, rethrow, or use the binding", () => { + assert.equal(detectErrorSwallow("try { f(); } catch (e) { console.error(e); }"), null); + assert.equal(detectErrorSwallow("try { f(); } catch (e) { throw e; }"), null); + assert.equal(detectErrorSwallow("try { f(); } catch (e) { cleanup(e); }"), null); + assert.equal(detectErrorSwallow("try { f(); } catch ($err) { handle($err); }"), null); +}); + +test("detectErrorSwallow: brace-balances nested blocks on one line", () => { + assert.equal(detectErrorSwallow("try { f(); } catch (e) { if (x) {} handle(e); }"), null); +}); + +test("detectErrorSwallow: flags unused bindings on single-line catches", () => { + assert.equal(detectErrorSwallow("try { f(); } catch (err) { cleanup(); }"), "unused-binding"); +}); + +test("scanPatchForErrorSwallow: flags added lines with correct locations", () => { + const findings = scanPatchForErrorSwallow( + "src/worker.ts", + patchOf([ + "export async function run() {", + " try {", + " await load();", + " } catch (e) {}", + "}", + ]), + ); + assert.deepEqual(findings, [{ file: "src/worker.ts", line: 4, kind: "empty-catch" }]); +}); + +test("scanPatchForErrorSwallow: supports multi-line catch blocks on added lines", () => { + const patch = [ + "@@ -1,0 +1,5 @@", + "+try {", + "+ await load();", + "+} catch (err) {", + "+ return null;", + "+}", + ].join("\n"); + assert.deepEqual(scanPatchForErrorSwallow("src/worker.ts", patch), [ + { file: "src/worker.ts", line: 3, kind: "return-null" }, + ]); +}); + +test("scanPatchForErrorSwallow: skips test files", () => { + assert.deepEqual( + scanPatchForErrorSwallow("src/worker.test.ts", patchOf(["catch (e) {}"])), + [], + ); +}); + +test("scanPatchForErrorSwallow: respects the findings cap", () => { + const lines = Array.from({ length: 30 }, () => "catch (e) {}"); + assert.equal(scanPatchForErrorSwallow("src/a.ts", patchOf(lines), { maxFindings: 3 }).length, 3); +}); + +test("scanErrorSwallow: aggregates across files and renders a public-safe brief", async () => { + const findings = await scanErrorSwallow({ + files: [ + { path: "src/a.ts", patch: patchOf(["catch (e) {}"]) }, + { path: "lib/b.py", patch: patchOf(["except RuntimeError: pass"]) }, + ], + }); + assert.deepEqual(findings, [ + { file: "src/a.ts", line: 1, kind: "empty-catch" }, + { file: "lib/b.py", line: 1, kind: "empty-catch" }, + ]); + + const { promptSection } = renderBrief({ errorSwallow: findings }); + assert.match(promptSection, /Swallowed errors/); + assert.match(promptSection, /src\/a\.ts:1/); + assert.doesNotMatch(promptSection, /catch \(e\)/); +}); + +test("detectErrorSwallow: flags Go if-err checks that swallow the error", () => { + assert.equal(detectErrorSwallow("if err != nil {}"), "empty-catch"); + assert.equal(detectErrorSwallow("if err != nil { return }"), "unused-binding"); + assert.equal(detectErrorSwallow("if err != nil { return nil }"), "return-null"); + assert.equal(detectErrorSwallow("if writeErr != nil { return nil }"), "return-null"); +}); + +test("detectErrorSwallow: does not flag Go if-err checks that propagate, log, or panic", () => { + assert.equal(detectErrorSwallow("if err != nil { return err }"), null); + assert.equal(detectErrorSwallow("if err != nil { return fmt.Errorf(\"x: %w\", err) }"), null); + assert.equal(detectErrorSwallow("if err != nil { log.Println(err) }"), null); + assert.equal(detectErrorSwallow("if err != nil { panic(err) }"), null); +}); + +test("detectErrorSwallow: does not mistake an unrelated nil-pointer check for error handling", () => { + assert.equal(detectErrorSwallow("if node != nil { return defaultNode }"), null); +}); + +test("detectErrorSwallow: recognizes the Go if-with-initializer form", () => { + assert.equal(detectErrorSwallow("if err := doStuff(); err != nil { return }"), "unused-binding"); + assert.equal(detectErrorSwallow("if err := doStuff(); err != nil { return err }"), null); +}); + +test("detectErrorSwallow: flags a bare Python except naming no exception type, regardless of body", () => { + assert.equal(detectErrorSwallow("except:"), "bare-except"); + assert.equal(detectErrorSwallow(" except: "), "bare-except"); + assert.equal(detectErrorSwallow("except: # noqa"), "bare-except"); +}); + +test("detectErrorSwallow: does not flag a Python except naming a real exception type", () => { + assert.equal(detectErrorSwallow("except Exception:"), null); + assert.equal(detectErrorSwallow("except (TypeError, ValueError):"), null); +}); + +test("scanPatchForErrorSwallow: supports multi-line Go if-err blocks on added lines", () => { + const patch = [ + "@@ -1,0 +1,4 @@", + "+resp, err := doStuff()", + "+if err != nil {", + "+ return", + "+}", + ].join("\n"); + assert.deepEqual(scanPatchForErrorSwallow("main.go", patch), [ + { file: "main.go", line: 2, kind: "unused-binding" }, + ]); +}); + +test("scanPatchForErrorSwallow: flags a bare except on an added Python line", () => { + const findings = scanPatchForErrorSwallow("lib/b.py", patchOf(["except:", " handle()"])); + assert.deepEqual(findings, [{ file: "lib/b.py", line: 1, kind: "bare-except" }]); +}); + +test("scanPatchForErrorSwallow: skips Go test files", () => { + assert.deepEqual( + scanPatchForErrorSwallow("main_test.go", patchOf(["if err != nil {}"])), + [], + ); +}); diff --git a/src/review/enrichment-analyzer-names.ts b/src/review/enrichment-analyzer-names.ts index 3a8b7d47a1..e6580b36cf 100644 --- a/src/review/enrichment-analyzer-names.ts +++ b/src/review/enrichment-analyzer-names.ts @@ -46,6 +46,7 @@ export const REES_ANALYZER_NAMES = [ "floatingPromise", "deepNesting", "errorSwallow", + "complexity", "unsafeAny", "a11y", "i18n",