eslint-factory: require null-safe companion for typeof err === "object" catch guards

[Copilot]

Both no-unsafe-catch-error-property and no-unsafe-promise-catch-error-property treated bare typeof err === "object" as a safe guard, which is unsound because null satisfies that check. This change tightens guard semantics so typeof only suppresses unsafe-property diagnostics when paired with a non-null/truthy guard.

• Guard semantics tightened in both rules

  • Updated guard detection in:
    • eslint-factory/src/rules/no-unsafe-catch-error-property.ts
    • eslint-factory/src/rules/no-unsafe-promise-catch-error-property.ts
  • typeof err === "object" (or reversed form) now requires a companion non-null signal before setting guarded state:
    • err !== null
    • err != null
    • err && typeof err === "object"
    • prior if (!err) return; followed by typeof err === "object"

• Existing guard behavior preserved

  • err instanceof Error remains a valid guard.
  • getErrorMessage(err) guard behavior is unchanged.

• Tests realigned with intended safety model

  • Moved bare-typeof cases from valid to invalid in catch and promise-catch suites.
  • Kept null-checked / truthy-checked variants valid, including parity cases across both rules.

// now invalid (insufficient)
if (typeof err === "object") {
  console.log(err.status);
}

// remains valid (null-safe)
if (typeof err === "object" && err !== null) {
  console.log(err.status);
}

[github-actions]

Great work on this fix! 👋 Tightening the typeof err === "object" guard semantics to require a null-safe companion is a real correctness improvement — catching the subtle null case that was silently slipping through both no-unsafe-catch-error-property and no-unsafe-promise-catch-error-property is exactly the kind of precision this linting layer needs.

The PR is well-structured: the logic change, the test realignment (moving bare-typeof cases from valid to invalid), and the parity maintained across both rule suites all look solid. Description is thorough and the before/after code examples make the intent immediately clear.

This looks ready for reviewer eyes. 🚀

Generated by ✅ Contribution Check · 251.3 AIC · ⌖ 20.4 AIC · ⊞ 6.3K · ◷

[github-actions]

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

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

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

[github-actions]

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

[github-actions]

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

[Copilot]

Pull request overview

Tightens the null-safety model in the eslint-factory rules that allow property access on caught/rejected values, ensuring typeof err === "object" is only treated as a safe guard when paired with a non-null/truthy signal (since null also satisfies typeof === "object").

Changes:

• Added non-null companion guard tracking (hasNonNullGuard) and new guard-detection helpers to both unsafe-error-property rules.
• Realigned tests so bare-typeof cases are invalid and null-checked / truthy-checked variants remain valid.
• Updated the generated pr-triage-agent workflow lockfile, bumping gh-aw-firewall container tags.

Show a summary per file

File	Description
eslint-factory/src/rules/no-unsafe-promise-catch-error-property.ts	Adds non-null companion guard tracking and updates typeof guard semantics for .catch() callbacks.
eslint-factory/src/rules/no-unsafe-promise-catch-error-property.test.ts	Moves bare-typeof cases to invalid and adds valid null-safe variants for promise .catch().
eslint-factory/src/rules/no-unsafe-catch-error-property.ts	Adds non-null companion guard tracking and updates typeof guard semantics for catch (err) blocks.
eslint-factory/src/rules/no-unsafe-catch-error-property.test.ts	Moves bare-typeof cases to invalid and adds valid null-safe variants for catch blocks.
.github/workflows/pr-triage-agent.lock.yml	Regenerates lockfile with updated gh-aw-firewall container tags.

Review details

Tip

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

• Files reviewed: 5/5 changed files
• Comments generated: 3
• Review effort level: Low

[github-actions]

Test Quality Sentinel Report

Score: 94/100 - Excellent (TypeScript/vitest tests analyzed directly from branch; .test.ts not captured by pre-fetch step)

Analyzed 5 tests across 2 files: 5 design tests (100%), 0 implementation tests (0%), 4 with edge/error cases (80%), 0 duplicate clusters, no test inflation, 0 guideline violations.

Test classification summary:

