Observability/signup

[KyleTryon]

This pull request introduces enhanced monitoring, security auditing, and reliability improvements to authentication and subscription-related API endpoints. The main changes include adding Sentry spans and breadcrumbs for observability, logging security events for authentication actions, and implementing retry logic for transient feed fetch failures.

Observability and Monitoring Enhancements:

• Added Sentry spans and breadcrumbs to the authentication (authRouter) and articles (articlesRouter) endpoints to track login, password change, password reset, and batch article operations. This includes setting span attributes, capturing exceptions, and emitting metrics for success and failure cases. [1] [2] [3] [4] [5] [6]

Security Auditing Improvements:

• Integrated security event logging for successful and failed login, password change, and password reset actions, capturing details like user ID, IP address, user agent, and error metadata. Audit logging failures are handled gracefully and reported to Sentry. [1] [2] [3] [4]

Reliability Improvements for Feed Fetching:

• Implemented retry logic with exponential backoff for transient HTTP errors (e.g., 502, 503, 504, 429) when fetching feeds in the subscriptionsRouter. Retries are tracked with Sentry breadcrumbs, and final failures are reported with detailed error context. [1] [2] [3]

Utility Function Addition:

• Added a new extractHeaders utility in security.ts to normalize headers from requests, supporting both Headers objects and plain objects. This is used throughout the routers for consistent header processing.

Code Quality and Consistency:

• Refactored user ID extraction and error handling for authentication flows to improve clarity and maintainability. Ensured consistent use of extracted header information and robust error reporting. [1] [2]

Let me know if you want to dive deeper into any specific part of these changes!

[KyleTryon]

@sentry review

[sentry]

The code uses await Sentry.startSpan() syntax on line 426, but Sentry's startSpan is not an async function and should not be awaited. The Sentry.startSpan() signature takes a callback that can be async, but the startSpan call itself is synchronous. The correct pattern is return Sentry.startSpan({...}, async (span) => {...}) without awaiting the startSpan call.
_{Severity: HIGH}

🤖 Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: packages/api/src/routers/auth.ts#L426-L430

Potential issue: The code uses `await Sentry.startSpan()` syntax on line 426, but
Sentry's startSpan is not an async function and should not be awaited. The
`Sentry.startSpan()` signature takes a callback that can be async, but the startSpan
call itself is synchronous. The correct pattern is `return Sentry.startSpan({...}, async
(span) => {...})` without awaiting the startSpan call.

_{Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5366126}

[sentry]

Unnecessary awaits on non-async breadcrumb calls. Lines 121-130, 142-152, 222-230, and 237-245 all contain await Sentry.addBreadcrumb(...). However, Sentry.addBreadcrumb() is a synchronous function and should not be awaited. Removing these awaits will improve performance and align with Sentry API usage patterns.
_{Severity: MEDIUM}

🤖 Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: packages/api/src/services/email.ts#L121-L245

Potential issue: Unnecessary awaits on non-async breadcrumb calls. Lines 121-130,
142-152, 222-230, and 237-245 all contain `await Sentry.addBreadcrumb(...)`. However,
`Sentry.addBreadcrumb()` is a synchronous function and should not be awaited. Removing
these awaits will improve performance and align with Sentry API usage patterns.

_{Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5366126}

[sentry]

On line 468, parentSpan?.setAttributes() is called but the previous pattern throughout the codebase uses parentSpan?.setAttribute() (singular). This inconsistency may indicate a typo. Verify the correct Sentry API method name - it should likely be setAttributes() for setting multiple attributes at once, but the singular form is used elsewhere in the codebase.
_{Severity: MEDIUM}

🤖 Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: packages/api/src/routers/auth.ts#L468-L476

Potential issue: On line 468, `parentSpan?.setAttributes()` is called but the previous
pattern throughout the codebase uses `parentSpan?.setAttribute()` (singular). This
inconsistency may indicate a typo. Verify the correct Sentry API method name - it should
likely be `setAttributes()` for setting multiple attributes at once, but the singular
form is used elsewhere in the codebase.

_{Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5366126}

[sentry]

In the OPML import span loop (lines 1879-2099), the loop uses for (const feedInfo of feedsToProcess) with a shouldStopImporting flag that gets set to true when the limit is reached. However, the shouldStopImporting variable is declared inside the Sentry.startSpan callback scope on line 1834, but it's set inside nested Sentry.startSpan callbacks (line 1932). This creates a scope issue where the outer loop's break condition might not work correctly. The variable should be declared in the outer scope before the loop.
_{Severity: HIGH}

🤖 Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: packages/api/src/routers/subscriptions.ts#L1834-L1840

