Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -67,23 +67,24 @@ GITTENSORY_REVIEW_ENRICHMENT=false
# 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,commitLint
# looseRange,terminology,todoMarker,magicNumber,conflictMarker,commitLint,errorSwallow
#
# Profile defaults:
# fast: dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol
# redos,provenance,secretLog,typosquat,iacMisconfig,nativeBuild,testRatio,migrationSafety
# looseRange,terminology,todoMarker,magicNumber,conflictMarker
# looseRange,terminology,todoMarker,magicNumber,conflictMarker,errorSwallow
# balanced (default): dependency,lockfileDrift,secret,license,installScript,heavyDependency
# 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,commitLint
# conflictMarker,commitLint,errorSwallow
# deep: dependency,lockfileDrift,secret,license,installScript,heavyDependency,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,commitLint
# errorSwallow
# END GENERATED REES ANALYZERS

# Submitter-reputation spend control (internal-only): downgrades new/burst/low-rep
Expand Down
23 changes: 23 additions & 0 deletions apps/gittensory-ui/src/lib/rees-analyzers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -909,6 +909,29 @@ export const REES_ANALYZERS = [
"Structured-fields-only: reads commit.message subjects, linted independently, never cross-line state. Fail-safe on missing token/fetch error.",
},
},
{
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 blocks that swallow the error: an empty body, a body that just returns null/undefined, or one that neither rethrows, logs, nor references the caught binding.",
looksAt:
"Added lines in JS/TS source (a single-line `catch` block) and Python (`except …: pass`).",
reports: "File, line, and the swallow kind — never line content.",
network: "Pure local analyzer. No external network call.",
notes:
"Single-line by design: a catch body spread across lines is not tracked (the safe, false-negative direction). String literals and comments are blanked first.",
},
},
] as const satisfies readonly ReesAnalyzerDoc[];

export const REES_ANALYZER_NAMES = REES_ANALYZERS.map((analyzer) => analyzer.name);
26 changes: 26 additions & 0 deletions review-enrichment/analyzer-metadata.json
Original file line number Diff line number Diff line change
Expand Up @@ -1023,6 +1023,32 @@
"network": "Calls the GitHub PR-commits API once, bounded to one page.",
"notes": "Structured-fields-only: reads commit.message subjects, linted independently, never cross-line state. Fail-safe on missing token/fetch error."
}
},
{
"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 blocks that swallow the error: an empty body, a body that just returns null/undefined, or one that neither rethrows, logs, nor references the caught binding.",
"looksAt": "Added lines in JS/TS source (a single-line `catch` block) and Python (`except …: pass`).",
"reports": "File, line, and the swallow kind — never line content.",
"network": "Pure local analyzer. No external network call.",
"notes": "Single-line by design: a catch body spread across lines is not tracked (the safe, false-negative direction). String literals and comments are blanked first."
}
}
]
}
140 changes: 140 additions & 0 deletions review-enrichment/src/analyzers/error-swallow.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
// Error-swallow analyzer (#2014). Flags newly-added catch blocks that swallow the error — an empty body, a body
// that just returns null/undefined, or a body that neither rethrows, logs, nor references the caught binding — a
// top source of silent failures. Pure compute over added diff lines, no network. Scoped to JS/TS (a `catch`
// block) and Python (a bare `except … : pass`). Detection is SINGLE-LINE by design (the catch/except and its
// body on one added line, the compact form the pattern targets): a body spread across multiple lines is not
// tracked — missing it is the safe (false-negative) direction, and there is no cross-line state. String literals
// and comments are blanked first (a `catch {}` in a string, and a comment-only body which is itself a swallow).
// Follows the actions-pin.ts added-line hunk-walk pattern. Line-cited via hunk headers.
import type { EnrichRequest, ErrorSwallowFinding } from "../types.js";
import { codeOnly } from "./secret-log.js";

const MAX_FINDINGS = 25;
const MAX_LINE_CHARS = 2000;

const JS_EXTS = new Set(["ts", "tsx", "mts", "cts", "js", "jsx", "mjs", "cjs"]);
const PY_EXTS = new Set(["py"]);

