Skip to content

feat(rees): add error-swallow analyzer for empty catch blocks (#2014) - #3525

Closed
RealDiligent wants to merge 2 commits into
JSONbored:mainfrom
RealDiligent:feat/enrichment-error-swallow-2014
Closed

feat(rees): add error-swallow analyzer for empty catch blocks (#2014)#3525
RealDiligent wants to merge 2 commits into
JSONbored:mainfrom
RealDiligent:feat/enrichment-error-swallow-2014

Conversation

@RealDiligent

Copy link
Copy Markdown
Contributor

Summary

Closes #2014
Part of #1499

Supersedes #3516 (auto-closed for line-local scanning defects).

Test plan

  • review-enrichment: npm run build && npm run metadata:check
  • node --test test/error-swallow.test.ts test/analyzer-registry.test.ts (14/14 pass)
  • Root npm run typecheck
  • Multiline JS catch, context-hunk line numbers, Python adjacency tests added

RealDiligent and others added 2 commits July 5, 2026 23:37
…red#2014)

Detect JS/TS empty catches, unused bindings, and return-null swallow patterns
in added patch lines. Part of JSONbored#1499.

Co-authored-by: Cursor <cursoragent@cursor.com>
…SONbored#2014)

Collect added JS catch bodies across following diff lines, advance newLine
for context rows, use only the immediate next Python except body line, and
skip optional-catch blocks that run non-empty code without swallowing.

Co-authored-by: Cursor <cursoragent@cursor.com>
@RealDiligent
RealDiligent requested a review from JSONbored as a code owner July 5, 2026 15:38
@superagent-security

Copy link
Copy Markdown
Contributor

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

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

loopover-orb Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Caution

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

🛑 Gittensory review result - reject/close recommended

Review updated: 2026-07-05 15:40:05 UTC

10 files · 1 AI reviewer · 1 blocker · readiness 80/100 · CI pending · blocked

🛑 Suggested Action - Reject/Close

  • AI reviewers agree on a likely critical defect: review-enrichment/src/analyzers/error-swallow.ts:42 uses `\b${binding}\\b` semantics for all catch bindings, but `$` is a valid JS identifier character and not a regex word character, so `catch ($err) { handle($err)
  • }` is misreported as `unused-binding`
  • change the binding check to use identifier-boundary lookarounds, e.g. ``const escaped = binding.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")
  • if (binding && new RegExp(`(?<![A-Za-z0-9_$])${escaped}(?![A-Za-z0-9_$])`).test(body)) return true
  • `` or tell me why `$` bindings are intentionally unsupported. — Resolve the flagged defect, or override if the AI reviewers are mistaken, then re-run the gate.

Review summary
This adds the requested REES analyzer and wires it through the registry, metadata, rendering, UI list, and analyzer-name allowlist, with tests for the main JS/TS and Python patch paths. The issue link is present and the generated metadata surfaces are updated consistently. I found one reachable correctness problem in the analyzer’s binding-use detection for valid JavaScript catch identifiers containing `$`, which can produce false positive unused-binding findings.

Blockers

  • review-enrichment/src/analyzers/error-swallow.ts:42 uses `\b${binding}\\b` semantics for all catch bindings, but `$` is a valid JS identifier character and not a regex word character, so `catch ($err) { handle($err); }` is misreported as `unused-binding`; change the binding check to use identifier-boundary lookarounds, e.g. ``const escaped = binding.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); if (binding && new RegExp(`(?<![A-Za-z0-9_$])${escaped}(?![A-Za-z0-9_$])`).test(body)) return true;`` or tell me why `$` bindings are intentionally unsupported.
Nits — 5 non-blocking
  • nit: review-enrichment/src/analyzers/error-swallow.ts:92 only looks at the immediate next added Python line, so multi-line `except Exception as e:` bodies that use/log the binding after a first setup line can be reported as `unused-binding`; if that is an intentional shallow heuristic, add a test or docs note so the analyzer contract is explicit.
  • nit: review-enrichment/src/analyzers/error-swallow.ts:57 treats any `.error(` or `.warn(` call as logging even when it is unrelated to the caught error, which is fine as a low-noise heuristic but should have a targeted test because it defines the analyzer’s false-negative posture.
  • Add regression coverage in review-enrichment/test/error-swallow.test.ts for `$`-prefixed and `$`-suffixed JS catch bindings that are actually referenced.
  • Add one Python test that documents the chosen behavior for a multi-line `except` body where the first added statement does not mention the binding but a later added statement logs or rethrows it.
  • Consider sharing the limits values from the analyzer descriptor source when generating review-enrichment/analyzer-metadata.json and apps/gittensory-ui/src/lib/rees-analyzers.ts, so the three copies cannot drift silently.

Why this is blocked

  • review-enrichment/src/analyzers/error-swallow.ts:42 uses `\b${binding}\\b` semantics for all catch bindings, but `$` is a valid JS identifier character and not a regex word character, so `catch ($err) { handle($err); }` is misreported as `unused-binding`; change the binding check to use identifier-boundary lookarounds, e.g. ``const escaped = binding.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); if (binding && new RegExp(`(?<![A-Za-z0-9_$])${escaped}(?![A-Za-z0-9_$])`).test(body)) return true;`` or tell me why `$` bindings are intentionally unsupported.
Signal Result Evidence
Code review ❌ 1 blocker 1 reviewer
Linked issue ✅ Linked #2014
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low 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: 181 registered-repo PR(s), 37 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor RealDiligent; Gittensor profile; 181 PR(s), 0 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Review context
  • Author: RealDiligent
  • 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: 181 PR(s), 0 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Await review-lane availability.
  • Refresh registry data or choose a registered active repo.
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

@loopover-orb

loopover-orb Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

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:42 uses `\b${binding}\\b` semantics for all catch bindings, but `$` is a valid JS identifier character and not a regex word character, so `catch ($err) { handle($err); }` is misreported as `unused-binding`; change the binding check to use identifier-boundary lookarounds, e.g. ``const escaped = binding.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); if (binding && new RegExp(`(?<![A-Za-z0-9_$])${escaped}(?![A-Za-z0-9_$])`).test(body)) return true;`` or tell me why `$` bindings are intentionally unsupported.). 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.

@loopover-orb loopover-orb Bot closed this Jul 5, 2026
@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.09%. Comparing base (fc021a5) to head (da972fe).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3525   +/-   ##
=======================================
  Coverage   93.09%   93.09%           
=======================================
  Files         301      301           
  Lines       31450    31450           
  Branches    11483    11483           
=======================================
  Hits        29279    29279           
  Misses       1517     1517           
  Partials      654      654           
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.

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 0.25x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

1 participant