• valid: typeof err === 'object' with non-null guard (no-unsafe-catch-error-property.test.ts:345) - Design - covers 5 null-check patterns
• invalid: bare typeof err === 'object' guard is insufficient (no-unsafe-catch-error-property.test.ts:358) - Design - error assertions + suggestion output checks
• valid: typeof err === 'object' with truthy err guard (no-unsafe-catch-error-property.test.ts:470) - Design - single valid case
• valid: typeof err === 'object' with non-null guard in .catch() (no-unsafe-promise-catch-error-property.test.ts:243) - Design - 5 null-check patterns
• invalid: bare typeof err === 'object' guard insufficient in .catch() (no-unsafe-promise-catch-error-property.test.ts:256) - Design - error assertions

Test inflation: +47 test vs +74 prod (ratio 0.63); +24 test vs +75 prod (ratio 0.32). Both well under 2:1.

Verdict: Check passed. 0% implementation tests (threshold 30%). No violations. All tests enforce behavioral contracts confirming bare typeof without null-safe companion is rejected, and 5 null-check variants are accepted.

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 · 65.9 AIC · ⌖ 10.5 AIC · ⊞ 7K · ◷
_{Comment /review to run again}

[github-actions]

Test Quality Sentinel: 94/100. 0% implementation tests (threshold: 30%). All 5 TypeScript/vitest tests are behavioral design tests with no guideline violations.

[github-actions]

Review: eslint-factory null-safe typeof guard tightening

Overall: The direction is correct and important — bare typeof err === "object" genuinely allows null through, and this PR closes that unsound path. The LogicalExpression conjunct collector and the !err return early-exit pattern are solid. Three issues need addressing before merge.

🔴 Blocking

1. IfStatement handler misses block-body early return (see inline comment on line 173)

if (!err) { return; } — the common braced form — is not recognized as a non-null guard because the consequent is a BlockStatement, not a bare ReturnStatement. This produces a false negative that counterparts the feature being introduced. The same gap is in no-unsafe-promise-catch-error-property.ts.

🟡 Non-blocking

2. Helper functions duplicated verbatim (see inline comment on promise rule line 36)

isTypeofObjectCheck and isNonNullGuardCheck are copied word-for-word into both rule files. Extracting them to a shared guard-helpers.ts prevents future divergence.

3. BinaryExpression cross-block null-check false negative (see inline comment on catch rule line 135)

A standalone if (err !== null) { doX(); } followed by a separate if (typeof err === 'object') { err.status } is incorrectly treated as safe. At minimum this deserves a known-limitations comment; ideally tighten to require the null-check to appear as a direct IfStatement test.

What's working well

• LogicalExpression conjunct flattening correctly handles all orderings of typeof/null within the same expression.
• !err return (bare form) correctly sets hasNonNullGuard.
• Test suite is substantially improved; moving bare-typeof cases to invalid is the right call.
• Both rules updated in parity.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 59.5 AIC · ⌖ 8.65 AIC · ⊞ 4.9K

[github-actions]

Code duplication — extract shared helpers. isTypeofObjectCheck and isNonNullGuardCheck are copied verbatim from no-unsafe-catch-error-property.ts. Any future logic change will need to be applied in both files, making divergence likely.

Consider extracting them to a shared utility, e.g. eslint-factory/src/rules/guard-helpers.ts, then importing from both rules.

@copilot please address this.

[github-actions]

IfStatement handler misses block-body early return. node.consequent.type === AST_NODE_TYPES.ReturnStatement only matches bare if (!err) return;, but NOT the equally common if (!err) { return; } (a BlockStatement whose first/only statement is ReturnStatement).

Example that will NOT be recognized as a non-null guard today:

} catch (err) {
  if (!err) { return; }             // consequent is BlockStatement — missed!
  if (typeof err === 'object') {
    console.log(err.status);        // false negative: rule stays silent
  }
}

Consider also matching:

node.consequent.type === AST_NODE_TYPES.BlockStatement &&
node.consequent.body.length === 1 &&
node.consequent.body[0].type === AST_NODE_TYPES.ReturnStatement

The same gap exists in no-unsafe-promise-catch-error-property.ts.

@copilot please address this.

[github-actions]

Potential false negative: BinaryExpression cross-block null-check. The hasNonNullGuard flag is set for any err !== null expression seen anywhere in the catch block. This means the following will incorrectly be considered safe:

} catch (err) {
  if (err !== null) {
    doSomethingElse();   // null check scoped only here
  }
  // err might be null again here
  if (typeof err === 'object') {
    console.log(err.status);  // false negative: hasNonNullGuard=true from above
  }
}

The LogicalExpression handler correctly handles in-expression conjuncts. For sequential statements, only the !err return pattern (via IfStatement) is truly scoped. The BinaryExpression path treats any null-check anywhere in the block as sufficient.

This is an edge case but worth documenting as a known limitation (e.g., a comment), or tightening by only allowing BinaryExpression-based null checks that appear as the direct test of an IfStatement (not inside another block).

@copilot please address this.

[github-actions]

Review Summary 🧠

Applied /diagnosing-bugs, /tdd, /codebase-design — requesting changes on one blocking correctness bug.

🚨 Blocking: False negative in BinaryExpression handler

The new isNonNullGuardCheck call inside the BinaryExpression visitor sets hasNonNullGuard = true for any err !== null expression encountered in the catch frame — including standalone ones in unrelated if blocks. This means:

catch (err) {
  if (err !== null) { doSomething(); }  // sets hasNonNullGuard for the frame
  if (typeof err === 'object') {        // sees hasNonNullGuard → hasGuard = true
    console.log(err.status);            // allowed — but null.status would throw!
  }
}

Fix: remove the isNonNullGuardCheck branch from BinaryExpression in both rule files. The inline && case is already handled entirely by LogicalExpression. IfStatement handles the early-return pattern, and the BinaryExpression path for typeof still uses hasNonNullGuard correctly.

Also add a regression test for this pattern so it can't silently regress.

⚠️ Non-blocking findings

• DRY: isTypeofObjectCheck / isNonNullGuardCheck are copy-pasted into both files — extract to guard-helpers.ts.
• throw early exit: IfStatement handler only recognises return, not throw — both are valid early exits.
• Nested guard scope: IfStatement fires at any nesting depth; a if (!err) return; inside a nested if may not guard subsequent top-level code.

@copilot please address the review comments above.

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

[github-actions]

Skills-Based Review 🧠

Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on a correctness bug in the new guard detection logic.

📋 Key Themes & Highlights

Key Themes

• False negative in BinaryExpression handler (blocking): The new isNonNullGuardCheck branch in BinaryExpression sets hasNonNullGuard = true for any err !== null expression seen in the catch frame — including standalone ones in separate if blocks. This causes a subsequent bare typeof err === 'object' to incorrectly pass as guarded, defeating the whole purpose of the PR. The inline && case is fully covered by LogicalExpression; the early-return case by IfStatement. The BinaryExpression path for isNonNullGuardCheck should be removed.

• Missing regression test (blocking): There is no test asserting that if (err !== null) { } if (typeof err === 'object') { err.status; } is invalid. Without it the false negative is undetectable and can regress silently.

• DRY violation (non-blocking): isTypeofObjectCheck and isNonNullGuardCheck are copy-pasted identically into both rule files. Extract to a shared guard-helpers.ts.

• IfStatement handler gaps (non-blocking): Only return is recognized as an early exit (missing throw); the visitor also fires for nested if statements that may not guard top-level code.

Positive Highlights

• ✅ The core insight is correct and important: typeof null === 'object' makes bare typeof an unsound guard.
• ✅ The LogicalExpression visitor correctly flattens && chains and handles all conjunct orderings.
• ✅ The !err early-return pattern is a nice addition to the guard vocabulary.
• ✅ Test names are updated to describe the actual invariant, not just the old behaviour.
• ✅ Both rules get symmetric treatment.

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

[github-actions]

[/diagnosing-bugs] This BinaryExpression path introduces a false negative: any standalone if (err !== null) { ... } in the catch block sets hasNonNullGuard = true for the whole frame, making a subsequent bare if (typeof err === 'object') incorrectly appear as fully guarded.

💡 Root cause & suggested fix

The pattern that would be incorrectly allowed:

catch (err) {
  if (err !== null) { doSomething(); } // BinaryExpression → hasNonNullGuard = true
  if (typeof err === 'object') {        // sees hasNonNullGuard → sets hasGuard = true
    console.log(err.status);            // UNSAFE: null.status throws!
  }
}

