Add internal exception handling to _send_error_notification

[dcellison]

Summary

Move exception handling into _send_error_notification() so it never raises. A notification helper that can propagate exceptions defeats its own purpose - it exists to tell the user something went wrong, and a failure in that notification should be logged and swallowed, not bubbled up.

Changes

Change	Detail
_send_error_notification()	Added try/except around the HTTP request; logs warning on failure
triage_issue() caller	Removed redundant try/except wrapper (function now handles its own errors)
Docstring	Added "Never raises" contract note

Test plan

• test_does_not_raise_on_connection_error - ConnectionError caught, function returns cleanly
• test_does_not_raise_on_timeout - TimeoutError caught, function returns cleanly
• test_logs_warning_on_failure - warning logged with repo and issue number
• All 1129 tests pass, make check clean

Fixes #162

[dcellison]

Review by Kai

Review: #175

The design decision is sound — a notification helper that can raise defeats its purpose. The implementation is mostly clean, with one meaningful finding.

---

Warning: Exception details silently discarded

triage.py, new except Exception block (~line 838)

The original caller used log.exception(...), which captures the full traceback. The replacement uses log.warning(...) with no exception info:

except Exception:
    log.warning(
        "Failed to send triage error notification for %s#%d",
        metadata.repo,
        metadata.number,
    )

The why of the failure (network refused, TLS error, DNS failure, etc.) is completely lost. When notifications silently stop working in production, you'll have no way to diagnose it from logs.

Fix: add exc_info=True:

log.warning(
    "Failed to send triage error notification for %s#%d",
    metadata.repo,
    metadata.number,
    exc_info=True,
)

---

Suggestion: Minor log message inconsistency

The non-200 branch (triage.py ~line 832) logs without repo/issue context, while the exception branch does include it. Not a bug, but the non-200 path is now harder to correlate to a specific issue in logs. Low priority.

---

Tests

The three new tests cover the stated contract. One note: the AsyncMock(side_effect=...) pattern used for session.post is technically mocking the call site, but aiohttp's session.post(...) is used as an async context manager — the exception would normally surface during __aenter__, not the call itself. If these tests are green and not vacuous, the mock wiring is working; worth a quick sanity-check that the side_effect is actually being triggered rather than the tests passing because no assertion fires on a clean mock path.

---

Overall: Approve with the exc_info=True fix. The refactor is correct and the "never raises" contract is the right design.

[dcellison]

Review by Kai

Review: #175 (updated)

The prior review's primary finding — exc_info=True missing from the warning — has been addressed. The implementation now matches the stated contract.

---

No new findings

The diff is clean:

• exc_info=True is present in the except Exception block (triage.py, new line ~838) — exception details will appear in logs.
• The try/except correctly wraps the full async with block, so both session creation failures and request failures are caught.
• The caller in triage_issue() is correctly simplified — no wrapper needed since the callee guarantees no raise.
• The docstring update accurately reflects the new contract.

The non-200 log message still lacks repo/issue context (raised as low priority in the prior review), but that's a pre-existing gap the author has already seen.

---

Approve. The refactor is correct, the contract is enforced, and the key logging regression is fixed.