fix(eslint-factory): treat try/finally without catch as unprotected in isInsideTryBlock

[Copilot]

isInsideTryBlock was returning true for any enclosing TryStatement, including try/finally with no catch. A catch-less try/finally only runs cleanup — the throw resumes propagating after finally, so the fs/JSON.parse call is still unhandled.

Changes

• require-fs-sync-try-catch.ts / require-json-parse-try-catch.ts: Gate the protective check on ancestor.handler != null, so only try/catch and try/catch/finally are treated as error-handling boundaries; try/finally alone is not.

// before
if (ancestor.type === "TryStatement" && !crossedDeferredBoundary) {

// after — requires a catch clause
if (ancestor.type === "TryStatement" && !crossedDeferredBoundary && ancestor.handler != null) {

• Tests (both rules):
  • invalid: try { fs.writeFileSync(p, d); } finally { cleanup(); } — now flagged
  • valid: try { fs.writeFileSync(p, d); } catch (e) {} finally { cleanup(); } — still passes

[Copilot]

Pull request overview

This PR fixes a correctness issue in the eslint-factory rules’ isInsideTryBlock logic: a try/finally without a catch is no longer treated as an error-handling boundary, so sync fs.*Sync(...) calls and JSON.parse(...) inside such blocks are correctly flagged as unprotected.

Changes:

• Update isInsideTryBlock in both rules to only treat TryStatement nodes with a non-null handler (i.e., an actual catch) as protective.
• Add/extend tests to confirm try/catch/finally remains protective while try/finally (no catch) is now invalid for both rules.

Show a summary per file

File	Description
eslint-factory/src/rules/require-json-parse-try-catch.ts	Require ancestor.handler != null for a try to be considered protective.
eslint-factory/src/rules/require-json-parse-try-catch.test.ts	Add coverage for try/catch/finally (valid) and try/finally (invalid) cases.
eslint-factory/src/rules/require-fs-sync-try-catch.ts	Require ancestor.handler != null for a try to be considered protective.
eslint-factory/src/rules/require-fs-sync-try-catch.test.ts	Add coverage for try/catch/finally (valid) and try/finally (invalid) cases.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 4/4 changed files
• Comments generated: 0
• Review effort level: Low

[github-actions]

Hey @pelikhan 👋 — great bug to catch! A try/finally-without-catch block should absolutely still be flagged, and fixing the ancestor.handler != null guard in both require-fs-sync-try-catch and require-json-parse-try-catch is the right approach.

A couple of things to address before this is ready for review:

• No code has landed yet — the PR currently has 0 additions and 0 deletions. The checklist in the body reads like a solid plan; it just needs to be executed.
• Tests are missing — the checklist correctly calls out the four test cases to add (invalid try/finally, valid try/catch/finally for both rules). Without them, there's no safety net for this fix.

If you'd like a hand, you can assign this prompt to your coding agent:

Fix the `isInsideTryBlock` function in both ESLint rules so that a `try/finally` block (with no `catch`) is still flagged.

Files to edit:
- eslint-factory/src/rules/require-fs-sync-try-catch.ts
- eslint-factory/src/rules/require-json-parse-try-catch.ts

Change: In the `isInsideTryBlock` ancestor-walking loop, add the condition `ancestor.handler != null` so that a TryStatement only counts as "caught" when an actual catch clause is present.

Then add the following test cases to each rule's test file:
1. Invalid: `try { fs.writeFileSync(p, d); } finally { cleanup(); }` — should be flagged (no catch).
2. Valid: `try { fs.writeFileSync(p, d); } catch (e) {} finally { cleanup(); }` — should pass (has catch).

Run the test suite after making the changes to confirm all tests pass.

Generated by ✅ Contribution Check · 160.8 AIC · ⌖ 33.9 AIC · ⊞ 6.2K · ◷

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

[github-actions]

⚠️ PR Code Quality Reviewer failed during code quality review.

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #44256 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).

[github-actions]

🧪 Test Quality Sentinel Report

✅ Test Quality Score: 90/100 — Excellent

Analyzed 4 test(s): 4 design, 0 implementation, 0 violation(s).

📊 Metrics (4 tests)

Metric	Value
Analyzed	4 (Go: 0, JS: 4)
✅ Design	4 (100%)
⚠️ Implementation	0 (0%)
Edge/error coverage	4 (100%)
Duplicate clusters	0
Inflation	Yes (justified)
🚨 Violations	0

Test	File	Classification	Notes
valid: try/catch/finally is treated as protective	require-fs-sync-try-catch.test.ts:70	Design contract	Tests fix acceptance (2 code paths)
invalid: try/finally without catch is not protective	require-fs-sync-try-catch.test.ts:77	Design contract	Tests bug fix (2 variants with error+suggestion validation)
valid: try/catch/finally is treated as protective	require-json-parse-try-catch.test.ts:155	Design contract	Tests fix acceptance (2 code paths)
invalid: try/finally without catch is not protective	require-json-parse-try-catch.test.ts:162	Design contract	Tests bug fix (1 variant with error+suggestion validation)

Verdict

✅ Passed. 0% implementation tests (threshold: 30%). All 4 tests are design-focused behavioral contracts directly testing the bug fix. No violations.

Summary: Tests directly validate that try/catch/finally is protected (valid) and try/finally without catch is not protected (invalid). Both happy-path and failure paths are exercised with full error message and suggestion output validation using ESLint's RuleTester. Test inflation ratio (30–45:1) is normal for ESLint rule testing due to multiple code variant validation per scenario.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

🧪 Test quality analysis by Test Quality Sentinel · 15.2 AIC · ⌖ 13.4 AIC · ⊞ 6.8K · ◷
_{Comment /review to run again}

[github-actions]

✅ Test Quality Sentinel: 90/100. 0% implementation tests (threshold: 30%).

[github-actions]

The fix is correct and well-tested.

TryStatement.handler in the ESTree AST is the catch clause node — it is null when there is no catch, which is exactly the right discriminator. A try/finally only guarantees cleanup code runs; the thrown error is re-thrown after finally completes, so the call is effectively unprotected.

Both rules are updated symmetrically, and the new test cases cover the two critical scenarios:

• try/finally (no catch) — correctly flagged as invalid
• try/catch/finally — correctly accepted as valid

No issues found.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 22.7 AIC · ⌖ 6.1 AIC · ⊞ 4.8K

[github-actions]

Skills-Based Review 🧠

Applied /diagnosing-bugs and /tdd — approving with one minor documentation suggestion.

📋 Key Themes & Highlights

Root Cause

The isInsideTryBlock predicate was checking only ancestor.type === "TryStatement", but a try/finally without a catch clause does not handle exceptions — the throw resumes propagating after finally runs. The fix correctly gates on ancestor.handler != null, which is the ESTree field for the catch clause.

Positive Highlights

• ✅ Minimal, surgical fix — 1-line change per rule, no logic restructuring needed
• ✅ Symmetric fix across both require-fs-sync-try-catch and require-json-parse-try-catch
• ✅ Tests cover both directions: try/finally (now invalid) and try/catch/finally (still valid)
• ✅ Auto-fix suggestion output is verified in the new test cases — wraps the call inside a new try/catch rather than discarding the outer finally

Minor Observation

• The isInsideTryBlock function comment in both files doesn't mention the handler != null invariant. See inline comments for a suggested documentation addition — this is non-blocking.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 53.8 AIC · ⌖ 6.66 AIC · ⊞ 6.6K
_{Comment /matt to run again}

[github-actions]

[/diagnosing-bugs] The handler != null guard is correct, but the function's existing comment doesn't document the catch-clause contract. A future reader seeing isInsideTryBlock return false for a try/finally block may be confused without this context.

💡 Suggested comment addition

// Walk from innermost ancestor outward. Stop marking a try as protective once a deferred
// callback boundary has been crossed (the callback fires after the try has returned).
// NOTE: only TryStatements with a catch clause (handler != null) are treated as protective;
// a bare try/finally does not catch exceptions — the throw continues propagating after finally.
let crossedDeferredBoundary = false;

This makes the protection contract self-documenting and prevents future contributors from widening the check back to handler == null cases.

@copilot please address this.

[github-actions]

[/diagnosing-bugs] Same handler != null fix as the fs rule — identical concern about the missing comment. Consider adding the same note here to keep both implementations self-consistent in their documentation.

💡 Suggested comment addition

// Walk from innermost ancestor outward. If we cross a deferred function boundary
// (e.g., a .then/.on/setTimeout callback), a try statement further out does NOT
// protect the node — the callback runs after the try has already returned.
// NOTE: only TryStatements with a catch clause (handler != null) are treated as protective;
// a bare try/finally does not catch exceptions — the throw continues propagating after finally.
let crossedDeferredBoundary = false;

@copilot please address this.

[github-actions]

Verdict: APPROVE ✅

The fix is correct and well-targeted. Grumpy-coder sub-agent found nothing; my independent pass agrees.

Review notes

What the fix does

TryStatement.handler is the AST node for the catch clause. Before this fix, isInsideTryBlock returned true for any enclosing TryStatement, including try/finally with no catch. A catch-less try/finally lets the exception propagate after cleanup — so the original code was a false negative. The one-liner guard ancestor.handler != null is the correct, minimal fix.

The change is symmetric across both rules (require-fs-sync-try-catch and require-json-parse-try-catch), and the new test cases confirm both the fixed invalid case and the still-valid try/catch/finally case.

Minor observation (non-blocking)

The new invalid: try/finally without catch test in require-fs-sync-try-catch.test.ts covers writeFileSync and readFileSync, but not appendFileSync (which is also in FS_SYNC_METHODS). The rule logic is method-agnostic so correctness is unaffected, but a third case would complete coverage parity with the full method set.

🔎 Code quality review by PR Code Quality Reviewer · 151.8 AIC · ⌖ 6.21 AIC · ⊞ 5.4K
_{Comment /review to run again}

[github-actions]

🎉 This pull request is included in a new release.

Release: v0.82.5