The inline && case is already handled exclusively by LogicalExpression (by the time children's BinaryExpression fires, hasGuard is already true and the visitor early-returns). The isNonNullGuardCheck branch here is redundant for inline && and harmful for the standalone case.

Fix: remove the isNonNullGuardCheck branch from BinaryExpression:

BinaryExpression(node) {
  // ...
  // isNonNullGuardCheck removed — LogicalExpression covers inline &&,
  // IfStatement sets hasNonNullGuard for the early-return pattern
  if (isTypeofObjectCheck(node, top.varName) && top.hasNonNullGuard) {
    top.hasGuard = true;
  }
},

@copilot please address this.

[github-actions]

[/diagnosing-bugs] Same false negative as in the catch rule: any standalone err !== null expression inside the .catch() callback sets hasNonNullGuard = true for the whole frame, incorrectly marking a subsequent bare typeof err === 'object' guard as safe.

💡 Fix

Remove the isNonNullGuardCheck branch from BinaryExpression here, mirroring the fix for no-unsafe-catch-error-property.ts. The LogicalExpression visitor already handles inline && patterns; IfStatement handles the early-return pattern.

@copilot please address this.

[github-actions]

[/tdd] The new invalid test block covers bare typeof but is missing a regression test for the BinaryExpression false negative: a standalone err !== null in a separate if should not guard a subsequent typeof check.

💡 Suggested test to add

it("invalid: err !== null in a separate if does not guard a subsequent typeof check", () => {
  cjsRuleTester.run("no-unsafe-catch-error-property", noUnsafeCatchErrorPropertyRule, {
    valid: [],
    invalid: [
      {
        code: `try { f(); } catch (err) { if (err !== null) { } if (typeof err === 'object') { console.log(err.status); } }`,
        errors: [
          {
            messageId: "unsafeProperty",
            data: { prop: "status", errorVar: "err" },
          },
        ],
      },
    ],
  });
});

Without this, the fix to the BinaryExpression handler cannot be verified and the false negative can silently regress.

@copilot please address this.

[github-actions]

[/codebase-design] isTypeofObjectCheck and isNonNullGuardCheck are copy-pasted verbatim into both rule files, creating a maintenance trap: any future change to guard semantics must be applied in two places.

💡 Suggested extraction

Extract to a shared module, e.g. eslint-factory/src/rules/guard-helpers.ts:

// guard-helpers.ts
import { AST_NODE_TYPES, TSESTree } from "`@typescript-eslint/utils`";

export function isTypeofObjectCheck(node: TSESTree.Expression, varName: string): boolean { ... }
export function isNonNullGuardCheck(node: TSESTree.Expression, varName: string): boolean { ... }

Then import in each rule:

import { isTypeofObjectCheck, isNonNullGuardCheck } from "./guard-helpers";

This also enables testing the helpers in isolation, which would directly expose edge cases like the false negative discussed in the BinaryExpression handler.

@copilot please address this.

[github-actions]

[/diagnosing-bugs] The early-exit check only recognises return; a throw statement is equally valid as a non-null escape hatch but won't set hasNonNullGuard.

💡 Suggested fix

const consequent = node.consequent;
const isEarlyExit =
  consequent.type === AST_NODE_TYPES.ReturnStatement ||
  consequent.type === AST_NODE_TYPES.ThrowStatement;
if (isEarlyExit) {
  top.hasNonNullGuard = true;
}

The same change is needed in no-unsafe-promise-catch-error-property.ts.

@copilot please address this.

[github-actions]

[/diagnosing-bugs] The IfStatement visitor fires for any depth in the catch block, including if (!err) return; nested inside another branch. A deeply-nested early exit may not actually guard the top-level catch body, creating a potential false negative.

💡 Example & mitigation

catch (err) {
  if (someCondition) {
    if (!err) return;  // only runs when someCondition is truthy
  }
  // err could still be falsy here — but hasNonNullGuard is now set!
  if (typeof err === 'object') {
    console.log(err.status);  // incorrectly allowed
  }
}

Consider restricting hasNonNullGuard = true to IfStatement nodes whose parent is the CatchClause's BlockStatement directly (depth-1 check). This trades off some false-positive tolerance for correctness on the false-negative side.

@copilot please address this.