// A JS/TS `catch` with an OPTIONAL binding and a single-line body captured up to the first `}`.
// group 1 = the binding name (when `catch (e)`); group 2 = the body between the braces.
const JS_CATCH_RE = /\bcatch\s*(?:\(\s*([A-Za-z_$][\w$]*)[^)]*\))?\s*\{([^}]*)\}/;
// A Python bare `except …: pass` — the canonical error-swallow.
const PY_EXCEPT_PASS_RE = /^\s*except\b[^:]*:\s*pass\s*$/;

// Body signals that mean the error is HANDLED, not swallowed: a rethrow, or a logging call.
const RETHROW_RE = /\bthrow\b/;
const LOG_RE = /\b(?:console|logger|log)\s*\.\s*\w+\s*\(|\blog\s*\(|\bwarn\s*\(|\berror\s*\(/i;
// A body that is EXACTLY `return null`/`return undefined` — a swallow via a null return. Anchored to the whole
// (already-trimmed) body so a `return null` that is only ONE branch of a body that also rethrows
// (`if (x) return null; throw e;`) is NOT mistaken for a pure null-return swallow.
const RETURN_NULL_RE = /^return\s+(?:null|undefined)\s*;?$/;

function extOf(path: string): string | null {
const base = path.split("/").pop() ?? path;
const dot = base.lastIndexOf(".");
return dot > 0 ? base.slice(dot + 1).toLowerCase() : null;
}

/** Classify a single added source line for an error-swallow, given its file's language. Returns the kind, or
* null. Strings/comments are blanked first so a `catch {}` in a string is not matched. Pure. */
export function detectErrorSwallow(
line: string,
lang: "js" | "py",
): ErrorSwallowFinding["kind"] | null {
if (lang === "py") {
return PY_EXCEPT_PASS_RE.test(line) ? "empty-catch" : null;
}
const code = codeOnly(line).replace(/\/\*.*?\*\//g, " ").replace(/\/\/.*$/, "");
const match = JS_CATCH_RE.exec(code);
if (!match) return null;
const binding = match[1];
const body = (match[2] ?? "").trim();
if (!body) return "empty-catch";
if (RETURN_NULL_RE.test(body)) return "return-null";
// A body that neither rethrows, logs, nor references the caught binding swallows the error. Only meaningful
// when there IS a binding to ignore — a bindingless `catch { doStuff() }` is not an "unused binding". The
// binding is a JS identifier that may contain `$`, so it is regex-escaped before use, and referenced-ness is
// tested with identifier-char boundaries (not `\b`, which mishandles a leading `$`/`_`) so `report($err)`
// correctly counts as a reference to `$err`.
if (binding && !RETHROW_RE.test(body) && !LOG_RE.test(body)) {
const escaped = binding.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
const referencesBinding = new RegExp(`(?<![\\w$])${escaped}(?![\\w$])`).test(body);
if (!referencesBinding) return "unused-binding";
}
return null;
}

type ScanLimits = {
maxFindings?: number;
signal?: AbortSignal;
};

/** Scan one file patch's added lines for error-swallow 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) return [];
const ext = extOf(path);
const lang: "js" | "py" | null = ext
? JS_EXTS.has(ext)
? "js"
: PY_EXTS.has(ext)
? "py"
: null
: null;
if (!lang) return [];

const findings: ErrorSwallowFinding[] = [];
let newLine = 0;
let inHunk = false;
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;
continue;
}
// Skip pre-hunk preamble; inside a hunk `+++x`/`+++ x` is added content, not a header.
if (!inHunk) continue;
if (line.startsWith("+")) {
const body = line.slice(1);
if (body.length <= MAX_LINE_CHARS) {
const kind = detectErrorSwallow(body, lang);
if (kind) {
findings.push({ file: path, line: newLine, kind });
if (findings.length >= maxFindings) return findings;
}
}
newLine++;
} else if (!line.startsWith("-") && !line.startsWith("\\")) {
// A `\ No newline at end of file` marker is not a new-file line — do not advance the cursor
// (same class as the actions-pin / iac-misconfig line-number fix).
newLine++;
}
}
return findings;
}

/** Analyzer entrypoint: scan every changed JS/TS/Python file's added lines for error-swallow catch blocks. */
export async function scanErrorSwallow(
req: EnrichRequest,
signal?: AbortSignal,
): Promise<ErrorSwallowFinding[]> {
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;
}
40 changes: 40 additions & 0 deletions review-enrichment/src/analyzers/registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import { scanLooseRanges } from "./loose-range.js";
import { scanMagicNumbers } from "./magic-number.js";
import { scanConflictMarkers } from "./conflict-marker.js";
import { scanCommitLint } from "./commit-lint.js";
import { scanErrorSwallow } from "./error-swallow.js";
import { scanTerminology } from "./terminology.js";
import { scanTodoMarker } from "./todo-marker.js";
import { scanTyposquat } from "./typosquat.js";
Expand Down Expand Up @@ -959,6 +960,45 @@ export const ANALYZER_DESCRIPTORS = [
run: (req, { signal, analysis, diagnostics }) =>
scanCommitLint(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 blocks that swallow the error: an empty body, a body that just returns null/undefined, or one that neither rethrows, logs, nor references the caught binding.",
looksAt: "Added lines in JS/TS source (a single-line `catch` block) and Python (`except …: pass`).",
reports: "File, line, and the swallow kind — never line content.",
network: "Pure local analyzer. No external network call.",
notes:
"Single-line by design: a catch body spread across lines is not tracked (the safe, false-negative direction). String literals and comments are blanked first.",
},
render: (findings, helpers) => {
if (!findings.length) return [];
const explain = (kind: (typeof findings)[number]["kind"]): string => {
switch (kind) {
case "empty-catch":
return "an empty catch body silently discards the error";
case "return-null":
return "the catch returns null/undefined, swallowing the error";
case "unused-binding":
return "the catch never rethrows, logs, or references the caught error";
}
};
const lines = ["### Swallowed errors (silent failure risk)"];
for (const item of findings) {
lines.push(
`- ${helpers.safeCodeSpan(`${item.file}:${item.line}`)} — ${explain(item.kind)}`,
);
}
return lines;
},
run: (req, { signal }) => scanErrorSwallow(req, signal),
}),
] as const satisfies readonly AnyAnalyzerDescriptor[];

export const ANALYZER_NAMES = ANALYZER_DESCRIPTORS.map(
Expand Down
1 change: 1 addition & 0 deletions review-enrichment/src/render.ts
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,7 @@ export function renderBrief(
lines.push(...renderDescriptorSection("magicNumber", findings.magicNumber));
lines.push(...renderDescriptorSection("conflictMarker", findings.conflictMarker));
lines.push(...renderDescriptorSection("commitLint", findings.commitLint));
lines.push(...renderDescriptorSection("errorSwallow", findings.errorSwallow));

if (!lines.length) return { promptSection: "", systemSuffix: "" };

Expand Down
9 changes: 9 additions & 0 deletions review-enrichment/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,14 @@ export interface CommitLintFinding {
reason: "bad-type" | "missing-colon" | "too-long" | "empty";
}

/** A newly-added catch block that swallows the error — an empty body, a null/undefined return, or a body that
* never rethrows, logs, or references the caught binding (#2014, part of #1499). Reports location + kind. */
export interface ErrorSwallowFinding {
file: string;
line: number;
kind: "empty-catch" | "unused-binding" | "return-null";
}

/** Structured analyzer output. Each analyzer fills its own key; more land as analyzers ship (#1477/#1478). */
export interface BriefFindings {
dependency?: DependencyFinding[];
Expand Down Expand Up @@ -501,6 +509,7 @@ export interface BriefFindings {
magicNumber?: MagicNumberFinding[];
conflictMarker?: ConflictMarkerFinding[];
commitLint?: CommitLintFinding[];
errorSwallow?: ErrorSwallowFinding[];
}

/** A JSDoc/TSDoc block whose `@param` tags name parameters the adjacent function no longer declares — a
Expand Down
1 change: 1 addition & 0 deletions review-enrichment/test/analyzer-registry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ const EXPECTED_ANALYZERS = [
"magicNumber",
"conflictMarker",
"commitLint",
"errorSwallow",
];

test("analyzer descriptors cover the runtime registry in stable order", () => {
Expand Down
Loading
Loading