Potential issue: In the OPML import span loop (lines 1879-2099), the loop uses `for
(const feedInfo of feedsToProcess)` with a `shouldStopImporting` flag that gets set to
true when the limit is reached. However, the `shouldStopImporting` variable is declared
inside the `Sentry.startSpan` callback scope on line 1834, but it's set inside nested
`Sentry.startSpan` callbacks (line 1932). This creates a scope issue where the outer
loop's break condition might not work correctly. The variable should be declared in the
outer scope before the loop.

_{Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5366126}

[Copilot]

Pull request overview

This pull request enhances observability, security auditing, and reliability across authentication and subscription-related API endpoints by integrating Sentry instrumentation, implementing security event logging, and adding retry logic for transient failures.

Key Changes:

• Added comprehensive Sentry spans and breadcrumbs across authentication flows (login, password reset, password change) and article operations for better observability and performance tracking
• Integrated security audit logging for authentication events (successful/failed logins, password changes, resets) with IP address and user agent tracking
• Implemented exponential backoff retry logic for transient HTTP errors (502, 503, 504, 429) when fetching RSS feeds

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
packages/api/src/auth/security.ts	Added extractHeaders utility function to normalize request headers from different formats (Headers object vs plain object)
packages/api/src/routers/auth.ts	Wrapped login endpoint in Sentry span with detailed tracking; added security audit logging for login, password change, and password reset events with comprehensive error handling
packages/api/src/auth/better-auth.ts	Enhanced email sending callbacks with Sentry spans and breadcrumbs; refactored header extraction to use new utility function for consistency
packages/api/src/routers/subscriptions.ts	Implemented retry logic with exponential backoff for feed fetching in both create and preview endpoints; added Sentry instrumentation for OPML import with per-feed tracking
packages/api/src/routers/articles.ts	Wrapped batch operations for marking articles read/unread in Sentry spans for performance monitoring and error tracking
packages/api/src/services/rss-fetcher.ts	Added Sentry span around OpenGraph image extraction with detailed error tracking and metrics emission
packages/api/src/services/email.ts	Enhanced email service with additional breadcrumbs at key points (before API call, on success, on error) for better debugging

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The retry logic has the same issue as in the create endpoint: when parseFeed() throws an error (e.g., invalid feed XML), the error message won't match the regex /HTTP (\d+):/, so httpStatus will be 0. Since 0 is not in TRANSIENT_STATUS_CODES, the code will throw immediately without retrying. However, this also means that even if the fetch succeeded but parsing failed, we're still inside the retry loop. The parsing errors should not be retried - only fetch errors should be. Consider checking if the error occurred during fetch vs parse, and only apply retry logic to fetch errors.

[Copilot]

The action logged for a failed password reset should be "password_reset_failed" but it's currently set to "password_reset_success". This will cause failed password reset attempts to be logged as successful, which is incorrect and could lead to confusion in security auditing.

Suggested change

	action: "password_reset_success",
	action: "password_reset_failed",

[Copilot]

When value is undefined, calling String(value) will convert it to the string "undefined" instead of keeping it as undefined. This could lead to unexpected behavior where headers with undefined values are treated as having the literal string "undefined" as their value. Consider checking for undefined explicitly: value === undefined ? undefined : (Array.isArray(value) ? value[0] : String(value)).

Suggested change

	headers[key.toLowerCase()] = Array.isArray(value)
	? value[0]
	: String(value);
	headers[key.toLowerCase()] = value === undefined
	? undefined
	: Array.isArray(value)
	? value[0]
	: String(value);

[Copilot]

The retry logic has a potential issue: when parseFeed() throws an error (e.g., invalid feed XML), the error message won't match the regex /HTTP (\d+):/, so httpStatus will be 0. Since 0 is not in TRANSIENT_STATUS_CODES, the code will throw immediately without retrying. However, this also means that even if the fetch succeeded but parsing failed, we're still inside the retry loop. The parsing errors should not be retried - only fetch errors should be. Consider checking if the error occurred during fetch vs parse, and only apply retry logic to fetch errors.

[Copilot]

The value assigned to lastError here is unused.

Suggested change

lastError = new Error(errorMessage);

[Copilot]

The value assigned to lastError here is unused.

Suggested change

lastError = new Error(errorMessage);

[Copilot]

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated 8 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Header extraction and IP/user agent retrieval logic (lines 507-509) is duplicated at lines 608-610. Similarly, this pattern is repeated at lines 867-868, 884-886, 1061-1062, and 1078-1080. Consider extracting this into a helper function to reduce duplication and improve maintainability.

