Skip to content

feat(enrichment): add error-swallow catch analyzer#3356

Closed
jaso0n0818 wants to merge 1 commit into
JSONbored:mainfrom
jaso0n0818:feat/error-swallow-analyzer-2014
Closed

feat(enrichment): add error-swallow catch analyzer#3356
jaso0n0818 wants to merge 1 commit into
JSONbored:mainfrom
jaso0n0818:feat/error-swallow-analyzer-2014

Conversation

@jaso0n0818

Copy link
Copy Markdown
Contributor

Closes #2014

What

A new local REES analyzer, errorSwallow, that 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 the headless reviewer often misses. Pure compute over added diff lines, no network.

Detection (single-line, precision-first)

  • Scoped to JS/TS (a catch block) and Python (except …: pass). Non-source and other extensions are skipped.
  • Single-line by design (the catch/except and its body on one added line — the compact form the pattern targets, following actions-pin.ts): a body spread across multiple lines is not tracked — the safe, false-negative direction, with no cross-line state.
  • Three kinds, matching the issue's definition exactly:
    • empty-catch — an empty body (a comment-only body counts, since it swallows too).
    • return-null — the body just return null/return undefined.
    • unused-binding — a body that has a binding but neither rethrows (throw), logs (console.*/logger.*/log(/…), nor references the caught binding. A bindingless catch { … } is never an unused-binding.
  • String/comment blanking first (via secret-log.ts's codeOnly plus same-line comment strip), so a catch {} inside a string literal or a // } catch (e) {} note is not matched. Added lines only, line-cited via hunk headers, with the shared \ No newline line-counter fix; findings capped (maxFindings: 25) per file and globally.

Registration

Registered as a local descriptor (category quality, cost local, requires ["files"]) with an inline render(), following the redos/actions-pin descriptor shape. All wiring updated: types.ts (ErrorSwallowFinding + errorSwallow? key), render.ts, analyzer-registry.test.ts, root src/review/enrichment-analyzer-names.ts (canonical REES_ANALYZER_NAMES), root test/unit/enrichment-wire.test.ts, and the generated analyzer-metadata.json / rees-analyzers.ts / .env.example via node scripts/generate-analyzer-metadata.mjs.

Tests

review-enrichment/test/error-swallow.test.ts (12 tests) covers: empty catch flagged (with/without binding, comment-only body), return-null, unused-binding (ignored binding), a catch that rethrows/logs/references the binding NOT flagged, a bindingless catch with real work not flagged, catch inside a string/comment not matched, Python except: pass flagged vs a handling except not, non-JS/Py files skipped, added-line scanning with exact locations, added-lines-only with line-number accuracy across mixed hunks, the per-file cap + maxFindings: 0, the entrypoint's global cap across files, and the no-files case. Analyzer metadata is regenerated and committed.

@jaso0n0818 jaso0n0818 requested a review from JSONbored as a code owner July 5, 2026 04:45
@superagent-security

Copy link
Copy Markdown

Superagent didn't find any vulnerabilities or security issues in this PR.

1 similar comment
@superagent-security

Copy link
Copy Markdown

Superagent didn't find any vulnerabilities or security issues in this PR.

@gittensory-orb gittensory-orb Bot added the gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 1.25x multiplier. label Jul 5, 2026
@gittensory-orb

gittensory-orb Bot commented Jul 5, 2026

Copy link
Copy Markdown

Caution

🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥

🛑 Gittensory review result - reject/close recommended

Review updated: 2026-07-05 04:49:39 UTC

11 files · 1 AI reviewer · 1 blocker · readiness 62/100 · CI pending · blocked

🛑 Suggested Action - Reject/Close

  • AI reviewers agree on a likely critical defect: review-enrichment/src/analyzers/error-swallow.ts:48 flags any catch body containing `return null`/`return undefined` as `return-null`, so `catch (e) { if (recoverable) return null
  • throw e
  • }` is incorrectly reported even though the body rethrows
  • make this exact-body matching, e.g. `const RETURN_NULL_RE = /^\s*return\s+(?:null|undefined)\s*
  • ?\s*$/
  • `. — Resolve the flagged defect, or override if the AI reviewers are mistaken, then re-run the gate.

Review summary
The change wires a new local error-swallow analyzer through the registry, rendered brief, metadata, UI docs, root analyzer names, and focused tests. The end-to-end registration is coherent, but the core JS classifier is too broad in two reachable cases, producing false positives despite the PR's precision-first contract.

Blockers

  • review-enrichment/src/analyzers/error-swallow.ts:48 flags any catch body containing `return null`/`return undefined` as `return-null`, so `catch (e) { if (recoverable) return null; throw e; }` is incorrectly reported even though the body rethrows; make this exact-body matching, e.g. `const RETURN_NULL_RE = /^\s*return\s+(?:null|undefined)\s*;?\s*$/;`.
  • review-enrichment/src/analyzers/error-swallow.ts:54 interpolates the caught binding directly into a RegExp, so valid bindings containing `$` are treated as regex syntax and a real reference like `catch ($err) { report($err); }` is falsely reported as `unused-binding`; escape the identifier before building the regex, e.g. `const escapedBinding = binding.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");` and test `new RegExp(`\\b${escapedBinding}\\b`).test(body)`.
Nits — 6 non-blocking
  • nit: review-enrichment/src/analyzers/error-swallow.ts:21 says Python is scoped to a bare `except …: pass`, but `PY_EXCEPT_PASS_RE` also matches typed handlers like `except ValueError: pass`; either adjust the comment/docs wording or narrow the regex.
  • nit: review-enrichment/src/analyzers/error-swallow.ts:22 only matches Python `pass` with no trailing comment, so `except ValueError: pass # intentionally ignored` is missed even though comments are meant to be ignored for detection.
  • Add tests in review-enrichment/test/error-swallow.test.ts for mixed bodies containing `return null` plus `throw`/logging/reference so the `return-null` kind stays limited to a body that just returns null/undefined.
  • Add a JS test for `$` catch bindings, e.g. `catch ($err) { report($err); }`, to lock down identifier escaping in the unused-binding path.
  • Either update the Python docs/comments to say typed `except ...: pass` is intentionally covered, or rename the implementation comment away from "bare except" so the source matches the behavior.
  • Readiness score is below the configured threshold — Use the readiness panel as advisory maintainer context; the score does not block this PR.

Why this is blocked

  • review-enrichment/src/analyzers/error-swallow.ts:48 flags any catch body containing `return null`/`return undefined` as `return-null`, so `catch (e) { if (recoverable) return null; throw e; }` is incorrectly reported even though the body rethrows; make this exact-body matching, e.g. `const RETURN_NULL_RE = /^\s*return\s+(?:null|undefined)\s*;?\s*$/;`.
Signal Result Evidence
Code review ❌ 1 blocker 1 reviewer
Linked issue ✅ Linked #2014
Related work ⚠️ 3 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Change scope ❌ 8/20 High review scope from cached public metadata (1 linked issue).
Validation posture ❌ 5/25 Preflight is holding this PR: the review lane is unavailable, so it is not ready for automated review.
Contributor workload ✅ 10/10 Author activity: 444 registered-repo PR(s), 267 merged, 7 issue(s).
Contributor context ✅ Confirmed Gittensor contributor jaso0n0818; Gittensor profile; 444 PR(s), 7 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Review context
  • Author: jaso0n0818
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 444 PR(s), 7 issue(s).
  • Related work: Titles/paths share 5 meaningful terms. (issue #1477, issue #1514)
  • Related work: Titles/paths share 5 meaningful terms. (issue #1514, issue #2025)
  • Related work: Titles/paths share 7 meaningful terms. (issue #2033, issue #2017)
  • Additional title-only matches omitted; title-only overlap does not block.
Contributor next steps
  • Review top overlaps.
  • Add a concise scope and risk note.
  • Await review-lane availability.
  • Refresh registry data or choose a registered active repo.
  • Check active issues and PRs before submitting.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@codecov

codecov Bot commented Jul 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.88%. Comparing base (5584276) to head (d130570).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3356   +/-   ##
=======================================
  Coverage   93.88%   93.88%           
=======================================
  Files         280      280           
  Lines       30562    30562           
  Branches    11132    11132           
=======================================
  Hits        28694    28694           
  Misses       1211     1211           
  Partials      657      657           
Files with missing lines Coverage Δ
src/review/enrichment-analyzer-names.ts 100.00% <ø> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gittensory-orb

gittensory-orb Bot commented Jul 5, 2026

Copy link
Copy Markdown

Gittensory is closing this pull request on the maintainer's behalf (AI reviewers agree on a likely critical defect: review-enrichment/src/analyzers/error-swallow.ts:48 flags any catch body containing `return null`/`return undefined` as `return-null`, so `catch (e) { if (recoverable) return null; throw e; }` is incorrectly reported even though the body rethrows; make this exact-body matching, e.g. `const RETURN_NULL_RE = /^\s*return\s+(?:null|undefined)\s*;?\s*$/;`.). This is an automated maintenance action — to pursue this change, please open a new pull request with the issues resolved. Closed PRs may be analyzed later to improve review accuracy, but they are not automatically reopened or re-reviewed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 1.25x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(enrichment): empty-catch / error-swallow analyzer

1 participant