Skip to content

[Refactor]: Remove silent try/catch around instanceof in is/index.ts #72

Description

@martyy-code

Current State

In packages/errors/src/is/index.ts:66-73, the is() function wraps a try { ... } catch {} block around the instanceof check to defend against a "cross-realm errors" scenario that has never been demonstrated in the codebase, the test suite, or any open issue. The block is silent (catch {} with no re-raise, no log, no comment beyond a one-line scenario hypothesis).

if (typeof ErrorType === 'function' && 'prototype' in ErrorType) {
  try {
    if (error instanceof ErrorType) {
      return true;
    }
  } catch {
    // instanceof can fail for cross-realm errors
  }
}

This violates two rules:

  • Rule 0004 (No Speculative Defences): a guard must cover a demonstrated scenario, not a hypothetical one. The rule's source materials (Basarat Ali Syed, Ryan Cavanaugh, Vladimir Khorikov) explicitly carve out the == null idiom for absent values but offer no carve-out for try { } catch {} without a named bug reference.
  • Rule 0001 invariant 6 (no silent failures): an empty catch is exactly the smell the invariant forbids.

Located in:

  • packages/errors/src/is/index.ts:66-73

Problems with current implementation:

  • The cross-realm scenario has not been reported. No issue, no test, no production log references it.
  • The catch {} is silent: if instanceof ever does throw for a real reason, the user gets false back without any signal that the discrimination failed.
  • A reader has to determine whether the cross-realm scenario is real before they can trust the rest of the function. Every reader pays that tax.

Proposed State

After refactoring, the try { } catch {} is removed. The instanceof check is performed directly:

if (typeof ErrorType === 'function' && 'prototype' in ErrorType) {
  return error instanceof ErrorType;
}

If a cross-realm bug is ever reported, the guard is added back with the bug number in a comment (per rule 0004's "Bug-driven addition only" enforcement clause). Until then, the code path is the cleanest expression of the contract.

Expected improvements:

  • Compliance with rule 0004 (no speculative defences).
  • Compliance with rule 0001 invariant 6 (no silent failures).
  • The reader can trust the is() function without first auditing the cross-realm hypothesis.
  • The test suite is honest about what it covers: today, the function discriminates same-realm errors only; that contract is documented, not hidden behind a swallowed catch.

Motivation

This refactoring is needed because:

  • The guard violates the project's stated discipline (rule 0004) and one of the ten absolute invariants (rule 0001 invariant 6).
  • The catch silently swallows any future throw from instanceof for a reason that has nothing to do with cross-realm errors (e.g. a malformed ErrorType argument, a V8 regression). The user is told false with no signal.
  • The cost of the guard (silent failure on the unhappy path) is paid forever; the benefit (defence against an unobserved scenario) is hypothetical.

Triggers for this work:

  • Working on feature X and encountered this (audit of packages/errors/src/ against the 16 architecture rules, August 2026)
  • Technical debt accumulation
  • Performance issues
  • Maintainability concerns

Risks

Potential risks:

  • Risk 1: If a consumer passes a Proxy-wrapped or revoked ErrorType, instanceof may throw. — Mitigation: this is a different category from "cross-realm"; rule 0004 says the guard should be added when the bug is reported, with the bug number. Until then, the throw propagates to the caller, which is the correct behaviour (the caller passed the malformed value; the function is not the right place to silently lie about the result).
  • Risk 2: Test suite regression if a test currently relies on the silent catch. — Mitigation: search tests/ for any test that passes a malformed ErrorType and asserts is() returns false. If found, the test is updated to assert the throw (the test is documenting the contract the rule now enforces).
  • Risk 3: Consumer code that depends on is() not throwing for any input. — Mitigation: the public function never threw before; the change preserves that contract for valid inputs. For invalid inputs (malformed ErrorType), the throw is the correct behaviour and matches TypeScript's own instanceof semantics.

Migration Plan

Migration approach:

  1. Remove the try { ... } catch {} wrapper. Inline the instanceof check.
  2. Update the inline comment to document the new contract: the function discriminates same-realm errors; cross-realm discrimination is out of scope until a bug is reported.
  3. Search packages/errors/tests/ for any test that exercises the cross-realm path. Update or remove such tests.
  4. Run the full test suite; document the change in the PR description.

Rollback plan: revert the PR. The change is local to is/index.ts.

Backward Compatibility

  • This refactoring maintains full backward compatibility
  • This refactoring has breaking changes (migration required)
  • This refactoring deprecates APIs (grace period needed)

Note: for valid inputs (which is the documented and tested contract), the function returns the same value as before. The behaviour change only affects malformed inputs, which the previous silent catch hid.

Scope

Files/Folders affected:

  • packages/errors/src/is/index.ts (lines 66-73)
  • packages/errors/tests/ (any test that exercises the silent catch)

Out of scope:

Component(s) Affected

  • packages/db — Drizzle ORM schema + PostgreSQL
  • packages/api — tRPC server
  • packages/api/auth — Better Auth configuration
  • .github/workflows — CI/CD GitHub Actions
  • Multiple Components

Note: the component_affected dropdown is calibrated for a web template project, not for the @deessejs/errors package. The actual affected component is packages/errors.

Priority

  • p0: Critical - Blocking major work or causing bugs
  • p1: High - Important, should do soon
  • p2: Medium - Normal priority
  • p3: Low - Nice to have

Estimated Effort

  • effort: xs - Few minutes
  • effort: s - Half a day
  • effort: m - 1-2 days
  • effort: l - A week or more (needs breakdown)

Test Coverage Requirements

  • Existing tests cover this code area (will update)
  • Need to add new tests for this refactor
  • This area lacks test coverage (technical debt)
  • Integration tests will be added/updated
  • E2E tests will be added/updated

Testing Approach

Testing strategy:

  • Unit tests: confirm is(err, SomeError) returns true / false for same-realm discrimination (existing contract).
  • New test: confirm is(err, validErrorType) no longer swallows a hypothetical throw — if the inputs are valid, the function returns; if the inputs are invalid, the throw propagates (documented as the new contract).

Verification steps:

  1. pnpm --filter @deessejs/errors test:run — existing tests pass; updated tests reflect the new contract.
  2. pnpm --filter @deessejs/errors type-check — no regressions.

Related Issues / Pull Requests

  • Related audit: P0 feat: implement raise() function #2 in the internal audit of packages/errors/src/ against rules 0001-0016, August 2026.
  • Related rule: docs/engineering/architecture/rules/0004-no-speculative-defences.md.
  • Related rule: docs/engineering/architecture/rules/0001-project-mindset.md (invariant 6: no silent failures).
  • Blocked by: none.
  • Related: P0 feat: implement is() type checking function #3 (redundant guard in the same function).

Relevant Documentation

  • Architecture doc: docs/engineering/architecture/rules/0004-no-speculative-defences.md
  • Architecture doc: docs/engineering/architecture/rules/0001-project-mindset.md (ten invariants)

Pre-Submission Checklist

  • I have searched existing issues for related refactoring requests
  • Risks and migration plan are documented
  • Test coverage approach is defined
  • I understand this issue will be labeled according to the project taxonomy
  • This is NOT a security vulnerability (see security note above)

Metadata

Metadata

Assignees

No one assigned

    Labels

    p0: criticalEverything stops, fix it nowtype: refactorRefactoring / code restructuring

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions