feat(api): add email-only notifier for high/medium API errors (#809) - #818
Conversation
Epic #616 Phase 3 / sub-issue #809。重要 API エラー (severity=high/medium) の メール通知サービス `services/notifier.ts` を新設し、AI 解析コールバック (`PUT /api/webhooks/github/ai-result/:id`) の成功時に 1 回だけ呼ぶよう配線する。 Slack はこのリポジトリでは使わないため非対応。`MONITORING_NOTIFY_EMAIL` 未設定 / severity が low/unknown の場合は no-op。本文には sentry_issue_id / severity / title / 管理画面 URL のみ載せ、Authorization / Cookie / 内部 URL など PII は混入させない。 Phase 3 / #809: introduce `notifier.ts` for email-only alerts on high/medium severity API errors. Slack is intentionally unsupported. The notifier no-ops when `MONITORING_NOTIFY_EMAIL` is unset or severity is low/unknown. The AI callback is the single call site so duplicate alerts can't happen, and sends are fire-and-forget so the webhook response isn't delayed by Resend.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds Phase‑3 API error email alerting: new env vars in example, a notifier service that builds/sends guarded emails for medium/high severities, webhook logic to pre-read and fire the notifier on severity transitions, and tests for service and webhook behaviors. ChangesAPI Error Email Notification (single cohort)
Sequence DiagramsequenceDiagram
participant GitHub
participant Webhook as GitHub AI Callback<br/>Route
participant DB as Database
participant Notifier as Notifier<br/>Service
participant Email as Email<br/>Service
participant Admin as Admin UI
GitHub->>Webhook: PUT /webhooks/github/ai-result/:id
Webhook->>DB: SELECT pre-update row (getApiErrorById)
DB-->>Webhook: pre row / not found
Webhook->>DB: UPDATE api_errors (updateAiAnalysis)
DB-->>Webhook: updated row
Webhook->>Webhook: publishApiErrorUpdate(updated)
Webhook->>Notifier: notifyApiErrorAlert(payload) (async, fire-and-forget) when severityBecameNotifiable(pre, updated)
Notifier->>Notifier: validate severity & env, build escaped subject/html, normalize ADMIN_BASE_URL
Notifier->>Email: sendEmail(to, subject, html)
Email-->>Notifier: success: true/false or error
Email->>Admin: Administrator receives email with optional link to Admin UI
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related Issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8d89e5075
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // no-op when severity is `low`/`unknown` or `MONITORING_NOTIFY_EMAIL` is | ||
| // unset. This is the single call site to prevent duplicate alerts; we | ||
| // fire-and-forget so the webhook response doesn't await Resend. | ||
| void notifyApiErrorAlert({ |
There was a problem hiding this comment.
notifyApiErrorAlert is invoked unconditionally after updateAiAnalysis, but updateAiAnalysis intentionally supports partial/no-op callbacks (it returns the existing row when no fields are updated). That means a retry or follow-up callback like {} (or one that only updates another AI field) will resend the same high/medium alert for an already-escalated issue, creating duplicate operational emails. Please only trigger notifications when the callback actually transitions severity into high/medium (or otherwise prove this is the first alert for that row).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Fixed in 96da9b6.
The route now reads the row via getApiErrorById before updateAiAnalysis and gates the notifier on a new helper severityBecameNotifiable(prev, next) which returns true only when prev was NOT in {high, medium} AND next IS. This covers the cases you raised:
- Empty body / partial callback (e.g. only
ai_summary): post severity equals pre severity → no notify. - Idempotent retry (
high→high): no transition → no notify. - Lateral move (
medium→high): both notifiable → no notify (chosen so each row produces at most one email; medium→high escalation is intentionally collapsed into the original alert). - Downgrade (
high→low): post is not notifiable → no notify. - First-sight escalation (
unknown/low→high/medium): exactly one notify.
Tests added in githubAiCallback.test.ts cover all four no-notify paths plus the positive transition.
Generated by Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/api/src/services/notifier.ts (1)
98-102: ⚡ Quick winValidate
ADMIN_BASE_URLscheme before rendering links.
normalizeBaseUrlcurrently trims only; a misconfigured non-HTTP(S) value can still be embedded into alert HTML. Consider parsing withURLand allowing onlyhttps:(and optionallyhttp:for non-prod), otherwise omit the link.Suggested hardening diff
function normalizeBaseUrl(raw: string | undefined): string | null { if (!raw) return null; - const trimmed = raw.trim().replace(/\/+$/, ""); - return trimmed.length > 0 ? trimmed : null; + const trimmed = raw.trim().replace(/\/+$/, ""); + if (trimmed.length === 0) return null; + try { + const parsed = new URL(trimmed); + if (parsed.protocol !== "https:" && parsed.protocol !== "http:") return null; + return `${parsed.origin}${parsed.pathname}`.replace(/\/+$/, ""); + } catch { + return null; + } }Also applies to: 120-133
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/api/src/services/notifier.ts` around lines 98 - 102, normalizeBaseUrl currently only trims slashes and can return non-HTTP schemes; update it to parse the raw value with the URL constructor inside a try/catch, allow only "https:" (and allow "http:" conditionally when NOT in production), and return null for invalid or disallowed schemes so unsafe schemes are never embedded in alert HTML. Use the function name normalizeBaseUrl to locate the code and apply the same scheme-validation fix to the other similar block referenced around lines 120-133.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/api/.env.example`:
- Around line 47-48: Reorder the two environment variables so ADMIN_BASE_URL
appears before MONITORING_NOTIFY_EMAIL in the .env example to satisfy
dotenv-linter: update the file so the key ADMIN_BASE_URL is placed above
MONITORING_NOTIFY_EMAIL (no other changes), keeping existing formatting and
values intact.
---
Nitpick comments:
In `@server/api/src/services/notifier.ts`:
- Around line 98-102: normalizeBaseUrl currently only trims slashes and can
return non-HTTP schemes; update it to parse the raw value with the URL
constructor inside a try/catch, allow only "https:" (and allow "http:"
conditionally when NOT in production), and return null for invalid or disallowed
schemes so unsafe schemes are never embedded in alert HTML. Use the function
name normalizeBaseUrl to locate the code and apply the same scheme-validation
fix to the other similar block referenced around lines 120-133.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0961b0c6-81a5-466b-b6d6-ad2c64fef7d3
📒 Files selected for processing (5)
server/api/.env.exampleserver/api/src/__tests__/routes/webhooks/githubAiCallback.test.tsserver/api/src/routes/webhooks/githubAiCallback.tsserver/api/src/services/notifier.test.tsserver/api/src/services/notifier.ts
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements an email notification system for high-impact API errors. It introduces a new notifier service that sends alerts for high or medium severity errors and integrates it into the GitHub AI callback webhook. Feedback recommends using the getOptionalEnv helper for environment variable access to ensure consistency and sanitizing the email subject line to mitigate header injection risks.
| * @see https://github.com/otomatty/zedi/issues/809 | ||
| */ | ||
| import type { ApiErrorSeverity } from "../schema/apiErrors.js"; | ||
| import { sendEmail } from "./emailService.js"; |
There was a problem hiding this comment.
For consistency with other services in this repository (like emailService.ts), please use the getOptionalEnv helper instead of accessing process.env directly.
| import { sendEmail } from "./emailService.js"; | |
| import { getOptionalEnv } from "../lib/env.js"; | |
| import { sendEmail } from "./emailService.js"; |
There was a problem hiding this comment.
Adopted in 96da9b6. notifier.ts now imports getOptionalEnv from ../lib/env.js and uses it for both ADMIN_BASE_URL and MONITORING_NOTIFY_EMAIL, matching the pattern in emailService.ts.
Generated by Claude Code
| * renders. | ||
| */ | ||
| function buildEmail(payload: NotifyApiErrorAlertPayload): { subject: string; html: string } { | ||
| const adminBase = normalizeBaseUrl(process.env.ADMIN_BASE_URL); |
There was a problem hiding this comment.
Use the getOptionalEnv helper for environment variable access to maintain consistency across the codebase.
| const adminBase = normalizeBaseUrl(process.env.ADMIN_BASE_URL); | |
| const adminBase = normalizeBaseUrl(getOptionalEnv("ADMIN_BASE_URL")); |
There was a problem hiding this comment.
Applied in 96da9b6:
const adminBase = normalizeBaseUrl(getOptionalEnv("ADMIN_BASE_URL"));The signature of normalizeBaseUrl was simplified to take string (not string | undefined) since getOptionalEnv always returns a string ("" when unset), and an empty input now naturally falls through the early-return path. While here, also added http:/https: scheme validation (per CodeRabbit's nearby suggestion) so a misconfigured javascript: / data: URL can't ever be embedded as a clickable link in alert HTML.
Generated by Claude Code
| const adminBase = normalizeBaseUrl(process.env.ADMIN_BASE_URL); | ||
| const adminUrl = adminBase ? `${adminBase}/errors/${payload.apiErrorId}` : null; | ||
|
|
||
| const subject = `[zedi:${payload.severity}] API error ${payload.sentryIssueId}`; |
There was a problem hiding this comment.
Sanitize the email subject line by removing or replacing newline characters (\r, \n). While sentryIssueId is expected to be a safe identifier, it's a security best practice to prevent potential header injection vulnerabilities when including external data in email headers.
| const subject = `[zedi:${payload.severity}] API error ${payload.sentryIssueId}`; | |
| const subject = `[zedi:${payload.severity}] API error ${payload.sentryIssueId}`.replace(/[\r\n]/g, " "); |
There was a problem hiding this comment.
Applied in 96da9b6 with a slightly tighter regex (/[\r\n]+/g) so consecutive CR/LF runs collapse into a single space:
const subject = `[zedi:${payload.severity}] API error ${payload.sentryIssueId}`.replace(
/[\r\n]+/g,
" ",
);Resend takes the subject as a JSON field today so this isn't currently exploitable, but I agree this is cheap defense-in-depth for any future SMTP transport. Added a test (件名から CR/LF を取り除く) that asserts no CR/LF survives even when a malformed sentryIssueId is passed in.
Generated by Claude Code
| return { email: { sent: false } }; | ||
| } | ||
|
|
||
| const to = process.env.MONITORING_NOTIFY_EMAIL?.trim(); |
There was a problem hiding this comment.
Use the getOptionalEnv helper for environment variable access to maintain consistency across the codebase.
| const to = process.env.MONITORING_NOTIFY_EMAIL?.trim(); | |
| const to = getOptionalEnv("MONITORING_NOTIFY_EMAIL")?.trim(); |
There was a problem hiding this comment.
Applied in 96da9b6:
const to = getOptionalEnv("MONITORING_NOTIFY_EMAIL").trim();Note: getOptionalEnv returns "" (not undefined) when the var is unset, so optional chaining (?.trim()) isn't needed — "".trim() is "" and the subsequent if (!to) short-circuits to a no-op as before.
Generated by Claude Code
- gate `notifyApiErrorAlert` on severity transitions so retries / partial callbacks no longer re-send (Codex P1). Route now reads the row before `updateAiAnalysis` and only fires when severity transitions from a non-notifiable value into `high`/`medium`. - validate `ADMIN_BASE_URL` scheme (`http:` / `https:` only) and drop malformed values to prevent dangerous URLs in alert HTML (CodeRabbit). - reorder `.env.example` so `ADMIN_BASE_URL` precedes `MONITORING_NOTIFY_EMAIL` to satisfy dotenv-linter (CodeRabbit). - adopt `getOptionalEnv` for env access for parity with `emailService.ts` and strip CR/LF from email subject as defense-in-depth against header injection (gemini-code-assist). Tests cover: first-sight escalation fires once; retry (high→high), partial callback (no severity), and downgrade do NOT fire; non-HTTP schemes never produce an `href`; CR/LF never survives in the subject.
2 participants
Epic #616 Phase 3 / sub-issue #809。重要 API エラー (severity=high/medium) の
メール通知サービス
services/notifier.tsを新設し、AI 解析コールバック(
PUT /api/webhooks/github/ai-result/:id) の成功時に 1 回だけ呼ぶよう配線する。Slack はこのリポジトリでは使わないため非対応。
MONITORING_NOTIFY_EMAIL未設定 / severity が low/unknown の場合は no-op。本文には sentry_issue_id /
severity / title / 管理画面 URL のみ載せ、Authorization / Cookie / 内部 URL
など PII は混入させない。
Phase 3 / #809: introduce
notifier.tsfor email-only alerts onhigh/medium severity API errors. Slack is intentionally unsupported.
The notifier no-ops when
MONITORING_NOTIFY_EMAILis unset or severityis low/unknown. The AI callback is the single call site so duplicate
alerts can't happen, and sends are fire-and-forget so the webhook
response isn't delayed by Resend.
Summary by CodeRabbit
New Features
Chores
Tests