From 2fb5fb6052a08ea790095f06582c3429a0803418 Mon Sep 17 00:00:00 2001 From: bohdansolovie Date: Sun, 5 Jul 2026 11:04:21 +0200 Subject: [PATCH 1/2] feat(enrichment): add empty-catch / error-swallow analyzer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #2014 — flags swallowed errors in added JS/TS/Python catch/except blocks. Co-authored-by: Cursor --- .env.example | 25 +-- apps/gittensory-ui/src/lib/rees-analyzers.ts | 22 +++ review-enrichment/analyzer-metadata.json | 26 +++ .../src/analyzers/error-swallow.ts | 175 ++++++++++++++++++ review-enrichment/src/analyzers/registry.ts | 30 +++ review-enrichment/src/render.ts | 1 + review-enrichment/src/types.ts | 9 + .../test/analyzer-registry.test.ts | 1 + review-enrichment/test/error-swallow.test.ts | 87 +++++++++ 9 files changed, 364 insertions(+), 12 deletions(-) create mode 100644 review-enrichment/src/analyzers/error-swallow.ts create mode 100644 review-enrichment/test/error-swallow.test.ts diff --git a/.env.example b/.env.example index 460a4492c7..8c368892d8 100644 --- a/.env.example +++ b/.env.example @@ -65,25 +65,26 @@ GITTENSORY_REVIEW_ENRICHMENT=false # Current analyzer names: # dependency,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 -# commitLint +# nativeBuild,history,docCommentDrift,duplication,errorSwallow,churnHotspot,blameLink +# approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch,commitHygiene +# pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker,magicNumber +# conflictMarker,debugLeftover,commitLint # # Profile defaults: # fast: dependency,lockfileDrift,secret,license,installScript,heavyDependency,hardcodedUrl -# actionPin,eol,redos,provenance,secretLog,typosquat,iacMisconfig,nativeBuild,testRatio -# migrationSafety,looseRange,terminology,todoMarker,magicNumber,conflictMarker,debugLeftover +# actionPin,eol,redos,provenance,secretLog,typosquat,iacMisconfig,nativeBuild,errorSwallow +# testRatio,migrationSafety,looseRange,terminology,todoMarker,magicNumber,conflictMarker +# debugLeftover # balanced (default): dependency,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,commitLint +# commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift,duplication,errorSwallow +# churnHotspot,blameLink,approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch +# commitHygiene,pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology +# todoMarker,magicNumber,conflictMarker,debugLeftover,commitLint # deep: dependency,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 +# iacMisconfig,nativeBuild,history,docCommentDrift,duplication,errorSwallow,churnHotspot +# blameLink,approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch,commitHygiene # pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker,magicNumber # conflictMarker,debugLeftover,commitLint # END GENERATED REES ANALYZERS diff --git a/apps/gittensory-ui/src/lib/rees-analyzers.ts b/apps/gittensory-ui/src/lib/rees-analyzers.ts index 30e72ae3f9..5f78a07af4 100644 --- a/apps/gittensory-ui/src/lib/rees-analyzers.ts +++ b/apps/gittensory-ui/src/lib/rees-analyzers.ts @@ -559,6 +559,28 @@ export const REES_ANALYZERS = [ "Conservative: trivial/boilerplate lines are dropped and a long contiguous run is required, so incidental overlap is not flagged. Never returns code content.", }, }, + { + name: "errorSwallow", + title: "Swallowed errors", + 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 swallow errors without logging or rethrowing.", + looksAt: "Added lines in changed JS/TS/Python source files, excluding tests.", + reports: "File, line, and kind: empty-catch, unused-binding, or return-null.", + network: "Pure local analyzer. No external network call.", + notes: + "Precision-first: catches that log, rethrow, or reference the binding are not flagged. Python `except: pass` is included.", + }, + }, { name: "churnHotspot", title: "Churn hotspots", diff --git a/review-enrichment/analyzer-metadata.json b/review-enrichment/analyzer-metadata.json index 5bd5db75a5..161206ba04 100644 --- a/review-enrichment/analyzer-metadata.json +++ b/review-enrichment/analyzer-metadata.json @@ -637,6 +637,32 @@ "notes": "Conservative: trivial/boilerplate lines are dropped and a long contiguous run is required, so incidental overlap is not flagged. Never returns code content." } }, + { + "name": "errorSwallow", + "title": "Swallowed errors", + "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 swallow errors without logging or rethrowing.", + "looksAt": "Added lines in changed JS/TS/Python source files, excluding tests.", + "reports": "File, line, and kind: empty-catch, unused-binding, or return-null.", + "network": "Pure local analyzer. No external network call.", + "notes": "Precision-first: catches that log, rethrow, or reference the binding are not flagged. Python `except: pass` is included." + } + }, { "name": "churnHotspot", "title": "Churn hotspots", diff --git a/review-enrichment/src/analyzers/error-swallow.ts b/review-enrichment/src/analyzers/error-swallow.ts new file mode 100644 index 0000000000..e10eed332b --- /dev/null +++ b/review-enrichment/src/analyzers/error-swallow.ts @@ -0,0 +1,175 @@ +// 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 SINGLE_CATCH_RE = /catch\s*\(\s*(\w+)?\s*\)\s*\{([\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 bodySwallowsError(body: string, binding: string | null): ErrorSwallowFinding["kind"] | null { + const trimmed = body.trim(); + if (!trimmed) return "empty-catch"; + if (/^return\s+null\s*;?$/.test(trimmed)) return "return-null"; + if (!binding) return null; + const bindingRe = new RegExp(`\\b${escapeRegExp(binding)}\\b`); + if (/\bthrow\b/.test(trimmed)) return null; + if (/\b(?:console|logger|log|winston|pino|bunyan)\s*[.(]/.test(trimmed)) return null; + if (/\bprint\s*\(/.test(trimmed)) return null; + if (!bindingRe.test(trimmed)) return "unused-binding"; + return null; +} + +/** Classify one source line for an error-swallow pattern, or null. Pure. */ +export function detectErrorSwallow(line: string): ErrorSwallowFinding["kind"] | null { + const pyMatch = PY_EXCEPT_PASS_RE.exec(line); + if (pyMatch) { + return pyMatch[1] ? "unused-binding" : "empty-catch"; + } + + const single = SINGLE_CATCH_RE.exec(line); + if (single) { + return bodySwallowsError(single[2] ?? "", single[1] ?? null); + } + + return null; +} + +type PendingCatch = { + startLine: number; + binding: string | null; + body: string; + depth: number; +}; + +function updatePending(pending: PendingCatch, line: string): PendingCatch { + let depth = pending.depth; + for (const ch of line) { + if (ch === "{") depth++; + else if (ch === "}") depth--; + } + return { ...pending, body: `${pending.body}\n${line}`, depth }; +} + +function flushPending(pending: PendingCatch): ErrorSwallowFinding["kind"] | null { + const body = pending.body.replace(/^\s*\{/, "").replace(/\}\s*$/, ""); + return bodySwallowsError(body, pending.binding); +} + +type ScanLimits = { + maxFindings?: number; + signal?: AbortSignal; +}; + +/** Scan one file patch's added lines for swallowed errors, 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 || !isScannablePath(path)) return []; + const findings: ErrorSwallowFinding[] = []; + let newLine = 0; + let inHunk = false; + let pending: PendingCatch | null = null; + + const pushFinding = (line: number, kind: ErrorSwallowFinding["kind"]) => { + 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); + 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 && !body.slice(braceIndex + 1).includes("}")) { + let depth = 0; + for (const ch of body.slice(braceIndex)) { + if (ch === "{") depth++; + else if (ch === "}") depth--; + } + 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 f5fcc1ea39..3a65af002e 100644 --- a/review-enrichment/src/analyzers/registry.ts +++ b/review-enrichment/src/analyzers/registry.ts @@ -10,6 +10,7 @@ import { scanCommitSignature } from "./commit-signature.js"; import { dependencyAnalyzer } from "./dependency/descriptor.js"; import { scanDocCommentDrift } from "./doc-comment-drift.js"; import { scanDuplication } from "./duplication-scan.js"; +import { scanErrorSwallow } from "./error-swallow.js"; import { scanEol } from "./eol-check.js"; import { scanHardcodedUrl } from "./hardcoded-url.js"; import { scanHeavyDependencies } from "./heavy-dependency.js"; @@ -466,6 +467,35 @@ export const ANALYZER_DESCRIPTORS = [ run: (req, { signal, analysis, diagnostics }) => scanDuplication(req, fetch, { signal, analysis, diagnostics }), }), + descriptor({ + name: "errorSwallow", + title: "Swallowed errors", + category: "quality", + cost: "local", + defaultEnabled: true, + requires: ["files"], + limits: { maxFindings: 25, maxLineChars: 2000 }, + docs: { + summary: + "Flags newly-added catch/except blocks that swallow errors without logging or rethrowing.", + looksAt: "Added lines in changed JS/TS/Python source files, excluding tests.", + reports: "File, line, and kind: empty-catch, unused-binding, or return-null.", + network: "Pure local analyzer. No external network call.", + notes: + "Precision-first: catches that log, rethrow, or reference the binding are not flagged. Python `except: pass` is included.", + }, + render: (findings, helpers) => { + if (!findings.length) return []; + const lines = ["### Swallowed errors (empty catch / unused binding / return null)"]; + 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: "churnHotspot", title: "Churn hotspots", diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index b865adefd9..c1c6fc6714 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -481,6 +481,7 @@ export function renderBrief( lines.push(...renderDescriptorSection("magicNumber", findings.magicNumber)); lines.push(...renderDescriptorSection("conflictMarker", findings.conflictMarker)); lines.push(...renderDescriptorSection("debugLeftover", findings.debugLeftover)); + 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 3080f0a32f..37583d145f 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -482,6 +482,14 @@ export interface HardcodedUrlFinding { host: string; } +/** A newly-added catch/except block that swallows an error without logging or rethrowing (#2014, part of #1499). + * Reports location and kind only — never catch bodies or stack traces. */ +export interface ErrorSwallowFinding { + file: string; + line: number; + kind: "empty-catch" | "unused-binding" | "return-null"; +} + /** A PR commit subject that does not conform to the Conventional Commits spec (#2021, part of #1499). Reports a * short SHA prefix, the subject, and the failing reason — never author/email. */ export interface CommitLintFinding { @@ -529,6 +537,7 @@ export interface BriefFindings { conflictMarker?: ConflictMarkerFinding[]; debugLeftover?: DebugLeftoverFinding[]; hardcodedUrl?: HardcodedUrlFinding[]; + errorSwallow?: ErrorSwallowFinding[]; commitLint?: CommitLintFinding[]; } diff --git a/review-enrichment/test/analyzer-registry.test.ts b/review-enrichment/test/analyzer-registry.test.ts index 509f2c8758..f739eb5fb3 100644 --- a/review-enrichment/test/analyzer-registry.test.ts +++ b/review-enrichment/test/analyzer-registry.test.ts @@ -31,6 +31,7 @@ const EXPECTED_ANALYZERS = [ "history", "docCommentDrift", "duplication", + "errorSwallow", "churnHotspot", "blameLink", "approvalIntegrity", diff --git a/review-enrichment/test/error-swallow.test.ts b/review-enrichment/test/error-swallow.test.ts new file mode 100644 index 0000000000..4d72d868c7 --- /dev/null +++ b/review-enrichment/test/error-swallow.test.ts @@ -0,0 +1,87 @@ +// 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 (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); +}); + +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\)/); +}); From d1217a9cff38e3fa3a36b70afcbfa6bb2d391d75 Mon Sep 17 00:00:00 2001 From: bohdansolovie Date: Sun, 5 Jul 2026 11:11:02 +0200 Subject: [PATCH 2/2] fix(review): sync REES analyzer name registry with enrichment registry Add hardcodedUrl, errorSwallow, and debugLeftover so env/manifest validation matches the REES registry. Co-authored-by: Cursor --- src/review/enrichment-analyzer-names.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/review/enrichment-analyzer-names.ts b/src/review/enrichment-analyzer-names.ts index f1f7d44de3..52549c5af8 100644 --- a/src/review/enrichment-analyzer-names.ts +++ b/src/review/enrichment-analyzer-names.ts @@ -10,6 +10,7 @@ export const REES_ANALYZER_NAMES = [ "license", "installScript", "heavyDependency", + "hardcodedUrl", "actionPin", "eol", "redos", @@ -24,6 +25,7 @@ export const REES_ANALYZER_NAMES = [ "history", "docCommentDrift", "duplication", + "errorSwallow", "churnHotspot", "blameLink", "approvalIntegrity", @@ -39,6 +41,7 @@ export const REES_ANALYZER_NAMES = [ "todoMarker", "magicNumber", "conflictMarker", + "debugLeftover", "commitLint", ] as const;