diff --git a/.env.example b/.env.example index c26935a04a..105e47ed4f 100644 --- a/.env.example +++ b/.env.example @@ -68,26 +68,26 @@ 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,commitLint +# conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting,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,floatingPromise,deepNesting +# debugLeftover,sizeSmell,floatingPromise,deepNesting,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,floatingPromise,deepNesting -# commitLint +# 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,floatingPromise,deepNesting,commitLint +# conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting,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 4f38016f1e..52c5254ed9 100644 --- a/apps/gittensory-ui/src/lib/rees-analyzers.ts +++ b/apps/gittensory-ui/src/lib/rees-analyzers.ts @@ -1027,6 +1027,28 @@ export const REES_ANALYZERS = [ "Counts braces opened by if/for/while/switch/try/catch/else/do/with, arrow bodies, and function bodies — not object-literal braces. Resets across context lines.", }, }, + { + 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 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.", + 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).", + }, + }, { name: "commitLint", title: "Conventional-commit subjects", diff --git a/review-enrichment/analyzer-metadata.json b/review-enrichment/analyzer-metadata.json index d06295b18e..01c94f6199 100644 --- a/review-enrichment/analyzer-metadata.json +++ b/review-enrichment/analyzer-metadata.json @@ -1161,6 +1161,32 @@ "notes": "Counts braces opened by if/for/while/switch/try/catch/else/do/with, arrow bodies, and function bodies — not object-literal braces. Resets across context lines." } }, + { + "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 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.", + "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)." + } + }, { "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..4917665a52 --- /dev/null +++ b/review-enrichment/src/analyzers/error-swallow.ts @@ -0,0 +1,214 @@ +// 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; +} diff --git a/review-enrichment/src/analyzers/registry.ts b/review-enrichment/src/analyzers/registry.ts index a7206c8069..2f8e3d9582 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 { scanDeepNesting } from "./deep-nesting.js"; +import { scanErrorSwallow } from "./error-swallow.js"; import { scanFloatingPromise } from "./floating-promise.js"; import { scanSizeSmell } from "./size-smell.js"; import { scanCommitLint } from "./commit-lint.js"; @@ -1106,6 +1107,35 @@ export const ANALYZER_DESCRIPTORS = [ }, run: (req, { signal }) => scanDeepNesting(req, signal), }), + 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 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.", + 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).", + }, + 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: "commitLint", title: "Conventional-commit subjects", diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index 571acdca44..69e0f4635c 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -485,6 +485,7 @@ export function renderBrief( lines.push(...renderDescriptorSection("sizeSmell", findings.sizeSmell)); lines.push(...renderDescriptorSection("floatingPromise", findings.floatingPromise)); lines.push(...renderDescriptorSection("deepNesting", findings.deepNesting)); + 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 4a75803e29..0bbc6ef048 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -494,6 +494,14 @@ export interface SizeSmellFinding { name?: string; } +/** 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. */ +export interface ErrorSwallowFinding { + file: string; + line: number; + kind: "empty-catch" | "unused-binding" | "return-null"; +} + /** Deep control-flow nesting newly added in the diff (#2030, part of #1499). * Reports file, line, measured depth, and threshold — never source content. */ export interface DeepNestingFinding { @@ -570,6 +578,7 @@ export interface BriefFindings { sizeSmell?: SizeSmellFinding[]; floatingPromise?: FloatingPromiseFinding[]; deepNesting?: DeepNestingFinding[]; + 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 9e6e7ca626..63a1d05bf0 100644 --- a/review-enrichment/test/analyzer-registry.test.ts +++ b/review-enrichment/test/analyzer-registry.test.ts @@ -51,6 +51,7 @@ const EXPECTED_ANALYZERS = [ "sizeSmell", "floatingPromise", "deepNesting", + "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..8bd3645184 --- /dev/null +++ b/review-enrichment/test/error-swallow.test.ts @@ -0,0 +1,93 @@ +// 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\)/); +}); diff --git a/src/review/enrichment-analyzer-names.ts b/src/review/enrichment-analyzer-names.ts index 36e44da514..23b069d564 100644 --- a/src/review/enrichment-analyzer-names.ts +++ b/src/review/enrichment-analyzer-names.ts @@ -45,6 +45,7 @@ export const REES_ANALYZER_NAMES = [ "sizeSmell", "floatingPromise", "deepNesting", + "errorSwallow", "commitLint", ] as const;