Suggested change

	const headers = extractHeaders(ctx.req.headers);
	const ipAddress = getClientIp(headers);
	const userAgent = getUserAgent(headers);
	try {
	await logSecurityEvent(ctx.db, {
	userId,
	action: "login_success",
	ipAddress,
	userAgent,
	success: true,
	});
	} catch (auditError) {
	const { headers, ipAddress, userAgent } = getRequestClientInfo(ctx.req.headers, { getClientIp, getUserAgent, extractHeaders });

[Copilot]

The Sentry span for sending verification email is started without awaiting its completion (fire-and-forget pattern). While this is intentional for non-blocking email operations, the span is started but never properly awaited, which means the span tracking may not complete before the function returns. Consider using Sentry.startSpan(...).then(...) pattern or documenting this behavior more clearly to avoid confusion about incomplete spans.

[Copilot]

The OPML import loop processes feeds sequentially and creates a Sentry span for each feed. For large OPML files with many feeds, this could result in a very long operation with many spans. Consider adding a progress indicator or implementing batch processing with concurrent feed fetches (with appropriate rate limiting) to improve performance and user experience.

[Copilot]

The createdAt field is being manually set to new Date() in the logSecurityEvent function, but the database schema now has a SQL-level default for this column. This manual assignment is redundant and could potentially cause issues if the application's timestamp differs from the database server's timestamp. Consider removing this explicit assignment and relying on the database default for consistency.

Suggested change

createdAt: new Date(),

[Copilot]

The retry loop uses continue to retry on transient errors, but this skips the rest of the loop iteration without properly handling the error. After continue, the outer catch block at line 410 won't be reached for this iteration, meaning lastError won't be set. If all retries fail with transient errors using continue, the code could exit the loop without feedData being set and without proper error tracking. Consider throwing an error or using a different control flow pattern.

Suggested change

	continue; // Retry
	lastError = new Error(errorMessage);
	continue;

[Copilot]

Same issue as in the feed subscription endpoint: using continue to retry on transient errors can skip error tracking. If all retries result in continue statements, the loop will exit without feedData being set and without lastError being populated, potentially leading to the fallback error at line 1159 with an unhelpful message.

Suggested change

	) {
	) {
	lastError = new Error(errorMessage); // Track last error for fallback

[Copilot]

The retry configuration (MAX_RETRIES, RETRY_DELAY_MS, TRANSIENT_STATUS_CODES) is duplicated across multiple endpoints in this file (lines 305-308, 995-997). Consider extracting these constants to a shared location at the top of the file or in a separate constants module to maintain consistency and ease future updates.

[Copilot]

The security module imports (logSecurityEvent, getClientIp, getUserAgent, extractHeaders) are duplicated at lines 500-505 and 601-606 within the same function scope. These could be imported once at the beginning of the outer function scope to reduce redundancy and improve code maintainability.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The createdAt field is being explicitly set in the application code (line 55), but the database schema also has a SQL DEFAULT (line 461). This creates a redundancy issue:

1. The schema default uses unixepoch('subsecond') * 1000 which is milliseconds since epoch
2. The application code uses new Date() which creates a JavaScript Date object

When both are present, the application value will override the database default, but this defeats the purpose of having a database-level default. The comment mentions "defense-in-depth" but this is unnecessary complexity.

Recommendation: Either:

• Remove the explicit createdAt: new Date() from the insert statement (preferred - rely on DB default)
• Or remove the .default() from the schema if you want application-level control

Since the PR is specifically adding a DB-level SQL default (migration 0006), the application-level setting should be removed.

Suggested change

	// Explicitly set createdAt for consistency with app time
	// DB has SQL DEFAULT but we set it here for defense-in-depth
	createdAt: new Date(),

[Copilot]

The parse error handling has a critical bug. When a parse error is thrown at line 404-407, it's caught by the outer catch (error) block at line 409, which is labeled "only for FETCH errors now" (line 410).

This means:

1. Parse errors will be incorrectly treated as fetch errors
2. The code will attempt to retry parse errors if they occur on attempts < MAX_RETRIES
3. The error handling logic (lines 414-471) will incorrectly apply HTTP status code retry logic to parse errors

Fix: The throw new TRPCError at lines 404-407 should be changed to immediately exit the function, not be caught by the outer catch block. You can:

• Move the parse error throw outside of the try-catch structure
• Or use a flag to indicate parse error and check it before entering retry logic
• Or rethrow the TRPCError specifically in the outer catch block

[Copilot]

Same critical bug as in the feed subscription endpoint: Parse errors thrown at line 1080-1083 will be caught by the outer catch (error) block at line 1085, which is intended "only for FETCH errors" (line 1086).

This causes parse errors to be incorrectly handled as fetch errors and potentially retried when they shouldn't be. The fix is the same as for the subscription endpoint - ensure TRPCError throws escape the retry loop immediately.

Suggested change

	// This catch block is only for FETCH errors now
	// This catch block is only for FETCH errors now
	if (error instanceof TRPCError) {
	throw error;
	}