feat(emails): log every POST /api/emails attempt (email_send_log, 7-day observability)#731
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds best-effort logging of email send attempts into a new ChangesEmail attempt logging
Sequence Diagram(s)sequenceDiagram
participant Client
participant sendEmailHandler
participant validateSendEmailBody
participant processAndSendEmail
participant logEmailAttempt
participant insertEmailSendLog
Client->>sendEmailHandler: HTTP request
sendEmailHandler->>sendEmailHandler: readRawBody
sendEmailHandler->>validateSendEmailBody: validate body
alt validation rejected
validateSendEmailBody-->>sendEmailHandler: NextResponse
sendEmailHandler->>logEmailAttempt: log rejected (rawBody)
logEmailAttempt->>insertEmailSendLog: insert row
sendEmailHandler-->>Client: validation response
else validation passed
sendEmailHandler->>processAndSendEmail: send email
alt send failed
processAndSendEmail-->>sendEmailHandler: success false
sendEmailHandler->>logEmailAttempt: log send_failed
logEmailAttempt->>insertEmailSendLog: insert row
sendEmailHandler-->>Client: 502 error
else send succeeded
processAndSendEmail-->>sendEmailHandler: result.id
sendEmailHandler->>logEmailAttempt: log sent (resendId)
logEmailAttempt->>insertEmailSendLog: insert row
sendEmailHandler-->>Client: 200 success
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Confidence score: 2/5
- In
supabase/migrations/20260630_email_send_log.sql, the newemail_send_logtable stores raw email request bodies but does not enable RLS, which creates a real data-exposure risk through the Supabase API if public-schema access exists; merging as-is could leak sensitive request content — enable RLS (and corresponding policies) before merging. - In
supabase/migrations/20260630_email_send_log.sql,statusis only documented in a comment today, so invalid values can still be written and downstream logic/reporting can drift or fail unexpectedly — add aCHECKconstraint for the allowed status set before merging.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="supabase/migrations/20260630_email_send_log.sql">
<violation number="1" location="supabase/migrations/20260630_email_send_log.sql:4">
P1: Enable RLS on this new public table before storing raw email request bodies. Without it, the log table can expose captured request content through the Supabase API if public-schema privileges are available.</violation>
<violation number="2" location="supabase/migrations/20260630_email_send_log.sql:9">
P2: Add a CHECK constraint for the documented status values. The comment alone does not protect email_send_log from invalid statuses.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| create table if not exists public.email_send_log ( | ||
| id uuid primary key default gen_random_uuid(), | ||
| created_at timestamptz not null default now(), | ||
| account_id uuid, | ||
| chat_id text, | ||
| status text not null, -- 'sent' | 'send_failed' | 'rejected' | ||
| body_parsed boolean not null default false, | ||
| raw_body text, -- truncated copy of the request body | ||
| to_count integer, | ||
| subject text, | ||
| html_length integer, | ||
| text_length integer, | ||
| resend_id text, | ||
| error text | ||
| ); |
There was a problem hiding this comment.
P1: Enable RLS on this new public table before storing raw email request bodies. Without it, the log table can expose captured request content through the Supabase API if public-schema privileges are available.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At supabase/migrations/20260630_email_send_log.sql, line 4:
<comment>Enable RLS on this new public table before storing raw email request bodies. Without it, the log table can expose captured request content through the Supabase API if public-schema privileges are available.</comment>
<file context>
@@ -0,0 +1,20 @@
+-- Durable log of every POST /api/emails attempt (success, send-failure, and
+-- rejected/malformed), retained for debugging which params the API received
+-- up to several days back — the sandbox that built the request is ephemeral.
+create table if not exists public.email_send_log (
+ id uuid primary key default gen_random_uuid(),
+ created_at timestamptz not null default now(),
</file context>
| create table if not exists public.email_send_log ( | |
| id uuid primary key default gen_random_uuid(), | |
| created_at timestamptz not null default now(), | |
| account_id uuid, | |
| chat_id text, | |
| status text not null, -- 'sent' | 'send_failed' | 'rejected' | |
| body_parsed boolean not null default false, | |
| raw_body text, -- truncated copy of the request body | |
| to_count integer, | |
| subject text, | |
| html_length integer, | |
| text_length integer, | |
| resend_id text, | |
| error text | |
| ); | |
| create table if not exists public.email_send_log ( | |
| id uuid primary key default gen_random_uuid(), | |
| created_at timestamptz not null default now(), | |
| account_id uuid, | |
| chat_id text, | |
| status text not null, -- 'sent' | 'send_failed' | 'rejected' | |
| body_parsed boolean not null default false, | |
| raw_body text, -- truncated copy of the request body | |
| to_count integer, | |
| subject text, | |
| html_length integer, | |
| text_length integer, | |
| resend_id text, | |
| error text | |
| ); | |
| alter table public.email_send_log enable row level security; |
| created_at timestamptz not null default now(), | ||
| account_id uuid, | ||
| chat_id text, | ||
| status text not null, -- 'sent' | 'send_failed' | 'rejected' |
There was a problem hiding this comment.
P2: Add a CHECK constraint for the documented status values. The comment alone does not protect email_send_log from invalid statuses.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At supabase/migrations/20260630_email_send_log.sql, line 9:
<comment>Add a CHECK constraint for the documented status values. The comment alone does not protect email_send_log from invalid statuses.</comment>
<file context>
@@ -0,0 +1,20 @@
+ created_at timestamptz not null default now(),
+ account_id uuid,
+ chat_id text,
+ status text not null, -- 'sent' | 'send_failed' | 'rejected'
+ body_parsed boolean not null default false,
+ raw_body text, -- truncated copy of the request body
</file context>
| status text not null, -- 'sent' | 'send_failed' | 'rejected' | |
| status text not null check (status in ('sent', 'send_failed', 'rejected')), -- 'sent' | 'send_failed' | 'rejected' |
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Auto-approved: Adds a new database table and best-effort logging for email send attempts, plus tests. No existing logic is changed; the risk of breakage is very low.
Re-trigger cubic
…-day observability) When an empty/footer-only email went out we could not recover what the API received: safeParseJson discards malformed bodies, the route logged nothing, and the sandbox that built the request is ephemeral. This records every attempt — sent, send_failed, and rejected (empty/malformed) — so a send is debuggable days later. - lib/supabase/email_send_log/insertEmailSendLog.ts: typed insert. - lib/emails/logEmailAttempt.ts: builds the row (computes body_parsed, truncates raw_body to 10k); best-effort — never throws, so logging can't break a send. - sendEmailHandler: capture the raw body via request.clone() and log on every outcome (rejected attempts keep the raw body, so a malformed send is visible). - types/database.types.ts: email_send_log added so the typed insert compiles. The migration lives in the recoupable/database repo (mono/database) — applied via its CI — not here. Tests: logEmailAttempt unit (parsed/malformed/truncation/swallow-errors) + handler asserts the sent/rejected/send_failed log calls. Full lib/emails: 150. Ref: recoupable/chat#1829 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sweetmantech
force-pushed
the
feat/email-send-log
branch
from
046b57e to
7260809
Compare
…mn schema database#37 + #38 shipped email_send_log as 7 columns (id, created_at, account_id, chat_id, status, resend_id, raw_body) with RLS. Regenerate types/database.types.ts from the live schema (replaces the earlier hand-edit; also picks up unrelated drift) and trim the writer to match: - logEmailAttempt: insert only account_id/chat_id/status/resend_id/raw_body; drop body_parsed, error, subject, to_count, html_length, text_length; store the FULL raw_body (no truncation). - sendEmailHandler: drop the removed fields from the send_failed/sent log calls. - tests updated accordingly. Full lib/emails suite: 149 passing; tsc + lint clean. Ref: recoupable/chat#1829 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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 (5)
lib/emails/logEmailAttempt.ts (1)
30-30: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
||instead of??collapses a legitimately empty body intonull.
attempt.rawBody || nulltreats an empty string the same asundefined/missing, converting a genuinely empty request body intonull. Given the PR's explicit goal of preserving "malformed or empty requests whose original payload would otherwise be lost," using??would more faithfully distinguish "body was empty" (stored as"") from "no body field was ever provided." This is a minor fidelity gap sincereadRawBodyin the handler already collapses read-failures to""too, so the distinction is already partially lost upstream.🤖 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 `@lib/emails/logEmailAttempt.ts` at line 30, The `logEmailAttempt` mapping currently uses `attempt.rawBody || null`, which turns an intentional empty string into `null`. Update the `raw_body` assignment in `logEmailAttempt` to use nullish coalescing so `""` is preserved while only `undefined`/missing values become `null`, keeping the body fidelity consistent with the request handling path.lib/supabase/email_send_log/insertEmailSendLog.ts (2)
13-16: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
error: unknowndiscards a known, typed error shape.Supabase's insert/update/select calls return a
PostgrestError | null, notunknown. Typing the return asunknownforces every caller to cast/narrow before they can inspecterror.message/error.code, and the path instruction for this directory calls for returning typed results.✏️ Suggested fix
+import type { PostgrestError } from "`@supabase/supabase-js`"; + export async function insertEmailSendLog( row: Database["public"]["Tables"]["email_send_log"]["Insert"], -): Promise<{ error: unknown }> { +): Promise<{ error: PostgrestError | null }> { const { error } = await supabase.from("email_send_log").insert(row); return { error }; }🤖 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 `@lib/supabase/email_send_log/insertEmailSendLog.ts` around lines 13 - 16, The return type in insertEmailSendLog is too generic because it uses unknown for the Supabase error, which prevents callers from safely inspecting error details. Update the insertEmailSendLog function to return the typed Supabase/Postgrest error shape used by supabase.from(...).insert(...), and keep the destructured error value aligned with that type so callers can access fields like message and code without casting.Source: Path instructions
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRelative import instead of the documented alias path.
As per path instructions, "Import serverClient only within lib/supabase/" — and the coding guidelines for this directory specifically call out importing
serverClientvia@/lib/supabase/serverClient. This file uses a relative import (../serverClient) instead.✏️ Suggested fix
-import supabase from "../serverClient"; +import supabase from "`@/lib/supabase/serverClient`";🤖 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 `@lib/supabase/email_send_log/insertEmailSendLog.ts` at line 1, The import in insertEmailSendLog should use the documented supabase alias path instead of a relative path. Update the serverClient import in this module to reference `@/lib/supabase/serverClient` so it matches the directory guidance and keeps imports consistent with other lib/supabase code.Source: Coding guidelines
lib/emails/sendEmailHandler.ts (2)
38-38: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBest-effort logging blocks every response path with no timeout.
All three
await logEmailAttempt(...)calls sit directly on the response path (rejected, send_failed, sent) before the handler returns. SincelogEmailAttempt/insertEmailSendLogimpose no timeout on the underlying Supabase call, a slow or unresponsive database adds latency — or, in a worse-case Supabase outage, indefinitely hangs — every singlePOST /api/emailsrequest, even though this write is explicitly "best-effort" and shouldn't gate the response.Next.js (16.0.10, per this file's library context) has a stable
after()API fromnext/serverspecifically intended for "tasks and other side effects that should not block the response, such as logging and analytics." Moving these three calls intoafter(() => logEmailAttempt(...))would let the response return immediately while the log write happens post-response.♻️ Example refactor (rejection path shown; same pattern applies to the other two call sites)
+import { after } from "next/server"; + if (validated instanceof NextResponse) { - await logEmailAttempt({ rawBody, status: "rejected" }); + after(() => logEmailAttempt({ rawBody, status: "rejected" })); return validated; }Also applies to: 55-60, 67-73
🤖 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 `@lib/emails/sendEmailHandler.ts` at line 38, The three best-effort email logging calls are still blocking the `POST /api/emails` response path because `sendEmailHandler` awaits `logEmailAttempt` directly. Update `sendEmailHandler` to schedule the `logEmailAttempt(...)` work inside `after()` from `next/server` for the rejected, send_failed, and sent branches so the handler can return immediately while `logEmailAttempt`/`insertEmailSendLog` runs post-response without gating success.
33-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
sendEmailHandlernow exceeds the 20-line function-length guideline.The function spans roughly lines 33-79 (~47 lines), over the documented "flag functions longer than 20 lines" guideline (still under the 50-line path-instruction cap for
lib/**/*.ts). The new logging calls compound an already large handler. Consider extracting a smalllogOutcome(status, extra)wrapper to shrink the inline blocks at lines 38, 55-60, and 67-73.🤖 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 `@lib/emails/sendEmailHandler.ts` around lines 33 - 79, sendEmailHandler is now too long and should be shortened to fit the 20-line guideline. Extract the repeated email-attempt logging and response handling into small helpers, such as a logOutcome wrapper around logEmailAttempt and a separate failure/success response helper, so the main flow in sendEmailHandler stays compact. Keep the core orchestration in sendEmailHandler and move the inline blocks around validated checks and processAndSendEmail result handling into named functions to reduce the function size.Source: Coding guidelines
🤖 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 `@lib/emails/logEmailAttempt.ts`:
- Around line 23-35: logEmailAttempt is ignoring the { error } result from
insertEmailSendLog, so failed email_send_log writes are never visible. Update
logEmailAttempt to inspect the return value from insertEmailSendLog and emit the
error (for example via console.error or the project logger) when present, while
still keeping the function best-effort and non-throwing. The try/catch around
logEmailAttempt should remain only as a safeguard for unexpected throws.
---
Nitpick comments:
In `@lib/emails/logEmailAttempt.ts`:
- Line 30: The `logEmailAttempt` mapping currently uses `attempt.rawBody ||
null`, which turns an intentional empty string into `null`. Update the
`raw_body` assignment in `logEmailAttempt` to use nullish coalescing so `""` is
preserved while only `undefined`/missing values become `null`, keeping the body
fidelity consistent with the request handling path.
In `@lib/emails/sendEmailHandler.ts`:
- Line 38: The three best-effort email logging calls are still blocking the
`POST /api/emails` response path because `sendEmailHandler` awaits
`logEmailAttempt` directly. Update `sendEmailHandler` to schedule the
`logEmailAttempt(...)` work inside `after()` from `next/server` for the
rejected, send_failed, and sent branches so the handler can return immediately
while `logEmailAttempt`/`insertEmailSendLog` runs post-response without gating
success.
- Around line 33-79: sendEmailHandler is now too long and should be shortened to
fit the 20-line guideline. Extract the repeated email-attempt logging and
response handling into small helpers, such as a logOutcome wrapper around
logEmailAttempt and a separate failure/success response helper, so the main flow
in sendEmailHandler stays compact. Keep the core orchestration in
sendEmailHandler and move the inline blocks around validated checks and
processAndSendEmail result handling into named functions to reduce the function
size.
In `@lib/supabase/email_send_log/insertEmailSendLog.ts`:
- Around line 13-16: The return type in insertEmailSendLog is too generic
because it uses unknown for the Supabase error, which prevents callers from
safely inspecting error details. Update the insertEmailSendLog function to
return the typed Supabase/Postgrest error shape used by
supabase.from(...).insert(...), and keep the destructured error value aligned
with that type so callers can access fields like message and code without
casting.
- Line 1: The import in insertEmailSendLog should use the documented supabase
alias path instead of a relative path. Update the serverClient import in this
module to reference `@/lib/supabase/serverClient` so it matches the directory
guidance and keeps imports consistent with other lib/supabase code.
🪄 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: fe795af5-0a40-4365-9eba-e15fb8ace42c
⛔ Files ignored due to path filters (3)
lib/emails/__tests__/logEmailAttempt.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/emails/__tests__/sendEmailHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**types/database.types.tsis excluded by none and included by none
📒 Files selected for processing (3)
lib/emails/logEmailAttempt.tslib/emails/sendEmailHandler.tslib/supabase/email_send_log/insertEmailSendLog.ts
| * @returns A NextResponse with the send result. | ||
| */ | ||
| export async function sendEmailHandler(request: NextRequest): Promise<NextResponse> { | ||
| const rawBody = await readRawBody(request); |
There was a problem hiding this comment.
SRP
- actual: readRawBody called in handler
- required: readRawBody called in validateSendEmailBody
There was a problem hiding this comment.
Fixed in 48d01b4. readRawBody now lives in validateSendEmailBody — it reads the request body once (as text), parses it for schema validation, and returns rawBody on both the rejected and accepted paths ({ rawBody, error } | { rawBody, data }). The handler no longer touches the request; it just consumes validated.rawBody. (This also drops the separate safeParseJson read — one body read total.)
|
|
||
| await logEmailAttempt({ | ||
| rawBody, | ||
| status: "sent", | ||
| accountId: validated.accountId, | ||
| chatId: chat_id, | ||
| resendId: result.id, | ||
| }); |
There was a problem hiding this comment.
DRY
- actual: logEmailAttempt called twice in the handler
- required: logEmailAttempt called once in the handler for both cases handled in one call
There was a problem hiding this comment.
Fixed in 48d01b4. The handler now resolves a single { response, attempt } outcome — deliver() returns the sent/send_failed attempt, and the rejected branch yields { status: "rejected" } — then calls logEmailAttempt({ rawBody, ...attempt }) exactly once for every path. Locked in with expect(mockLogEmailAttempt).toHaveBeenCalledTimes(1).
✅ Preview verification — 2026-06-30Tested against the Vercel preview built from Goal: every
Confirmed
Notes
Schema matches the merged table (recoupable/database #37 + #38): |
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="supabase/migrations/20260630_email_send_log.sql">
<violation number="1" location="supabase/migrations/20260630_email_send_log.sql:4">
P1: Enable RLS on this new public table before storing raw email request bodies. Without it, the log table can expose captured request content through the Supabase API if public-schema privileges are available.</violation>
<violation number="2" location="supabase/migrations/20260630_email_send_log.sql:9">
P2: Add a CHECK constraint for the documented status values. The comment alone does not protect email_send_log from invalid statuses.</violation>
</file>
<file name="lib/emails/logEmailAttempt.ts">
<violation number="1" location="lib/emails/logEmailAttempt.ts:30">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
The PR description claims `logEmailAttempt.ts` truncates `raw_body` to 10k and computes `body_parsed` via `JSON.parse`, but the diff removes both features. The description and the actual code changes are inconsistent.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| chat_id: attempt.chatId ?? null, | ||
| status: attempt.status, | ||
| resend_id: attempt.resendId ?? null, | ||
| raw_body: attempt.rawBody || null, |
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
The PR description claims logEmailAttempt.ts truncates raw_body to 10k and computes body_parsed via JSON.parse, but the diff removes both features. The description and the actual code changes are inconsistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/emails/logEmailAttempt.ts, line 30:
<comment>The PR description claims `logEmailAttempt.ts` truncates `raw_body` to 10k and computes `body_parsed` via `JSON.parse`, but the diff removes both features. The description and the actual code changes are inconsistent.</comment>
<file context>
@@ -1,57 +1,33 @@
- text_length: attempt.text?.length ?? null,
resend_id: attempt.resendId ?? null,
- error: attempt.error ?? null,
+ raw_body: attempt.rawBody || null,
});
} catch {
</file context>
cubic-dev-ai
Bot
dismissed
their stale review
Dismissed because Cubic found issues in a newer review.
Address review on #731: - SRP: validateSendEmailBody now reads the request body once (readRawBody) and returns rawBody on both the rejected and accepted paths, so the handler no longer reads the request itself. - DRY: sendEmailHandler computes a single outcome and calls logEmailAttempt exactly once for every path (sent / send_failed / rejected). - Surface a returned insert error to server logs (observability) and store the raw body verbatim (preserve empty string). - Type insertEmailSendLog's error as PostgrestError | null. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addressed review —
|
There was a problem hiding this comment.
6 issues found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="supabase/migrations/20260630_email_send_log.sql">
<violation number="1" location="supabase/migrations/20260630_email_send_log.sql:4">
P1: Enable RLS on this new public table before storing raw email request bodies. Without it, the log table can expose captured request content through the Supabase API if public-schema privileges are available.</violation>
<violation number="2" location="supabase/migrations/20260630_email_send_log.sql:9">
P2: Add a CHECK constraint for the documented status values. The comment alone does not protect email_send_log from invalid statuses.</violation>
</file>
<file name="lib/emails/logEmailAttempt.ts">
<violation number="1" location="lib/emails/logEmailAttempt.ts:30">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
The PR description claims `logEmailAttempt.ts` truncates `raw_body` to 10k and computes `body_parsed` via `JSON.parse`, but the diff removes both features. The description and the actual code changes are inconsistent.</violation>
</file>
<file name="lib/emails/__tests__/validateSendEmailBody.test.ts">
<violation number="1" location="lib/emails/__tests__/validateSendEmailBody.test.ts:57">
P3: Same: the validation-error test (empty body → 400) asserts shape with `"error" in result` but never checks `rawBody` contents.</violation>
<violation number="2" location="lib/emails/__tests__/validateSendEmailBody.test.ts:86">
P3: Tests assert `"data" in result` / `"error" in result` to check the discriminated-union shape, but never verify the `rawBody` field — the central contract of the return type.</violation>
</file>
<file name="lib/emails/validateSendEmailBody.ts">
<violation number="1" location="lib/emails/validateSendEmailBody.ts:62">
P1: Malformed JSON is converted into a valid empty body, so authenticated bad requests can still reach `deliver()` and send the footer-only/default email this PR is meant to reject. Return a 400 on empty or unparsable raw bodies instead of passing `{}` to Zod.</violation>
</file>
<file name="lib/emails/__tests__/sendEmailHandler.test.ts">
<violation number="1" location="lib/emails/__tests__/sendEmailHandler.test.ts:72">
P3: Rejected and send_failed tests don't assert `toHaveBeenCalledTimes(1)`, so a double-log regression on error paths wouldn't be caught. Add the count assertion for consistency.</violation>
<violation number="2" location="lib/emails/__tests__/sendEmailHandler.test.ts:86">
P2: Rejected and send_failed test assertions omit `rawBody`, so a regression where `rawBody` is silently dropped on error paths would not be caught. Add `rawBody` to both assertions.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| let parsed: unknown = {}; | ||
| if (rawBody) { | ||
| try { | ||
| parsed = JSON.parse(rawBody); | ||
| } catch { | ||
| parsed = {}; | ||
| } | ||
| } |
There was a problem hiding this comment.
P1: Malformed JSON is converted into a valid empty body, so authenticated bad requests can still reach deliver() and send the footer-only/default email this PR is meant to reject. Return a 400 on empty or unparsable raw bodies instead of passing {} to Zod.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/emails/validateSendEmailBody.ts, line 62:
<comment>Malformed JSON is converted into a valid empty body, so authenticated bad requests can still reach `deliver()` and send the footer-only/default email this PR is meant to reject. Return a 400 on empty or unparsable raw bodies instead of passing `{}` to Zod.</comment>
<file context>
@@ -30,61 +29,74 @@ export type ValidatedSendEmailRequest = Omit<SendEmailBody, "to" | "subject"> &
- const result = sendEmailBodySchema.safeParse(body);
+): Promise<ValidateSendEmailResult> {
+ const rawBody = await readRawBody(request);
+ let parsed: unknown = {};
+ if (rawBody) {
+ try {
</file context>
| let parsed: unknown = {}; | |
| if (rawBody) { | |
| try { | |
| parsed = JSON.parse(rawBody); | |
| } catch { | |
| parsed = {}; | |
| } | |
| } | |
| if (!rawBody) { | |
| return { | |
| rawBody, | |
| error: NextResponse.json( | |
| { status: "error", missing_fields: [], error: "Request body must be valid JSON." }, | |
| { status: 400, headers: getCorsHeaders() }, | |
| ), | |
| }; | |
| } | |
| let parsed: unknown; | |
| try { | |
| parsed = JSON.parse(rawBody); | |
| } catch { | |
| return { | |
| rawBody, | |
| error: NextResponse.json( | |
| { status: "error", missing_fields: [], error: "Request body must be valid JSON." }, | |
| { status: 400, headers: getCorsHeaders() }, | |
| ), | |
| }; | |
| } |
| NextResponse.json({ status: "error", error: "Forbidden" }, { status: 403 }), | ||
| ); | ||
| mockValidateSendEmailBody.mockResolvedValue({ | ||
| rawBody: "{}", |
There was a problem hiding this comment.
P2: Rejected and send_failed test assertions omit rawBody, so a regression where rawBody is silently dropped on error paths would not be caught. Add rawBody to both assertions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/emails/__tests__/sendEmailHandler.test.ts, line 86:
<comment>Rejected and send_failed test assertions omit `rawBody`, so a regression where `rawBody` is silently dropped on error paths would not be caught. Add `rawBody` to both assertions.</comment>
<file context>
@@ -65,15 +68,24 @@ describe("sendEmailHandler", () => {
- NextResponse.json({ status: "error", error: "Forbidden" }, { status: 403 }),
- );
+ mockValidateSendEmailBody.mockResolvedValue({
+ rawBody: "{}",
+ error: NextResponse.json({ status: "error", error: "Forbidden" }, { status: 403 }),
+ });
</file context>
| }), | ||
| ); | ||
| // Single call, on every path (DRY); rawBody comes from validateSendEmailBody (SRP). | ||
| expect(mockLogEmailAttempt).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
P3: Rejected and send_failed tests don't assert toHaveBeenCalledTimes(1), so a double-log regression on error paths wouldn't be caught. Add the count assertion for consistency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/emails/__tests__/sendEmailHandler.test.ts, line 72:
<comment>Rejected and send_failed tests don't assert `toHaveBeenCalledTimes(1)`, so a double-log regression on error paths wouldn't be caught. Add the count assertion for consistency.</comment>
<file context>
@@ -65,15 +68,24 @@ describe("sendEmailHandler", () => {
}),
);
+ // Single call, on every path (DRY); rawBody comes from validateSendEmailBody (SRP).
+ expect(mockLogEmailAttempt).toHaveBeenCalledTimes(1);
expect(mockLogEmailAttempt).toHaveBeenCalledWith(
- expect.objectContaining({ status: "sent", resendId: "resend-id-1" }),
</file context>
| expect(result).not.toBeInstanceOf(NextResponse); | ||
| if (!(result instanceof NextResponse)) { | ||
| expect(result.subject).toBe("Pulse Report"); | ||
| expect("data" in result).toBe(true); |
There was a problem hiding this comment.
P3: Tests assert "data" in result / "error" in result to check the discriminated-union shape, but never verify the rawBody field — the central contract of the return type.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/emails/__tests__/validateSendEmailBody.test.ts, line 86:
<comment>Tests assert `"data" in result` / `"error" in result` to check the discriminated-union shape, but never verify the `rawBody` field — the central contract of the return type.</comment>
<file context>
@@ -87,18 +83,18 @@ describe("validateSendEmailBody", () => {
- expect(result).not.toBeInstanceOf(NextResponse);
- if (!(result instanceof NextResponse)) {
- expect(result.subject).toBe("Pulse Report");
+ expect("data" in result).toBe(true);
+ if ("data" in result) {
+ expect(result.data.subject).toBe("Pulse Report");
</file context>
| if (result instanceof NextResponse) { | ||
| expect(result.status).toBe(403); | ||
| const json = await result.json(); | ||
| expect("error" in result).toBe(true); |
There was a problem hiding this comment.
P3: Same: the validation-error test (empty body → 400) asserts shape with "error" in result but never checks rawBody contents.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/emails/__tests__/validateSendEmailBody.test.ts, line 57:
<comment>Same: the validation-error test (empty body → 400) asserts shape with `"error" in result` but never checks `rawBody` contents.</comment>
<file context>
@@ -58,10 +54,10 @@ describe("validateSendEmailBody", () => {
- if (result instanceof NextResponse) {
- expect(result.status).toBe(403);
- const json = await result.json();
+ expect("error" in result).toBe(true);
+ if ("error" in result) {
+ expect(result.error.status).toBe(403);
</file context>
|
|
||
| const { to, cc = [], subject, text, html = "", headers = {}, chat_id } = validated; | ||
| /** Sends a validated email; returns the HTTP response plus the attempt to log. */ | ||
| async function deliver( |
There was a problem hiding this comment.
SRP - new lib file for deliver function.
There was a problem hiding this comment.
Done in 70c155e. deliver is now lib/emails/deliverEmail.ts (one exported function: Resend send + response/attempt shaping), with __tests__/deliverEmail.test.ts covering the sent (200, resendId, chat_id→room_id) and send_failed (502, no resendId) paths. sendEmailHandler is now pure orchestration — validate → deliverEmail → one logEmailAttempt.
There was a problem hiding this comment.
Reverted in 780fe79 after discussion. On reflection deliverEmail's { response, attempt } return was handler-internal glue (single caller, only coherent inside the handler), not a reusable unit — so it's now inlined back into sendEmailHandler (~24 lines, still one logEmailAttempt call). readRawBody stays its own file since it's a cohesive, reusable utility next to safeParseJson. Mapping coverage (200 / 502 / rejected) remains in sendEmailHandler.test.ts via the processAndSendEmail mock.
| | { rawBody: string; data: ValidatedSendEmailRequest }; | ||
|
|
||
| /** Read the request body once, as text (the source for both parsing and logging). */ | ||
| async function readRawBody(request: NextRequest): Promise<string> { |
There was a problem hiding this comment.
SRP - new lib file for readRawBody
There was a problem hiding this comment.
Done in 70c155e. readRawBody is now its own file — lib/networking/readRawBody.ts, sibling to safeParseJson (same NextRequest signature) — with a dedicated __tests__/readRawBody.test.ts (body verbatim / empty body / throw → ""). validateSendEmailBody imports it.
Per the one-exported-function-per-file convention (review on #731): - lib/networking/readRawBody.ts — body-as-text read (sibling of safeParseJson); validateSendEmailBody imports it. - lib/emails/deliverEmail.ts — Resend send + response/attempt shaping; sendEmailHandler delegates to it. Handler is now pure orchestration. Both extractions get dedicated unit tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Live preview verification —
|
Sent path verified —
|
| Path | Auth | HTTP | logged status |
account_id | chat_id | resend_id | raw_body |
|---|---|---|---|---|---|---|---|
| rejected | none | 401 | rejected |
null | null | null | full (91 B) |
| sent | Bearer | 200 | sent |
fb678396… |
…-chat |
matches response | full (123 B) |
Closes out the review on this PR — SRP/DRY refactor + the deliverEmail/readRawBody extractions are behavior-identical and observability logs correctly on both paths.
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="supabase/migrations/20260630_email_send_log.sql">
<violation number="1" location="supabase/migrations/20260630_email_send_log.sql:4">
P1: Enable RLS on this new public table before storing raw email request bodies. Without it, the log table can expose captured request content through the Supabase API if public-schema privileges are available.</violation>
<violation number="2" location="supabase/migrations/20260630_email_send_log.sql:9">
P2: Add a CHECK constraint for the documented status values. The comment alone does not protect email_send_log from invalid statuses.</violation>
</file>
<file name="lib/emails/logEmailAttempt.ts">
<violation number="1" location="lib/emails/logEmailAttempt.ts:30">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
The PR description claims `logEmailAttempt.ts` truncates `raw_body` to 10k and computes `body_parsed` via `JSON.parse`, but the diff removes both features. The description and the actual code changes are inconsistent.</violation>
</file>
<file name="lib/emails/__tests__/validateSendEmailBody.test.ts">
<violation number="1" location="lib/emails/__tests__/validateSendEmailBody.test.ts:57">
P3: Same: the validation-error test (empty body → 400) asserts shape with `"error" in result` but never checks `rawBody` contents.</violation>
<violation number="2" location="lib/emails/__tests__/validateSendEmailBody.test.ts:86">
P3: Tests assert `"data" in result` / `"error" in result` to check the discriminated-union shape, but never verify the `rawBody` field — the central contract of the return type.</violation>
</file>
<file name="lib/emails/validateSendEmailBody.ts">
<violation number="1" location="lib/emails/validateSendEmailBody.ts:62">
P1: Malformed JSON is converted into a valid empty body, so authenticated bad requests can still reach `deliver()` and send the footer-only/default email this PR is meant to reject. Return a 400 on empty or unparsable raw bodies instead of passing `{}` to Zod.</violation>
</file>
<file name="lib/emails/__tests__/sendEmailHandler.test.ts">
<violation number="1" location="lib/emails/__tests__/sendEmailHandler.test.ts:72">
P3: Rejected and send_failed tests don't assert `toHaveBeenCalledTimes(1)`, so a double-log regression on error paths wouldn't be caught. Add the count assertion for consistency.</violation>
<violation number="2" location="lib/emails/__tests__/sendEmailHandler.test.ts:86">
P2: Rejected and send_failed test assertions omit `rawBody`, so a regression where `rawBody` is silently dropped on error paths would not be caught. Add `rawBody` to both assertions.</violation>
</file>
<file name="lib/networking/readRawBody.ts">
<violation number="1" location="lib/networking/readRawBody.ts:15">
P1: `readRawBody` catches every body-read failure and silently returns `""`, making an unreadable request body indistinguishable from a genuinely empty body. Downstream, `validateSendEmailBody` treats `""` as a malformed/empty request, returns 400, and `sendEmailHandler` logs `raw_body = ""` with `status: "rejected"`. This loses the very evidence the PR is meant to preserve when a read error occurs, and it misattributes server-side read failures to the client.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| try { | ||
| return await request.text(); | ||
| } catch { | ||
| return ""; |
There was a problem hiding this comment.
P1: readRawBody catches every body-read failure and silently returns "", making an unreadable request body indistinguishable from a genuinely empty body. Downstream, validateSendEmailBody treats "" as a malformed/empty request, returns 400, and sendEmailHandler logs raw_body = "" with status: "rejected". This loses the very evidence the PR is meant to preserve when a read error occurs, and it misattributes server-side read failures to the client.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/networking/readRawBody.ts, line 15:
<comment>`readRawBody` catches every body-read failure and silently returns `""`, making an unreadable request body indistinguishable from a genuinely empty body. Downstream, `validateSendEmailBody` treats `""` as a malformed/empty request, returns 400, and `sendEmailHandler` logs `raw_body = ""` with `status: "rejected"`. This loses the very evidence the PR is meant to preserve when a read error occurs, and it misattributes server-side read failures to the client.</comment>
<file context>
@@ -0,0 +1,17 @@
+ try {
+ return await request.text();
+ } catch {
+ return "";
+ }
+}
</file context>
Per review discussion on #731: deliverEmail's {response, attempt} return was handler-internal glue with a single caller, not a reusable unit. Inline it back into sendEmailHandler — still one logEmailAttempt call (DRY), still ~24 lines (under the cap). Keep readRawBody as its own file (cohesive, reusable). Mapping coverage stays in sendEmailHandler.test.ts via the processAndSendEmail mock. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ent footer-only sends)
Guard sendEmailBodySchema with a refine requiring a non-empty html or text body,
and drop the html "" default. A malformed/empty body parses to {} and now returns
400 instead of silently sending a "Message from Recoup" footer-only email.
Rebased onto main after #731 (email_send_log observability) rewrote
validateSendEmailBody: reconciled to the new { rawBody, error } | { rawBody, data }
return shape and the readRawBody read path (comment updated; safeParseJson no
longer used here). With #731 in place, a guarded 400 is also recorded as a
`rejected` row in email_send_log.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ent footer-only sends) (#729) Guard sendEmailBodySchema with a refine requiring a non-empty html or text body, and drop the html "" default. A malformed/empty body parses to {} and now returns 400 instead of silently sending a "Message from Recoup" footer-only email. Rebased onto main after #731 (email_send_log observability) rewrote validateSendEmailBody: reconciled to the new { rawBody, error } | { rawBody, data } return shape and the readRawBody read path (comment updated; safeParseJson no longer used here). With #731 in place, a guarded 400 is also recorded as a `rejected` row in email_send_log. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
What
Records every
POST /api/emailsattempt —sent,send_failed,rejected(empty/malformed) — inemail_send_log, so a send is debuggable days later without the ephemeral agent sandbox.Why
The empty footer-only emails reached customers and we could not recover what the API received (malformed bodies swallowed, route logged nothing, sandbox ephemeral). This is the instrument that makes the rest of the issue measurable.
Change
lib/emails/logEmailAttempt.ts— best-effort writer (never throws), inserts the 5 settable columns:account_id, chat_id, status, resend_id, raw_body. Stores the full raw body (no truncation).lib/emails/sendEmailHandler.ts— captures the raw body viarequest.clone()(validation still reads the original) and logs on every outcome; rejected attempts keep the raw body, so a malformed/empty send is visible after the fact.lib/supabase/email_send_log/insertEmailSendLog.ts— typed insert.types/database.types.ts— regenerated from the live schema (supabase gen typesequivalent); adds the 7-columnemail_send_log(id, created_at, account_id, chat_id, status, resend_id, raw_body).The table + RLS ship in the
databaserepo — #37 (create) and #38 (droperror, enable RLS), both merged. Schema reviewed down to the minimal 7 columns (KISS).Tests
logEmailAttemptunit (fields + full-body + swallow-errors) andsendEmailHandler(sent/rejected/send_failed log calls). Fulllib/emailssuite: 149 passing.tsc+ lint clean.Part of recoupable/chat#1829. Base
main.🤖 Generated with Claude Code