diff --git a/.env.example b/.env.example index dc561a2300..523703a037 100644 --- a/.env.example +++ b/.env.example @@ -68,25 +68,25 @@ 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,commitLint +# conflictMarker,debugLeftover,sizeSmell,errorSwallow,commitLint # # 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 +# debugLeftover,sizeSmell,errorSwallow # 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,commitLint +# todoMarker,magicNumber,conflictMarker,debugLeftover,sizeSmell,errorSwallow,commitLint # 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,commitLint +# conflictMarker,debugLeftover,sizeSmell,errorSwallow,commitLint # 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 730e7c655f..c227a5d5cf 100644 --- a/apps/gittensory-ui/src/lib/rees-analyzers.ts +++ b/apps/gittensory-ui/src/lib/rees-analyzers.ts @@ -981,6 +981,28 @@ export const REES_ANALYZERS = [ "File length is estimated from hunk headers (the visible patch), not a full checkout. Function detection is structural (`function` / arrow-with-brace) and counts added body lines until brace balance returns to zero.", }, }, + { + name: "errorSwallow", + title: "Error swallowing", + category: "quality", + cost: "local", + defaultEnabled: true, + profiles: ["fast", "balanced", "deep"], + requires: ["files"], + limits: { + maxFindings: 25, + maxLineChars: 2000, + }, + docs: { + summary: + "Flags newly added catch/except blocks that silently discard errors — empty bodies, unused bindings, or a lone return null.", + looksAt: "Added lines in changed JS/TS/Python source files (non-test).", + reports: "File, line, and kind: empty-catch, unused-binding, or return-null.", + network: "Pure local analyzer. No external network call.", + notes: + "Python `except: pass` is allowed. Catch blocks that log, rethrow, or reference the caught binding are not flagged.", + }, + }, { name: "commitLint", title: "Conventional-commit subjects", diff --git a/review-enrichment/analyzer-metadata.json b/review-enrichment/analyzer-metadata.json index 27e07e1ed7..81d11136b4 100644 --- a/review-enrichment/analyzer-metadata.json +++ b/review-enrichment/analyzer-metadata.json @@ -1107,6 +1107,32 @@ "notes": "File length is estimated from hunk headers (the visible patch), not a full checkout. Function detection is structural (`function` / arrow-with-brace) and counts added body lines until brace balance returns to zero." } }, + { + "name": "errorSwallow", + "title": "Error swallowing", + "category": "quality", + "cost": "local", + "defaultEnabled": true, + "profiles": [ + "fast", + "balanced", + "deep" + ], + "requires": [ + "files" + ], + "limits": { + "maxFindings": 25, + "maxLineChars": 2000 + }, + "docs": { + "summary": "Flags newly added catch/except blocks that silently discard errors — empty bodies, unused bindings, or a lone return null.", + "looksAt": "Added lines in changed JS/TS/Python source files (non-test).", + "reports": "File, line, and kind: empty-catch, unused-binding, or return-null.", + "network": "Pure local analyzer. No external network call.", + "notes": "Python `except: pass` is allowed. Catch blocks that log, rethrow, or reference the caught binding are not flagged." + } + }, { "name": "commitLint", "title": "Conventional-commit subjects", diff --git a/review-enrichment/src/analyzers/error-swallow.ts b/review-enrichment/src/analyzers/error-swallow.ts new file mode 100644 index 0000000000..5769451592 --- /dev/null +++ b/review-enrichment/src/analyzers/error-swallow.ts @@ -0,0 +1,196 @@ +// Error-swallow analyzer (#2014). Flags newly-added catch/except blocks that silently discard errors — +// empty bodies, unused bindings, or a lone `return null` with no log/rethrow. Pure compute over added diff +// lines; no network. JS/TS/Python only; Python `except: pass` is intentionally allowed. +import type { EnrichRequest, ErrorSwallowFinding } from "../types.js"; +import { codeOnly } from "./secret-log.js"; +import { isTestPath } from "./test-ratio.js"; + +const MAX_FINDINGS = 25; +const MAX_LINE_CHARS = 2000; +const MAX_MULTILINE_ADDED_LINES = 50; + +const SOURCE_EXTS = new Set(["ts", "tsx", "mts", "cts", "js", "jsx", "mjs", "cjs", "py"]); + +const JS_CATCH_RE = /\bcatch\s*(?:\(\s*([A-Za-z_$][\w$]*)\s*\))?\s*\{([\s\S]*)\}/; +const PYTHON_EXCEPT_RE = /^\s*except\b(?:\s+([^:\n]+?))?(?:\s+as\s+([A-Za-z_]\w*))?\s*:\s*(.*)$/; + +type ScanLimits = { + maxFindings?: number; + signal?: AbortSignal; +}; + +function sourceExtOf(path: string): string | null { + const match = /\.([A-Za-z0-9]+)$/.exec(path); + return match ? match[1]!.toLowerCase() : null; +} + +export function isErrorSwallowSourcePath(path: string): boolean { + const ext = sourceExtOf(path); + return Boolean(ext && SOURCE_EXTS.has(ext) && !isTestPath(path)); +} + +function isAddedPatchLine(line: string): boolean { + return line.startsWith("+") && !line.startsWith("+++"); +} + +function bodySwallowsError(body: string, binding: string | null, isPython: boolean): ErrorSwallowFinding["kind"] | null { + const inner = codeOnly(body).trim(); + if (!inner) return "empty-catch"; + if (isPython && /^pass(?:\s+#.*)?$/.test(inner)) return null; + + if (/^return\s+(?:null|None)\s*;?$/.test(inner) && !/\bthrow\b/.test(inner) && !mentionsLogOrBinding(inner, binding)) { + return "return-null"; + } + + if (/\bthrow\b/.test(inner)) return null; + if (mentionsLogOrBinding(inner, binding)) return null; + + if (binding) return "unused-binding"; + return null; +} + +function mentionsLogOrBinding(body: string, binding: string | null): boolean { + if (binding && new RegExp(`\\b${binding.replace(/[$]/g, "\\$")}\\b`).test(body)) return true; + return /\b(console\.|logger\.|log\.|print\s*\(|Sentry\.|captureException\b|reportError\b|\.error\s*\(|\.warn\s*\()/i.test(body); +} + +function classifyJsCatchText(text: string): ErrorSwallowFinding["kind"] | null { + const match = JS_CATCH_RE.exec(codeOnly(text)); + if (!match) return null; + return bodySwallowsError(match[2] ?? "", match[1] ?? null, false); +} + +function braceBlockComplete(text: string): boolean { + const code = codeOnly(text); + let depth = 0; + let seenOpen = false; + for (const ch of code) { + if (ch === "{") { + depth += 1; + seenOpen = true; + } else if (ch === "}") { + depth -= 1; + } + } + return seenOpen && depth <= 0; +} + +/** Collect consecutive added lines until a `{`/`}` block closes. Pure. */ +function collectAddedBraceBlock(lines: string[], startIndex: number): { text: string; lineCount: number } | null { + let text = ""; + let lineCount = 0; + for (let i = startIndex; i < lines.length && lineCount < MAX_MULTILINE_ADDED_LINES; i += 1) { + const line = lines[i]!; + if (!isAddedPatchLine(line)) break; + text += (lineCount > 0 ? "\n" : "") + line.slice(1); + lineCount += 1; + if (braceBlockComplete(text)) break; + } + return lineCount > 0 ? { text, lineCount } : null; +} + +/** Classify one added JS/TS catch (single- or multi-line text), or null when clean / out of scope. Pure. */ +export function detectJsCatchSwallow(line: string): ErrorSwallowFinding["kind"] | null { + if (!/\bcatch\b/.test(line)) return null; + return classifyJsCatchText(line); +} + +/** Classify one added Python except line (and optional immediate next added body line), or null. Pure. */ +export function detectPythonExceptSwallow(line: string, nextAddedLine?: string | null): ErrorSwallowFinding["kind"] | null { + const match = PYTHON_EXCEPT_RE.exec(line); + if (!match) return null; + const binding = match[2] ?? null; + let body = (match[3] ?? "").trim(); + if (!body && nextAddedLine) body = nextAddedLine.trim(); + if (/^\s*pass\s*$/.test(body) || body === "pass") return null; + return bodySwallowsError(body, binding, true); +} + +function pythonExceptUsesNextAddedLine(line: string): boolean { + const match = PYTHON_EXCEPT_RE.exec(line); + return Boolean(match && !(match[3] ?? "").trim()); +} + +/** Scan one file patch's added lines for error-swallowing catch blocks, line-cited via hunk headers. Pure. */ +export function scanPatchForErrorSwallow( + path: string, + patch: string, + limits: ScanLimits = {}, +): ErrorSwallowFinding[] { + const maxFindings = limits.maxFindings ?? MAX_FINDINGS; + if (maxFindings <= 0 || !isErrorSwallowSourcePath(path)) return []; + + const isPython = /\.pyi?$/i.test(path); + const findings: ErrorSwallowFinding[] = []; + const lines = patch.split("\n"); + let newLine = 0; + let inHunk = false; + + for (let index = 0; index < lines.length; index += 1) { + if (limits.signal?.aborted) throw new Error("analyzer_aborted"); + const line = lines[index]!; + const hunk = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(line); + if (hunk) { + newLine = Number(hunk[1]); + inHunk = true; + continue; + } + if (!inHunk) continue; + + if (isAddedPatchLine(line)) { + const body = line.slice(1); + if (body.length > MAX_LINE_CHARS) { + newLine += 1; + continue; + } + + let kind: ErrorSwallowFinding["kind"] | null = null; + let skipAddedLines = 0; + + if (isPython) { + const nextLine = lines[index + 1]; + const nextAdded = nextLine && isAddedPatchLine(nextLine) ? nextLine.slice(1) : null; + kind = detectPythonExceptSwallow(body, nextAdded); + if (pythonExceptUsesNextAddedLine(body) && nextAdded) skipAddedLines = 1; + } else if (/\bcatch\b/.test(body)) { + kind = detectJsCatchSwallow(body); + if (!kind) { + const block = collectAddedBraceBlock(lines, index); + if (block) { + kind = classifyJsCatchText(block.text); + skipAddedLines = block.lineCount - 1; + } + } + } + + if (kind) { + findings.push({ file: path, line: newLine, kind }); + if (findings.length >= maxFindings) return findings; + } + + newLine += 1 + skipAddedLines; + index += skipAddedLines; + } else if (!line.startsWith("-") && !line.startsWith("\\")) { + newLine += 1; + } + } + + return findings; +} + +/** Analyzer entrypoint: scan every changed non-test source file's added lines for error swallowing. */ +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 914e52f4b6..6410a74185 100644 --- a/review-enrichment/src/analyzers/registry.ts +++ b/review-enrichment/src/analyzers/registry.ts @@ -33,6 +33,7 @@ import { scanMagicNumbers } from "./magic-number.js"; import { scanConflictMarkers } from "./conflict-marker.js"; import { scanDebugLeftover } from "./debug-leftover.js"; import { scanSizeSmell } from "./size-smell.js"; +import { scanErrorSwallow } from "./error-swallow.js"; import { scanCommitLint } from "./commit-lint.js"; import { scanTerminology } from "./terminology.js"; import { scanTodoMarker } from "./todo-marker.js"; @@ -1046,6 +1047,35 @@ export const ANALYZER_DESCRIPTORS = [ }, run: (req, { signal }) => scanSizeSmell(req, signal), }), + descriptor({ + name: "errorSwallow", + title: "Error swallowing", + category: "quality", + cost: "local", + defaultEnabled: true, + requires: ["files"], + limits: { maxFindings: 25, maxLineChars: 2000 }, + docs: { + summary: + "Flags newly added catch/except blocks that silently discard errors — empty bodies, unused bindings, or a lone return null.", + looksAt: "Added lines in changed JS/TS/Python source files (non-test).", + reports: "File, line, and kind: empty-catch, unused-binding, or return-null.", + network: "Pure local analyzer. No external network call.", + notes: + "Python `except: pass` is allowed. Catch blocks that log, rethrow, or reference the caught binding are not flagged.", + }, + render: (findings, helpers) => { + if (!findings.length) return []; + const lines = ["### Error swallowing (silent catch/except added by this PR)"]; + for (const item of findings) { + lines.push( + `- ${helpers.safeCodeSpan(`${item.file}:${item.line}`)} — ${helpers.safeCodeSpan(item.kind)}`, + ); + } + return lines; + }, + run: (req, { signal }) => scanErrorSwallow(req, signal), + }), descriptor({ name: "commitLint", title: "Conventional-commit subjects", diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index 08486ae5e5..f79dbb29fc 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -483,6 +483,7 @@ export function renderBrief( lines.push(...renderDescriptorSection("conflictMarker", findings.conflictMarker)); lines.push(...renderDescriptorSection("debugLeftover", findings.debugLeftover)); lines.push(...renderDescriptorSection("sizeSmell", findings.sizeSmell)); + lines.push(...renderDescriptorSection("errorSwallow", findings.errorSwallow)); lines.push(...renderDescriptorSection("hardcodedUrl", findings.hardcodedUrl)); lines.push(...renderDescriptorSection("commitLint", findings.commitLint)); diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 6a7af486f2..01cdcf9c09 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -494,6 +494,13 @@ export interface SizeSmellFinding { name?: string; } +/** A catch/except block a PR added that swallows an error without logging, rethrowing, or using the binding (#2014). */ +export interface ErrorSwallowFinding { + file: string; + line: number; + kind: "empty-catch" | "unused-binding" | "return-null"; +} + /** An absolute HTTP(S) URL or raw IP:port endpoint hardcoded in non-test, non-config source (#2027, part of #1499). * Reports location, kind, and a redacted/truncated host — never full paths or query strings. */ export interface HardcodedUrlFinding { @@ -551,6 +558,7 @@ export interface BriefFindings { conflictMarker?: ConflictMarkerFinding[]; debugLeftover?: DebugLeftoverFinding[]; sizeSmell?: SizeSmellFinding[]; + errorSwallow?: ErrorSwallowFinding[]; hardcodedUrl?: HardcodedUrlFinding[]; commitLint?: CommitLintFinding[]; } diff --git a/review-enrichment/test/analyzer-registry.test.ts b/review-enrichment/test/analyzer-registry.test.ts index 3e4d06849f..22cac79b47 100644 --- a/review-enrichment/test/analyzer-registry.test.ts +++ b/review-enrichment/test/analyzer-registry.test.ts @@ -49,6 +49,7 @@ const EXPECTED_ANALYZERS = [ "conflictMarker", "debugLeftover", "sizeSmell", + "errorSwallow", "commitLint", ]; diff --git a/review-enrichment/test/error-swallow.test.ts b/review-enrichment/test/error-swallow.test.ts new file mode 100644 index 0000000000..7d2e8fe293 --- /dev/null +++ b/review-enrichment/test/error-swallow.test.ts @@ -0,0 +1,145 @@ +// 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 { + detectJsCatchSwallow, + detectPythonExceptSwallow, + 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("detectJsCatchSwallow: flags empty and unused-binding catches", () => { + assert.equal(detectJsCatchSwallow("try {} catch (e) {}"), "empty-catch"); + assert.equal(detectJsCatchSwallow("} catch {}"), "empty-catch"); + assert.equal(detectJsCatchSwallow("catch (err) { return null; }"), "return-null"); + assert.equal(detectJsCatchSwallow("catch (err) { doWork(); }"), "unused-binding"); +}); + +test("detectJsCatchSwallow: does not flag catches that log, rethrow, or use the binding", () => { + assert.equal(detectJsCatchSwallow("catch (e) { console.error(e); }"), null); + assert.equal(detectJsCatchSwallow("catch (e) { logger.warn(e); }"), null); + assert.equal(detectJsCatchSwallow("catch (e) { throw e; }"), null); + assert.equal(detectJsCatchSwallow("catch (e) { return handle(e); }"), null); + assert.equal(detectJsCatchSwallow("catch { cleanup(); }"), null); +}); + +test("detectJsCatchSwallow: flags multiline catch blocks", () => { + assert.equal( + detectJsCatchSwallow("catch (err) {\n}"), + "empty-catch", + ); + assert.equal( + detectJsCatchSwallow("catch (err) {\n return null;\n}"), + "return-null", + ); + assert.equal( + detectJsCatchSwallow("catch (err) {\n console.error(err);\n}"), + null, + ); + assert.equal( + detectJsCatchSwallow("catch (err) {\n doWork();\n}"), + "unused-binding", + ); +}); + +test("detectPythonExceptSwallow: allows except pass and flags empty/unused bodies", () => { + assert.equal(detectPythonExceptSwallow("except Exception: pass"), null); + assert.equal(detectPythonExceptSwallow("except Exception as e: pass"), null); + assert.equal(detectPythonExceptSwallow("except Exception:"), "empty-catch"); + assert.equal(detectPythonExceptSwallow("except Exception as e:", "return None"), "return-null"); + assert.equal(detectPythonExceptSwallow("except Exception as e:", "cleanup()"), "unused-binding"); + assert.equal(detectPythonExceptSwallow("except Exception:", "pass"), null); +}); + +test("scanPatchForErrorSwallow: flags added lines with correct locations and respects caps", () => { + const findings = scanPatchForErrorSwallow( + "src/widget.ts", + patchOf([ + "try { doWork(); } catch (e) {}", + "try { other(); } catch (err) { return null; }", + ]), + ); + assert.deepEqual(findings, [ + { file: "src/widget.ts", line: 1, kind: "empty-catch" }, + { file: "src/widget.ts", line: 2, kind: "return-null" }, + ]); + const many = Array.from({ length: 30 }, () => "catch (e) {}"); + assert.equal(scanPatchForErrorSwallow("src/a.ts", patchOf(many), { maxFindings: 3 }).length, 3); +}); + +test("scanPatchForErrorSwallow: reports correct line with preceding context", () => { + const patch = [ + "@@ -10,3 +10,4 @@", + " unchanged context", + "+ catch (err) {", + "+ }", + ].join("\n"); + const findings = scanPatchForErrorSwallow("src/widget.ts", patch); + assert.deepEqual(findings, [{ file: "src/widget.ts", line: 11, kind: "empty-catch" }]); +}); + +test("scanPatchForErrorSwallow: flags multiline JS catch blocks in patches", () => { + const findings = scanPatchForErrorSwallow( + "src/widget.ts", + patchOf(["try {", " catch (err) {", " }", "}"]), + ); + assert.deepEqual(findings, [{ file: "src/widget.ts", line: 2, kind: "empty-catch" }]); + + const logged = scanPatchForErrorSwallow( + "src/widget.ts", + patchOf(["catch (err) {", " console.error(err);", "}"]), + ); + assert.deepEqual(logged, []); + + const unused = scanPatchForErrorSwallow( + "src/widget.ts", + patchOf(["catch (err) {", " doWork();", "}"]), + ); + assert.deepEqual(unused, [{ file: "src/widget.ts", line: 1, kind: "unused-binding" }]); +}); + +test("scanPatchForErrorSwallow: uses only the immediate next added Python body line", () => { + const patch = [ + "@@ -1,0 +1,3 @@", + "+except Exception as e:", + "+ return None", + "+except OtherError: pass", + ].join("\n"); + const findings = scanPatchForErrorSwallow("lib/b.py", patch); + assert.deepEqual(findings, [{ file: "lib/b.py", line: 1, kind: "return-null" }]); +}); + +test("scanPatchForErrorSwallow: skips test files and clean input", () => { + assert.deepEqual( + scanPatchForErrorSwallow("src/widget.test.ts", patchOf(["catch (e) {}"])), + [], + ); + assert.deepEqual( + scanPatchForErrorSwallow("src/widget.ts", patchOf(["catch (e) { console.error(e); }"])), + [], + ); + assert.deepEqual( + scanPatchForErrorSwallow("src/widget.ts", patchOf(["catch { cleanup(); }"])), + [], + ); +}); + +test("scanErrorSwallow: aggregates across files and renders a value-safe brief", async () => { + const findings = await scanErrorSwallow({ + files: [ + { path: "src/a.ts", patch: patchOf(["catch (e) {}"]) }, + { path: "lib/b.py", patch: patchOf(["except Exception:"]) }, + ], + }); + assert.equal(findings.length, 2); + const { promptSection } = renderBrief({ + errorSwallow: findings, + }); + assert.match(promptSection, /Error swallowing/); + assert.match(promptSection, /src\/a\.ts:1/); + assert.doesNotMatch(promptSection, /catch \(e\)/); +}); diff --git a/src/review/enrichment-analyzer-names.ts b/src/review/enrichment-analyzer-names.ts index 00abaa4af1..34cec99327 100644 --- a/src/review/enrichment-analyzer-names.ts +++ b/src/review/enrichment-analyzer-names.ts @@ -43,6 +43,7 @@ export const REES_ANALYZER_NAMES = [ "conflictMarker", "debugLeftover", "sizeSmell", + "errorSwallow", "commitLint", ] as const;