ENG-25: Implement email verification, password recovery, and Resend email integration#9
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (14)
src/ai_trading_discipline_copilot/services/email_service.py (2)
42-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLocal imports inside method.
import htmlandfrom datetime import UTC, datetimeare imported insidesend_verification_emailrather than at module top. Moving these to the top-level imports is more idiomatic and avoids repeated import overhead on every call.🤖 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 `@src/ai_trading_discipline_copilot/services/email_service.py` around lines 42 - 43, The imports in send_verification_email are being done locally instead of at module scope. Move the html import and the UTC/datetime import to the top-level import section in email_service.py so EmailService.send_verification_email uses the shared module imports rather than re-importing on every call.
26-33: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo error handling around outbound email send.
Both call sites in
send_email/send_verification_emaildon't catch exceptions from Resend. Since these run insideBackgroundTasks, an unhandled exception is only logged internally by FastAPI/Starlette with no retry or alerting — verification/reset/registration emails can silently fail to send while the caller already returned a "success" response to the user.Consider wrapping the send call with try/except plus structured logging (and optionally a retry) so failures are observable.
Also applies to: 141-145
🤖 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 `@src/ai_trading_discipline_copilot/services/email_service.py` around lines 26 - 33, The Resend outbound send in send_email/send_verification_email has no exception handling, so BackgroundTasks failures can be swallowed and never surfaced. Wrap the resend.Emails.send call in try/except inside the email service methods, add structured logging with the exception details and recipient/subject context, and make sure the failure path is observable (and optionally retried) instead of silently succeeding. Use the send_email and send_verification_email symbols to update both call sites consistently.src/ai_trading_discipline_copilot/schemas/email_verification.py (1)
6-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent token validation vs.
ResetPasswordRequest.
ResetPasswordRequest.tokenenforcesmin_length=32(matchingsecrets.token_urlsafe(32)), butVerifyEmailRequest.tokenhas no length constraint despite using the same token generation scheme. Consider aligning for consistency and to reject malformed input earlier.♻️ Suggested fix
+from pydantic import BaseModel, EmailStr, Field + class VerifyEmailRequest(BaseModel): """Request to verify email.""" - token: str + token: str = Field(min_length=32)🤖 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 `@src/ai_trading_discipline_copilot/schemas/email_verification.py` around lines 6 - 9, Align VerifyEmailRequest.token with ResetPasswordRequest.token by adding the same minimum length validation used for the shared token format generated by secrets.token_urlsafe(32). Update the VerifyEmailRequest model in email_verification.py so it rejects too-short malformed tokens early, keeping its validation consistent with ResetPasswordRequest and the rest of the token-based flow.src/ai_trading_discipline_copilot/services/__init__.py (1)
7-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
__all__is not sorted (RUF022).Ruff flags the ordering. If this rule is enforced in CI it will fail; apply an isort-style sort.
Proposed ordering
__all__ = [ - "UserService", - "PasswordResetService", "EmailVerificationService", + "PasswordResetService", + "UserService", ]🤖 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 `@src/ai_trading_discipline_copilot/services/__init__.py` around lines 7 - 11, The __all__ export list in the services package is not in the sorted order expected by Ruff (RUF022). Update the __all__ definition in the __init__ module so the exported names are ordered alphabetically using isort-style sorting, keeping the existing symbols UserService, PasswordResetService, and EmailVerificationService in the corrected order.Source: Linters/SAST tools
src/ai_trading_discipline_copilot/routers/auth.py (1)
344-457: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
send_reset_email_taskduplicates the email HTML/template logic inEmailService.The inline reset-email HTML template mirrors
EmailService.send_verification_email(same styles, structure,html.escapehandling, footer/year). Building templates in the router while the verification email lives inEmailServicesplits responsibilities and duplicates markup that will drift. Consider moving this into anEmailService.send_password_reset_email(...)method alongsidesend_verification_emailand calling it from the background task.🤖 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 `@src/ai_trading_discipline_copilot/routers/auth.py` around lines 344 - 457, The password reset email template is duplicated in send_reset_email_task instead of living in EmailService like the verification email. Move the HTML/template-building logic into a dedicated EmailService.send_password_reset_email method, using the same pattern as send_verification_email, and have send_reset_email_task only prepare inputs and call that service method. Keep the escaping, subject, and footer/year handling inside EmailService so email rendering stays centralized and consistent.src/ai_trading_discipline_copilot/services/email_verification_service.py (1)
26-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffNear-total duplication with
PasswordResetService.
EmailVerificationServiceandPasswordResetServiceare structurally identical forgenerate_token,hash_token,create_*_token,validate_token, andcleanup_expired_tokens(only the model, expiry window, and log strings differ). This duplicated token lifecycle will drift over time (e.g., a security fix applied to one but not the other).Consider extracting a shared generic base (e.g., a
TokenServiceparameterized by model + expiry) so both services inherit token generation, hashing, rotation, validation, and cleanup.🤖 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 `@src/ai_trading_discipline_copilot/services/email_verification_service.py` around lines 26 - 203, `EmailVerificationService` is duplicating the same token lifecycle logic as `PasswordResetService`, so extract the shared behavior into a reusable `TokenService`-style base/helper that owns `generate_token`, `hash_token`, token creation/rotation, `validate_token`, and `cleanup_expired_tokens`. Parameterize the shared logic by the token model, expiry duration, and log message context, then make `EmailVerificationService` and `PasswordResetService` delegate to it so only the email-specific/user-specific pieces remain.tests/test_password_reset.py (1)
25-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate user-creation fixture across test files.
test_userhere largely duplicatesunverified_userintests/test_email_verification.py(same construction pattern, differing only inis_verified/username/email). Consider extracting a shared parametrized user factory fixture inconftest.pyto reduce duplication across the two new test suites.🤖 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 `@tests/test_password_reset.py` around lines 25 - 37, The test_user fixture duplicates the user creation logic already present in unverified_user, so extract the shared setup into a reusable parametrized user factory in conftest.py. Update both test_password_reset and test_email_verification to use that shared fixture/factory, keeping only the differing fields like username, email, and is_verified in the per-test parameters.tests/conftest.py (1)
123-133: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGlobal
is_verified=Truedefault could mask a missing production default.This event listener unconditionally forces
is_verified=Truefor everyUser()construction across the entire test suite unless a test explicitly passes the field. If the actual registration/service code path ever fails to explicitly setis_verified=False(e.g. a future refactor accidentally drops the explicit kwarg, relying on a column default that doesn't exist), no test would catch it, since this hook always injectsTrueregardless of what production code does. Consider instead giving theis_verifiedcolumn an explicit SQLAlchemy-level default (e.g.default=False) so behavior is consistent in both prod and tests, or updating pre-existing fixtures explicitly rather than a blanket event hook.Also, the
sqlalchemyandUserimports (Lines 125-126) are placed mid-file rather than with the top-level imports — consider consolidating for readability.🤖 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 `@tests/conftest.py` around lines 123 - 133, The test-only User init hook is masking real default behavior by forcing is_verified=True for every User() unless explicitly passed. Remove the blanket event listener in conftest and make is_verified’s intended default explicit in the User model/column definition (or update fixtures to pass it where needed) so production and tests behave consistently. Also move the sqlalchemy event and User imports into the top-level import section of conftest for readability.src/ai_trading_discipline_copilot/models/password_reset_token.py (1)
49-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate 15-minute expiry constant.
The model's
default=lambda: ... + timedelta(minutes=15)duplicates the same value used explicitly inPasswordResetService.create_reset_token. Since the service always overrides this default, consider extracting a shared constant (e.g.PASSWORD_RESET_TOKEN_TTL) to avoid the two literals drifting apart.🤖 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 `@src/ai_trading_discipline_copilot/models/password_reset_token.py` around lines 49 - 53, The password reset token TTL is hardcoded twice, once in PasswordResetToken.expires_at and again in PasswordResetService.create_reset_token, which can drift over time. Extract the 15-minute value into a shared constant such as PASSWORD_RESET_TOKEN_TTL and use it in both the model default and the service’s token creation logic so they stay consistent.src/ai_trading_discipline_copilot/core/config.py (2)
87-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow the caught exception type.
Ruff flags this blind
except Exception. Since onlyTypeAdapter(...).validate_python(...)can realistically fail here, catchingpydantic.ValidationError(orpydantic_core.ValidationError) would be more precise without changing behavior.🤖 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 `@src/ai_trading_discipline_copilot/core/config.py` around lines 87 - 90, The database URL validation in config.py is using a broad except Exception around TypeAdapter(PostgresDsn).validate_python, which should be narrowed. Update the try/except in the database URL parsing logic to catch the specific Pydantic validation error type raised by validate_python, such as pydantic.ValidationError or pydantic_core.ValidationError, while keeping the same ValueError re-raise behavior and message formatting.Source: Linters/SAST tools
59-68: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd basic validation for the new email/frontend settings.
resend_api_key,email_from, andfrontend_urlhave no validation, unlikesecret_key. An empty or malformed value (e.g., missing env var substituting a blank string, or afrontend_urlwith a trailing slash) will pass startup validation silently and only surface later as a broken verification/reset link or a failed Resend API call.♻️ Suggested validators
+ `@field_validator`("resend_api_key") + `@classmethod` + def resend_api_key_not_empty(cls, v: SecretStr) -> SecretStr: + if not v.get_secret_value().strip(): + raise ValueError("RESEND_API_KEY must not be empty") + return v + + `@field_validator`("frontend_url") + `@classmethod` + def normalize_frontend_url(cls, v: str) -> str: + if not v.strip(): + raise ValueError("FRONTEND_URL must not be empty") + return v.rstrip("/")🤖 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 `@src/ai_trading_discipline_copilot/core/config.py` around lines 59 - 68, The new email and frontend config fields in the settings model are currently accepted without validation, so add basic checks alongside the existing config validation in the Config class. Ensure `resend_api_key`, `email_from`, and `frontend_url` cannot be empty or blank, and normalize/validate `frontend_url` so it does not keep a trailing slash that would break generated links. Reuse the same settings validation pattern used for `secret_key`, and keep the checks attached to the relevant fields in `core/config.py` so startup fails fast on bad env values.src/ai_trading_discipline_copilot/models/__init__.py (1)
8-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSort
__all__to satisfy Ruff RUF022.Static analysis flags the list as unsorted.
♻️ Suggested sort
__all__ = [ - "User", - "RefreshToken", - "PasswordResetToken", - "EmailVerificationToken", + "EmailVerificationToken", + "PasswordResetToken", + "RefreshToken", + "User", ]🤖 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 `@src/ai_trading_discipline_copilot/models/__init__.py` around lines 8 - 13, The __all__ list in the models package is not sorted, triggering Ruff RUF022. Reorder the exported names in __all__ in alphabetical order in the __init__ module so the static analysis rule passes, keeping the existing symbols User, RefreshToken, PasswordResetToken, and EmailVerificationToken.Source: Linters/SAST tools
alembic/versions/526b088ddfed_add_email_verification_tokens_table.py (1)
24-36: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSame missing
expires_atindex as the password reset tokens migration.Same rationale as
alembic/versions/3ead82c29db6_add_password_reset_tokens_table.py: theemail_verification_tokenstable will also be swept for expired rows during cleanup, and lacks an index onexpires_at.♻️ Suggested index addition
op.create_index(op.f('ix_email_verification_tokens_user_id'), 'email_verification_tokens', ['user_id'], unique=False) + op.create_index(op.f('ix_email_verification_tokens_expires_at'), 'email_verification_tokens', ['expires_at'], unique=False)🤖 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 `@alembic/versions/526b088ddfed_add_email_verification_tokens_table.py` around lines 24 - 36, The email verification token migration is missing the same `expires_at` index used by the password reset tokens table, so add an index for `expires_at` in the `op.create_table` flow for `email_verification_tokens` alongside the existing `user_id` index. Update the migration so cleanup queries can efficiently target expired rows, using the existing `op.create_index` pattern and the `email_verification_tokens` table definition.alembic/versions/3ead82c29db6_add_password_reset_tokens_table.py (1)
24-36: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSchema matches the
PasswordResetTokenmodel contract.Columns, FK cascade, unique constraint on
token_hash, and theuser_idindex all align withsrc/ai_trading_discipline_copilot/models/password_reset_token.py.Consider adding an index on
expires_atto support the cleanup job that periodically deletes/queries expired tokens (per PR objectives: "automatic cleanup of expired verification and reset tokens"). Without it, cleanup queries filtering byexpires_at < now()will require a full table scan as the table grows.♻️ Suggested index addition
op.create_index(op.f('ix_password_reset_tokens_user_id'), 'password_reset_tokens', ['user_id'], unique=False) + op.create_index(op.f('ix_password_reset_tokens_expires_at'), 'password_reset_tokens', ['expires_at'], unique=False)🤖 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 `@alembic/versions/3ead82c29db6_add_password_reset_tokens_table.py` around lines 24 - 36, The `password_reset_tokens` schema already matches the model, but the cleanup path will be slow without an `expires_at` index. Update the `create_table` migration for `password_reset_tokens` to add an index on `expires_at` alongside the existing `op.create_index` for `user_id`, so the expired-token cleanup job can efficiently filter by `expires_at`. Keep the change within the Alembic migration that defines `op.create_table` for `password_reset_tokens`.
🤖 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 `@src/ai_trading_discipline_copilot/routers/auth.py`:
- Around line 333-341: Remove the temporary debug endpoint exposed by auth.py’s
test_email route before release. Delete the unauthenticated
`@router.post`("/test-email") handler and its direct EmailService.send_email call,
since it hardcodes a personal address and can be abused to send real emails. If
this endpoint must remain for internal use, protect it with admin authentication
and a debug-only feature flag, and remove the temporary docstring marker.
- Line 171: The refresh/logout/session cookie lookups in auth.py are still
hardcoded to "refresh_token" instead of using the shared _REFRESH_COOKIE_NAME
constant. Update the relevant request.cookies.get usages in the refresh_token
handling paths to reference _REFRESH_COOKIE_NAME so settings.cookie_name changes
do not break AuthRouter behavior.
In `@src/ai_trading_discipline_copilot/services/email_service.py`:
- Around line 17-33: The send_email method in EmailService is doing a
synchronous Resend request inside an async function, which blocks the event
loop. Update the EmailService.send_email path to use the async Resend API by
awaiting resend.Emails.send_async with the same payload, and ensure the project
dependency includes the async extra for Resend so the async client is available.
---
Nitpick comments:
In `@alembic/versions/3ead82c29db6_add_password_reset_tokens_table.py`:
- Around line 24-36: The `password_reset_tokens` schema already matches the
model, but the cleanup path will be slow without an `expires_at` index. Update
the `create_table` migration for `password_reset_tokens` to add an index on
`expires_at` alongside the existing `op.create_index` for `user_id`, so the
expired-token cleanup job can efficiently filter by `expires_at`. Keep the
change within the Alembic migration that defines `op.create_table` for
`password_reset_tokens`.
In `@alembic/versions/526b088ddfed_add_email_verification_tokens_table.py`:
- Around line 24-36: The email verification token migration is missing the same
`expires_at` index used by the password reset tokens table, so add an index for
`expires_at` in the `op.create_table` flow for `email_verification_tokens`
alongside the existing `user_id` index. Update the migration so cleanup queries
can efficiently target expired rows, using the existing `op.create_index`
pattern and the `email_verification_tokens` table definition.
In `@src/ai_trading_discipline_copilot/core/config.py`:
- Around line 87-90: The database URL validation in config.py is using a broad
except Exception around TypeAdapter(PostgresDsn).validate_python, which should
be narrowed. Update the try/except in the database URL parsing logic to catch
the specific Pydantic validation error type raised by validate_python, such as
pydantic.ValidationError or pydantic_core.ValidationError, while keeping the
same ValueError re-raise behavior and message formatting.
- Around line 59-68: The new email and frontend config fields in the settings
model are currently accepted without validation, so add basic checks alongside
the existing config validation in the Config class. Ensure `resend_api_key`,
`email_from`, and `frontend_url` cannot be empty or blank, and
normalize/validate `frontend_url` so it does not keep a trailing slash that
would break generated links. Reuse the same settings validation pattern used for
`secret_key`, and keep the checks attached to the relevant fields in
`core/config.py` so startup fails fast on bad env values.
In `@src/ai_trading_discipline_copilot/models/__init__.py`:
- Around line 8-13: The __all__ list in the models package is not sorted,
triggering Ruff RUF022. Reorder the exported names in __all__ in alphabetical
order in the __init__ module so the static analysis rule passes, keeping the
existing symbols User, RefreshToken, PasswordResetToken, and
EmailVerificationToken.
In `@src/ai_trading_discipline_copilot/models/password_reset_token.py`:
- Around line 49-53: The password reset token TTL is hardcoded twice, once in
PasswordResetToken.expires_at and again in
PasswordResetService.create_reset_token, which can drift over time. Extract the
15-minute value into a shared constant such as PASSWORD_RESET_TOKEN_TTL and use
it in both the model default and the service’s token creation logic so they stay
consistent.
In `@src/ai_trading_discipline_copilot/routers/auth.py`:
- Around line 344-457: The password reset email template is duplicated in
send_reset_email_task instead of living in EmailService like the verification
email. Move the HTML/template-building logic into a dedicated
EmailService.send_password_reset_email method, using the same pattern as
send_verification_email, and have send_reset_email_task only prepare inputs and
call that service method. Keep the escaping, subject, and footer/year handling
inside EmailService so email rendering stays centralized and consistent.
In `@src/ai_trading_discipline_copilot/schemas/email_verification.py`:
- Around line 6-9: Align VerifyEmailRequest.token with
ResetPasswordRequest.token by adding the same minimum length validation used for
the shared token format generated by secrets.token_urlsafe(32). Update the
VerifyEmailRequest model in email_verification.py so it rejects too-short
malformed tokens early, keeping its validation consistent with
ResetPasswordRequest and the rest of the token-based flow.
In `@src/ai_trading_discipline_copilot/services/__init__.py`:
- Around line 7-11: The __all__ export list in the services package is not in
the sorted order expected by Ruff (RUF022). Update the __all__ definition in the
__init__ module so the exported names are ordered alphabetically using
isort-style sorting, keeping the existing symbols UserService,
PasswordResetService, and EmailVerificationService in the corrected order.
In `@src/ai_trading_discipline_copilot/services/email_service.py`:
- Around line 42-43: The imports in send_verification_email are being done
locally instead of at module scope. Move the html import and the UTC/datetime
import to the top-level import section in email_service.py so
EmailService.send_verification_email uses the shared module imports rather than
re-importing on every call.
- Around line 26-33: The Resend outbound send in
send_email/send_verification_email has no exception handling, so BackgroundTasks
failures can be swallowed and never surfaced. Wrap the resend.Emails.send call
in try/except inside the email service methods, add structured logging with the
exception details and recipient/subject context, and make sure the failure path
is observable (and optionally retried) instead of silently succeeding. Use the
send_email and send_verification_email symbols to update both call sites
consistently.
In `@src/ai_trading_discipline_copilot/services/email_verification_service.py`:
- Around line 26-203: `EmailVerificationService` is duplicating the same token
lifecycle logic as `PasswordResetService`, so extract the shared behavior into a
reusable `TokenService`-style base/helper that owns `generate_token`,
`hash_token`, token creation/rotation, `validate_token`, and
`cleanup_expired_tokens`. Parameterize the shared logic by the token model,
expiry duration, and log message context, then make `EmailVerificationService`
and `PasswordResetService` delegate to it so only the
email-specific/user-specific pieces remain.
In `@tests/conftest.py`:
- Around line 123-133: The test-only User init hook is masking real default
behavior by forcing is_verified=True for every User() unless explicitly passed.
Remove the blanket event listener in conftest and make is_verified’s intended
default explicit in the User model/column definition (or update fixtures to pass
it where needed) so production and tests behave consistently. Also move the
sqlalchemy event and User imports into the top-level import section of conftest
for readability.
In `@tests/test_password_reset.py`:
- Around line 25-37: The test_user fixture duplicates the user creation logic
already present in unverified_user, so extract the shared setup into a reusable
parametrized user factory in conftest.py. Update both test_password_reset and
test_email_verification to use that shared fixture/factory, keeping only the
differing fields like username, email, and is_verified in the per-test
parameters.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d2104d42-d812-4c96-91fc-c032b62565d5
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
alembic/versions/3ead82c29db6_add_password_reset_tokens_table.pyalembic/versions/526b088ddfed_add_email_verification_tokens_table.pydocker-compose.ymlpyproject.tomlsrc/ai_trading_discipline_copilot/core/config.pysrc/ai_trading_discipline_copilot/models/__init__.pysrc/ai_trading_discipline_copilot/models/email_verification_token.pysrc/ai_trading_discipline_copilot/models/password_reset_token.pysrc/ai_trading_discipline_copilot/models/user.pysrc/ai_trading_discipline_copilot/routers/auth.pysrc/ai_trading_discipline_copilot/schemas/__init__.pysrc/ai_trading_discipline_copilot/schemas/email_verification.pysrc/ai_trading_discipline_copilot/schemas/password_reset.pysrc/ai_trading_discipline_copilot/services/__init__.pysrc/ai_trading_discipline_copilot/services/auth_service.pysrc/ai_trading_discipline_copilot/services/email_service.pysrc/ai_trading_discipline_copilot/services/email_verification_service.pysrc/ai_trading_discipline_copilot/services/password_reset_service.pysrc/ai_trading_discipline_copilot/services/user_service.pytests/conftest.pytests/test_email_verification.pytests/test_password_reset.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ai_trading_discipline_copilot/routers/auth.py (1)
113-123: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDrop the token-persistence concern; add failure logging around the background email send.
create_verification_token()already commits before returning, so the token is durable here. The remaining issue is thatbackground_tasks.add_task(EmailService.send_verification_email, ...)has no local error handling, so a Resend failure can still leave the user with no verification email and no application-level signal.🤖 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 `@src/ai_trading_discipline_copilot/routers/auth.py` around lines 113 - 123, The verification token persistence concern can be ignored here because create_verification_token already commits; the real gap is that the email send via background_tasks.add_task in the auth.register flow has no failure logging. Add local error handling around the EmailService.send_verification_email background task setup/dispatch path so any Resend/send failure is captured and logged with user/email context, while keeping the existing registration and token creation flow in place.
🤖 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.
Outside diff comments:
In `@src/ai_trading_discipline_copilot/routers/auth.py`:
- Around line 113-123: The verification token persistence concern can be ignored
here because create_verification_token already commits; the real gap is that the
email send via background_tasks.add_task in the auth.register flow has no
failure logging. Add local error handling around the
EmailService.send_verification_email background task setup/dispatch path so any
Resend/send failure is captured and logged with user/email context, while
keeping the existing registration and token creation flow in place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 708eca42-3d50-427c-bb18-dca62e7141b3
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
pyproject.tomlsrc/ai_trading_discipline_copilot/routers/auth.pysrc/ai_trading_discipline_copilot/services/email_service.pytests/conftest.py
🚧 Files skipped from review as they are similar to previous changes (3)
- pyproject.toml
- src/ai_trading_discipline_copilot/services/email_service.py
- tests/conftest.py
2 participants
Summary
This PR introduces a complete production-ready email authentication workflow for the AI Trading Discipline Copilot.
The authentication system now supports secure email verification, password recovery, refresh token session management, and modern email delivery using Resend.
✨ Features Added
Email Verification
Password Recovery
Email Integration
Authentication Improvements
🛡️ Security Improvements
🧪 Testing
Manual Testing
Automated Testing
Files Added
Notes
This PR completes the production-ready authentication email workflow using Resend while keeping the architecture modular and aligned with the existing authentication system.
Summary by CodeRabbit
New Features
Bug Fixes
Chores