[eslint-miner] eslint-factory: add no-empty-catch-block rule - #52681
[eslint-miner] eslint-factory: add no-empty-catch-block rule#52681github-actions[bot] wants to merge 3 commits into
Conversation
Adds a new custom ESLint rule that flags empty catch blocks
(`catch {}` / `catch (err) {}`) in actions/setup/js scripts.
An empty catch silently swallows the original error with no
logging, fallback assignment, or explanatory comment, making
failures (corrupted state files, cleanup errors) hard to diagnose
from CI logs.
The rule allows catch blocks that contain a comment documenting
the intentional no-op, keeping false positives at zero.
Scanning actions/setup/js confirmed 3 real occurrences with this
exact pattern (fuzz_template_substitution_harness.cjs,
load_experiment_state_from_repo.cjs, pick_experiment.cjs), and
lint:setup-js now reports exactly those 3 with no other matches.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Reviewed PR #52681 (eslint-factory: new no-empty-catch-block rule) against the ponytail-review skill. The rule is ~38 lines with a single structural check (empty catch body + no comment), no speculative options, no unused abstractions, and the test file is proportionate. Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions detected).
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 4.16 AIC · ⌖ 7.23 AIC · ⊞ 4K
Comment /review to run again
| // A comment inside the (otherwise empty) braces documents intent, e.g.: | ||
| // } catch { /* best-effort cleanup, ignore */ } | ||
| const commentsInside = sourceCode.getCommentsInside(node.body); | ||
| if (commentsInside.length > 0) return; |
There was a problem hiding this comment.
The rule is too permissive: any comment inside an otherwise empty catch suppresses the warning, so catch { /* TODO */ } or a stale copy-pasted comment now bypasses the check while still swallowing the error with no logging or fallback. That undercuts the rule’s stated safety goal and will miss exactly the failures this PR is trying to surface.
💡 Tighten the exception instead of treating all comments as intent
Right now the implementation only checks getCommentsInside(node.body).length > 0, which means the presence of any comment disables the rule. That’s a weak proxy for documented intent and it creates an easy false-negative path.
At minimum, constrain the allowlist to comments matching an explicit intentional-ignore pattern, or better yet require a real handling action (log / fallback / rethrow) and keep comments informational only. For example:
const commentsInside = sourceCode.getCommentsInside(node.body);
const hasIntentionalIgnoreComment = commentsInside.some(comment =>
/intentional|best-effort|ignore/i.test(comment.value),
);
if (hasIntentionalIgnoreComment) return;Without that, this rule will look useful in CI while silently allowing empty catches to persist behind low-signal comments.
There was a problem hiding this comment.
Pull request overview
Adds a custom ESLint rule that detects undocumented empty catch blocks in setup JavaScript scripts.
Changes:
- Implements and tests
no-empty-catch-block. - Registers and enables the rule at warning severity.
- Allows empty catch blocks containing explanatory comments.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/no-empty-catch-block.ts |
Implements detection and reporting. |
eslint-factory/src/rules/no-empty-catch-block.test.ts |
Tests valid and invalid catch blocks. |
eslint-factory/src/index.ts |
Registers the rule. |
eslint-factory/eslint.config.cjs |
Enables the rule as a warning. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
| @@ -0,0 +1,38 @@ | |||
| import { ESLintUtils, TSESTree } from "@typescript-eslint/utils"; | |||
|
|
|||
| const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`); | |||
There was a problem hiding this comment.
The no-empty-catch-block rule implementation is correct and well-structured. CatchClause visitor correctly targets empty body arrays, getCommentsInside properly allows documented-intent escapes, tests cover all valid and invalid patterns, and the rule is properly registered. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 18.4 AIC · ⌖ 7.85 AIC · ⊞ 5.4K
Test Quality Sentinel 🧪SummaryScore: 100/100 ✅ Excellent This PR introduces a new ESLint rule ( Test Coverage
Test ClassificationTest Analysis (3 tests)
Quality Signals
ResultImplementation Test Ratio: 0% (all design tests) — threshold: ≤30%
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — approving with one minor suggestion on rule metadata scope.
📋 Summary
Positive Highlights
- ✅ Clean, minimal rule implementation — purely structural check (empty block + no comment), low false-positive risk
- ✅ Good test coverage: valid cases cover all intended escape hatches (log, fallback assign, comment, rethrow), invalid cases cover parameter-less and named catch
- ✅ Comment-based escape hatch is the right design — avoids suppression comments and stays readable
- ✅ Rule registered and enabled as
warn(correct severity for a new rule before fixing existing occurrences) - ✅ PR description clearly explains the 3 real-world occurrences that motivated the rule
Minor Issue
- The
docs.descriptionmentions in
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 22.7 AIC · ⌖ 8.69 AIC · ⊞ 7K
Comment /matt to run again
| name: "no-empty-catch-block", | ||
| meta: { | ||
| type: "problem", | ||
| docs: { |
There was a problem hiding this comment.
[/codebase-design] The docs.description says in
|
@copilot Unresolved review threads (newest first):
Please refresh the branch if needed, address the unresolved review feedback, rerun validation, and use the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in d4d7096: documented the rule in the README and tightened the exception to explicit, case-insensitive |
Summary
Daily ESLint Miner run for github/gh-aw. Adds one net-new custom rule:
no-empty-catch-block.Evidence
Scanned actions/setup/js/**/*.cjs for empty
catch {}/catch (err) {}blocks that swallow errors with no logging, fallback value, or explanatory comment. Found 3 real occurrences:These hide real failures (corrupted state files, cleanup errors) that are otherwise invisible in CI logs.
Rule design
catch { /* best-effort cleanup, ignore */ }), avoiding false positives on already-documented intentional swallows.Validation
cd eslint-factory && npm install— OKcd eslint-factory && npm run build— OKcd eslint-factory && npm test— new rule's 3 tests pass; 1 pre-existing unrelated failure in require-fs-io-try-catch.test.ts (not touched by this change)cd eslint-factory && npm run lint:setup-js— new rule fires exactly 3 times, matching the 3 occurrences found during manual scanning; no other files triggered it (no false positives observed)Scope
No changes outside eslint-factory; no fixes applied to actions/setup/js source files themselves (out of scope for rule-authoring PRs — follow-up fix PRs can address the 3 flagged sites).
Run: https://github.com/github/gh-aw/actions/runs/31795158705> Generated by 👨🍳 PR Sous Chef · gpt54 · 9.47 AIC · ⌖ 7.2 AIC · ⊞ 8.5K · ◷