feat: Add Sentry webhook receiver and admin error management API - #812
Conversation
- POST /api/webhooks/sentry with HMAC-SHA256 signature verification using SENTRY_WEBHOOK_SECRET; payload is normalized via extractSentrySummary and upserted via apiErrorService.upsertFromSentrySummary. - /api/admin/errors GET list (paginated + status/severity filters), GET :id, PATCH :id status (delegates to apiErrorService updateApiErrorStatus, maps ApiErrorStatusConflictError to 409, invalid transitions to 400). - Mount new routes in app.ts and admin/index.ts; add SENTRY_WEBHOOK_SECRET to server/api/.env.example. - Vitest coverage: signature OK / NG / missing / unconfigured secret, ignored payloads (no sentry_issue_id), invalid JSON, plus admin route auth/role and PATCH transition / 404 / 409 cases. Refs Epic #616 (Phase 1).
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughImplements Phase 1 of Epic ChangesSentry Webhook & Admin Error Management
Sequence DiagramsequenceDiagram
participant Sentry as Sentry (SaaS)
participant Webhook as POST /api/webhooks/sentry
participant Verify as Signature Verification
participant Extract as extractSentrySummary()
participant DB as api_errors Table
participant Admin as GET /admin/errors
Sentry->>Webhook: POST with raw body + sentry-hook-signature header
Webhook->>Verify: Verify HMAC-SHA256 signature
alt Invalid Signature
Verify-->>Sentry: 403 Forbidden
else Valid Signature
Webhook->>Webhook: Parse JSON payload
alt Invalid JSON
Webhook-->>Sentry: 400 Bad Request
else Valid JSON
Webhook->>Extract: extractSentrySummary(payload)
alt Cannot extract sentryIssueId
Extract-->>Webhook: null (payload ignored)
Webhook-->>Sentry: 200 { received: true, ignored: true }
else Successfully extracted
Extract-->>Webhook: SentrySummaryExtraction
Webhook->>DB: upsertFromSentrySummary(db, summary)
DB-->>Webhook: { id, ... }
Webhook-->>Sentry: 200 { received: true, id: ... }
end
end
end
Admin->>Webhook: (Later) GET /admin/errors?limit=10&offset=0
Webhook->>DB: listApiErrors(filters, limit, offset)
DB-->>Webhook: { errors[], total, limit, offset }
Webhook-->>Admin: 200 JSON response
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly Related Issues
Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 0/5 reviews remaining, refill in 3 minutes and 35 seconds. Comment |
![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 introduces a Sentry webhook receiver and administrative endpoints for managing API errors. Key additions include HMAC-SHA256 signature verification for Sentry webhooks, payload normalization, and a set of admin routes for listing, viewing, and updating error statuses. Feedback focuses on improving robustness by validating UUID formats for route parameters, handling potential null values in request bodies, and replacing fragile regex-based error handling with dedicated custom error classes.
| const id = c.req.param("id"); | ||
| const db = c.get("db"); |
There was a problem hiding this comment.
id パラメータが UUID 形式であることを検証していません。不正な形式の文字列が渡されると、Postgres がエラーを投げ、結果として 500 Internal Server Error が返される可能性があります。事前に形式をチェックし、404 を返すのが適切です。
const id = c.req.param("id");
if (!/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(id)) {
return c.json({ error: "Not found" }, 404);
}
const db = c.get("db");| const id = c.req.param("id"); | ||
| const db = c.get("db"); |
There was a problem hiding this comment.
PATCH エンドポイントでも同様に、id が UUID 形式であることを検証することを推奨します。不正な形式の場合に 500 エラーになるのを防ぐことができます。
const id = c.req.param("id");
if (!/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(id)) {
return c.json({ error: "Not found" }, 404);
}
const db = c.get("db");| return c.json({ error: "invalid JSON body" }, 400); | ||
| } | ||
|
|
||
| if (typeof body.status !== "string") { |
There was a problem hiding this comment.
c.req.json() が null を返す可能性があるため（リクエストボディが 'null' の場合など）、body.status の参照で TypeError が発生するリスクがあります。body が存在することを確認してからプロパティにアクセスするようにしてください。
| if (typeof body.status !== "string") { | |
| if (!body || typeof body.status !== "string") { |
| if (err instanceof Error && /invalid api_errors status transition/i.test(err.message)) { | ||
| return c.json({ error: err.message }, 400); | ||
| } |
There was a problem hiding this comment.
サービス層からのエラーメッセージを正規表現で判定するのは保守性が低く脆弱です。メッセージ内容が変更されると判定が機能しなくなります。ApiErrorInvalidTransitionError などのカスタムエラークラスを定義し、instanceof で判定することを検討してください。
| /required|must be|less than or equal/i.test(err.message) && | ||
| !/no rows/i.test(err.message) | ||
| ) { |
There was a problem hiding this comment.
この正規表現によるエラー判定は脆弱です。apiErrorService.ts 側のバリデーションロジックやメッセージが変更されると、意図せず 500 エラーとして扱われる可能性があります。専用のカスタムエラークラス（例: ApiErrorValidationError）を導入して判定することを推奨します。
![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: 2f10a525aa
ℹ️ 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".
| asString(issue?.id) ?? | ||
| asString(event?.issue_id) ?? | ||
| asString(eventIssue?.id) ?? | ||
| asString(group?.id) ?? | ||
| asString(data.id) |
There was a problem hiding this comment.
data.error.id when extracting Sentry issue IDs
extractSentrySummary claims to support data.error payloads (and already reads error.title), but extractSentryIssueId never checks data.error.id. For webhook events where the identifier exists only under data.error, summary becomes null, the handler returns 200 {ignored:true}, and no api_errors row is created, so those errors are silently dropped from admin error tracking.
Useful? React with 👍 / 👎.
…ction (#803) Address review feedback on PR #812: - apiErrorService: introduce ApiErrorValidationError and ApiErrorInvalidTransitionError so route handlers can branch on `instanceof` instead of regex-matching error messages. - routes/webhooks/sentry: replace regex-based 400 detection with `instanceof ApiErrorValidationError`, and extend extractSentryIssueId to also read `data.error.id` so payloads where the identifier only lives under `data.error` are no longer silently dropped (Codex P1). - routes/admin/errors: validate `:id` matches the UUID pattern up front (return 404 instead of letting Postgres surface a 500), guard against `c.req.json()` returning literal `null`, and use the typed transition error for 400 mapping. - Tests: cover invalid-UUID 404, JSON-null body 400, and the data.error.id extraction path.
![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 (2)
server/api/src/routes/admin/errors.ts (2)
164-165: ⚡ Quick winReplace message-regex error mapping with typed error handling.
Line 164-Line 165 depends on matching text in
err.message; wording changes would unexpectedly convert a client 400 into a 500. Prefer a dedicated typed error fromapiErrorServiceandinstanceofhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/src/routes/admin/errors.ts` around lines 164 - 165, Replace the fragile regex message check with a proper typed error from apiErrorService: import the dedicated error class (e.g., InvalidStatusTransitionError or the actual class exported by apiErrorService), and change the condition in the error handler that currently tests err.message to use "err instanceof <TypedErrorClass>" so you can return c.json({ error: err.message }, 400) only for that type; leave other error branches unchanged so non-matching errors still propagate as before.
28-28: ⚡ Quick winCentralize
ApiErrorStatusliterals to avoid enum drift.Line 28 duplicates workflow statuses already defined in service transition rules. If the source-of-truth changes, this parser can become stale and reject valid values.
♻️ Proposed refactor
import { + ALLOWED_API_ERROR_STATUS_TRANSITIONS, ApiErrorStatusConflictError, API_ERROR_LIST_DEFAULT_LIMIT, API_ERROR_LIST_MAX_LIMIT, getApiErrorById, listApiErrors, updateApiErrorStatus, } from "../../services/apiErrorService.js"; -const VALID_STATUSES = ["open", "investigating", "resolved", "ignored"] as const; +const VALID_STATUSES = Object.keys( + ALLOWED_API_ERROR_STATUS_TRANSITIONS, +) as readonly ApiErrorStatus[];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/src/routes/admin/errors.ts` at line 28, VALID_STATUSES duplicates the canonical workflow statuses and risks drifting; replace the hardcoded list with the single source-of-truth (the ApiErrorStatus definition used by the service transition rules) by importing that symbol and deriving VALID_STATUSES from it (e.g., Object.values(ApiErrorStatus) or the exported union type), then update any parsing/validation logic that references VALID_STATUSES to use the imported ApiErrorStatus so future changes remain centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/api/src/routes/webhooks/sentry.ts`:
- Around line 197-205: Reorder and sanitize route selection so we prefer non-PII
transaction names first: check tags.transaction, then issue.metadata.transaction
(via metadata.transaction), then issue.shortId; only if those are missing fall
back to event.request.url but strip origin, query string and fragment (use only
the pathname) before returning, and still prepend request method
(request.method) if present; update the logic around request/request.url,
tags.transaction, metadata.transaction, and issue.shortId to reflect this
preference and sanitize the URL fallback.
---
Nitpick comments:
In `@server/api/src/routes/admin/errors.ts`:
- Around line 164-165: Replace the fragile regex message check with a proper
typed error from apiErrorService: import the dedicated error class (e.g.,
InvalidStatusTransitionError or the actual class exported by apiErrorService),
and change the condition in the error handler that currently tests err.message
to use "err instanceof <TypedErrorClass>" so you can return c.json({ error:
err.message }, 400) only for that type; leave other error branches unchanged so
non-matching errors still propagate as before.
- Line 28: VALID_STATUSES duplicates the canonical workflow statuses and risks
drifting; replace the hardcoded list with the single source-of-truth (the
ApiErrorStatus definition used by the service transition rules) by importing
that symbol and deriving VALID_STATUSES from it (e.g.,
Object.values(ApiErrorStatus) or the exported union type), then update any
parsing/validation logic that references VALID_STATUSES to use the imported
ApiErrorStatus so future changes remain centralized.
🪄 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: 86a66ecf-7852-4414-9e41-fbfaa90a6171
📒 Files selected for processing (7)
server/api/.env.exampleserver/api/src/__tests__/routes/admin/errors.test.tsserver/api/src/__tests__/routes/webhooks/sentry.test.tsserver/api/src/app.tsserver/api/src/routes/admin/errors.tsserver/api/src/routes/admin/index.tsserver/api/src/routes/webhooks/sentry.ts
…803) CodeRabbit review feedback (PR #812): - routes/webhooks/sentry: reorder extractRoute to prefer the scrubbed `transaction` tag, then issue.metadata.transaction, then issue.shortId, before falling back to request.url. URL fallback now reduces the value to its pathname (origin / query / fragment dropped) so capability tokens and absolute hosts never persist into api_errors.route. Adds tests for the new ordering and the URL-stripping behavior. - routes/admin/errors: derive VALID_STATUSES from ALLOWED_API_ERROR_STATUS_TRANSITIONS so adding a new ApiErrorStatus automatically expands the accepted PATCH inputs without requiring a manual edit on this list.
2 participants
概要
Sentry Internal Integration webhook 受信機能と、管理画面用の API エラー管理 API を実装します。これにより、Sentry から送信されるエラーイベントを自動的に取り込み、管理画面でエラーの一覧表示・詳細確認・ステータス管理ができるようになります。
変更点
Sentry webhook 受信エンドポイント (
POST /api/webhooks/sentry)Sentry-Hook-Signatureヘッダ）apiErrorService.upsertFromSentrySummaryへの委譲管理 API エラー一覧・詳細・更新エンドポイント
GET /api/admin/errors— ページネーション + status/severity フィルタGET /api/admin/errors/:id— 詳細取得PATCH /api/admin/errors/:id— ワークフロー状態更新（open → investigating → resolved / ignored）テスト
設定
.env.exampleにSENTRY_WEBHOOK_SECRETを追加app.tsに webhook ルートをマウントroutes/admin/index.tsに errors ルートをマウント変更の種類
テスト方法
npm testで全テストが通ることを確認sentry.test.ts: 署名検証、ペイロード抽出、エラーハンドリングerrors.test.ts: 一覧・詳細・更新エンドポイント、認可、並行更新検知環境変数
SENTRY_WEBHOOK_SECRETを設定して、Sentry から webhook を送信管理画面で
/api/admin/errorsにアクセスチェックリスト
関連 Issue
Closes #616, #803
https://claude.ai/code/session_01Gyem2NDUc6JT4kZaxYU6Zo
Summary by CodeRabbit