mailbox-aware OIDC 인증 기반 및 메일함 스코프 확장 - #199
Conversation
Add mailbox-scoped dashboard, auth, execution queue, and mail-account infrastructure while closing trusted-header and outbound mail SSRF gaps for the staged OIDC integration path.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Note
|
| Layer / File(s) | Summary |
|---|---|
Auth and runtime config backend/api/auth.py, backend/core/config.py, backend/api/runtime_config.py, backend/tests/auth_helpers.py, backend/tests/conftest.py |
Adds AUTH_MODE and OIDC settings; implements JWT/JWKS decoding, cached JWKS client, updates get_auth_context/get_current_user to use bearer token, and exposes runtime flags including manual_bearer_login_enabled. Test helpers and autouse fixture added. |
DB models and bootstrap/backfill backend/db/models.py, backend/scripts/bootstrap_db.py |
Adds MailboxAccount and ExecutionItem models, scopes uniqueness to organization/user/mailbox, adds Email.user_id and mailbox_account_id, and parameterizes bootstrap backfill with legacy IDs. |
Mail server security & routing backend/services/mail_server_security.py, backend/services/mailbox_routing.py |
Adds host/port validation, DNS resolution safety checks, MailServerConnectTarget, and mailbox-account resolution helpers for routing imports/sends. |
Email client & workers backend/services/email_client.py, backend/services/imap_worker.py, backend/services/pop3_worker.py |
Send path now validates/resolves SMTP target and uses pinned-connect clients; IMAP/POP3 workers reworked to sync per MailboxAccount with pinned SSL clients and DNS re-resolution. |
Import fixtures & bootstrap wiring backend/import_fixtures.py, backend/scripts/import_fixtures.py |
ZIP/.eml importer resolves mailbox_account_id, upgrades legacy NULL-mailbox rows when matched, and uses conditional upsert conflict targeting. |
App wiring & deps backend/main.py, backend/requirements.txt |
Wires new routers, starts/stops IMAP+POP3 workers in lifespan, and adds PyJWT dependency. |
APIs and services
| Layer / File(s) | Summary |
|---|---|
Emails API & mailbox scope backend/api/emails.py, backend/api/mailbox_scope.py |
Add mailbox_account_id to list/detail/thread responses, enforce ownership via require_owned_mailbox_account, and select SMTP creds from MailboxAccount (or TenantConfig fallback). |
Mailbox accounts API backend/api/mailbox_accounts.py |
CRUD endpoints for mailbox accounts with sanitization, host/port validation, default-reply enforcement, secret masking, and error mapping (409/503). |
Execution items API backend/api/execution_items.py |
New router providing list, queue-from-email (idempotent/upsert), and patch-to-done endpoints with schemas. |
Search, network, prompts backend/api/search.py, backend/api/network.py, backend/api/prompts.py |
Mailbox-scoped search and reply-count subquery, network graph filtered by Email.user_id, prompts refactored to AuthContext/org-scoped behavior and workspace-admin enforcement for prompt testing. |
LLM providers & runtime backend/api/llm_providers.py, backend/api/llm.py |
Providers scoped to organization with require_provider_admin gate and IntegrityError handling; LLM endpoints preserve behavior and format changes. |
Calendar sync backend/api/calendar.py |
Sync endpoint now depends on server-side credential lookup (get_calendar_credentials_for_user) and returns 503 when missing; client user_token is ignored. |
Frontend pages, client, and execution queue
| Layer / File(s) | Summary |
|---|---|
Api client & runtime-config frontend/src/lib/api-client.ts, frontend/src/lib/runtime-config.ts |
Refactor ApiClient to use bearer token in localStorage, decode claims, gate dev-header emission via runtime-config; add runtime-config client and tests; add manual bearer login UI. |
Execution queue & UI integration frontend/src/lib/execution-queue.ts, frontend/src/components/EmailList.tsx, frontend/src/app/ai-hub/actions/page.tsx, frontend/src/components/EmailDetail.tsx |
Client execution-queue persisted per-user/org or volatile when authenticated; EmailList supports swipe enqueue/complete, mailbox filter, and integrates ExecutionItem APIs; AI Hub actions page and EmailDetail adjusted for mailbox context. |
| Pages and components many files under frontend/src/app/* and frontend/src/components/* |
Add/modify numerous pages (ai-hub context/decisions/actions, compose client/server, compose tests, labels/projects/all/sent/drafts/starred placeholders), DashboardLayout insight and scroll restoration, Settings mailbox accounts UI, PromptStudio access gating, and many tests. |
| Tests & e2e helpers many frontend/tests/*, frontend/tests/e2e/helpers.ts |
Extensive Vitest suite additions and Playwright e2e mocks for runtime-config and execution-items, plus many component tests updated for mailbox-aware behavior. |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- Seongho-Bae/naruon#191: Related auth/role-scoping refactors; overlaps in AuthContext and RBAC wiring.
- Seongho-Bae/naruon#134: Overlaps frontend navigation and DashboardLayout/inbox presentation changes.
- Seongho-Bae/naruon#147: Related auth and email client/send-path changes.
"A rabbit hops with a tiny cheer,
Tokens signed, and inboxes clear.
Mailboxes routed, queues take flight,
Workers hum through day and night.
Hooray — small hops, big change in sight!" 🐇✨
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
feature/oidc-integration-20260513
1 similar comment
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| async with lifespan(FastAPI()): | ||
| raise RuntimeError("app failed") | ||
|
|
||
| imap_stop.assert_awaited_once() |
Greptile Summarymailbox-aware OIDC 인증 기반과 개인 메일함 계정 레지스트리를 추가하고, owner/mailbox 범위의 email/search/thread API 및 execution item queue를 도입하는 대규모 PR입니다. 전반적인 방향성은 명확하나, 이전 리뷰에서 지적된 다수의 미해결 항목이 아직 남아 있습니다.
Confidence Score: 3/5이전 리뷰에서 지적된 여러 결함(SMTP TLS 분기 누락, 동기 DNS 블로킹, _clear_default_reply 경쟁 조건, hybrid 모드 폴백, prompt_templates 마이그레이션 누락 등)이 아직 수정되지 않아 운영 환경 배포 전 추가 작업이 필요합니다. 이번 리뷰에서 신규로 발견된 backend/api/emails.py ( Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Client Request] --> B{Authorization header present?}
B -- No --> C[401 Authentication Required]
B -- Yes --> D{AUTH_MODE}
D -- oidc or hybrid --> E[jwt.get_unverified_header]
E --> F{OIDC_JWKS_URL set?}
F -- Yes --> G[RS256: asyncio.to_thread JWKS fetch]
F -- No --> H[HS256: shared secret]
G --> I[jwt.decode algorithms RS256]
H --> J[jwt.decode algorithms HS256]
I --> K[build AuthContext from claims]
J --> K
K --> L[API handler]
L --> M{mailbox_account_id filter?}
M -- Yes --> N[require_owned_mailbox_account]
M -- No --> O[user_id scoped query]
N --> O
O --> P[Return response]
Reviews (11): Last reviewed commit: "fix: fail closed on missing OIDC verifie..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| def build_reply_counts_subquery( | ||
| current_user: str, mailbox_account_id: int | None = None | ||
| ): |
There was a problem hiding this comment.
mailbox_account_id IS NULL인 행도 포함
mailbox_scope_predicate는 OR mailbox_account_id IS NULL 조건을 추가하기 때문에, mailbox_account_id=1로 검색하면 계정 1의 메일과 레거시(소유 계정 미할당) 메일이 함께 반환됩니다. 반면 GET /api/emails?mailbox_account_id=1 엔드포인트는 동일 파라미터에 등호 조건만 사용하므로, 두 API의 필터링 결과가 일치하지 않습니다. 사용자가 특정 메일함으로 검색·필터링할 때 다른 메일함의 레거시 메일이 섞여 나올 수 있습니다.
| if port not in allowed_ports: | ||
| raise MailServerValidationError( | ||
| f"{label} 포트는 허용된 메일 포트만 사용할 수 있습니다." | ||
| ) | ||
| return normalized_host | ||
|
|
||
|
|
||
| def _resolve_safe_mail_server_ips(host: str, port: int) -> list[str]: | ||
| try: | ||
| resolved_addresses = socket.getaddrinfo(host, port, type=socket.SOCK_STREAM) | ||
| except socket.gaierror: | ||
| return [] | ||
|
|
||
| safe_ips: list[str] = [] | ||
| for *_, sockaddr in resolved_addresses: | ||
| resolved_ip = sockaddr[0] |
There was a problem hiding this comment.
_resolve_safe_mail_server_ips는 표준 라이브러리의 socket.getaddrinfo를 동기적으로 호출하는데, 이 함수는 호스트명 해석 결과가 나올 때까지 블로킹됩니다. 이 함수는 validate_mail_server_host → _validate_mailbox_payload를 통해 create_mailbox_account·update_mailbox_account async 핸들러에서 직접 호출됩니다. DNS 응답이 수백 ms ~ 수 초 지연되는 경우 전체 asyncio 이벤트 루프가 정지되어 다른 모든 요청이 응답을 받지 못하게 됩니다. asyncio.to_thread(socket.getaddrinfo, ...) 패턴이나 비동기 DNS 라이브러리(aiodns 등)로 감싸야 합니다.
|
|
||
| async def _get_account_or_404( | ||
| db: AsyncSession, current_user: str, account_id: int | ||
| ) -> MailboxAccount: | ||
| result = await db.execute( | ||
| select(MailboxAccount).where( | ||
| MailboxAccount.id == account_id, MailboxAccount.user_id == current_user | ||
| ) | ||
| ) | ||
| account = result.scalar_one_or_none() | ||
| if not account: | ||
| raise HTTPException(status_code=404, detail="Mailbox account not found") | ||
| return account | ||
|
|
||
|
|
||
| async def _clear_default_reply(db: AsyncSession, current_user: str) -> None: | ||
| result = await db.execute( | ||
| select(MailboxAccount).where(MailboxAccount.user_id == current_user) |
There was a problem hiding this comment.
_clear_default_reply 경쟁 조건 — DB 수준 유일성 보장 없음
_clear_default_reply는 해당 유저의 모든 계정을 SELECT한 뒤 Python 레이어에서 is_default_reply = False로 바꾸고, 이후 새 계정에 is_default_reply = True를 설정합니다. 두 개의 요청이 동시에 도달하면 양쪽 다 clear를 완료한 후 각자의 계정을 default로 설정해 커밋할 수 있어, 두 계정이 동시에 is_default_reply = True인 상태가 됩니다. DB 스키마에 partial unique index ON mailbox_accounts (user_id) WHERE is_default_reply = true를 추가하거나, UPDATE ... SET is_default_reply = FALSE WHERE user_id = :uid 단일 문으로 교체하면 원자적으로 처리할 수 있습니다.
| else: | ||
| item = ExecutionItem( | ||
| user_id=auth_context.user_id, | ||
| organization_id=auth_context.organization_id, | ||
| workspace_id=auth_context.workspace_id, | ||
| source_mailbox_account_id=email.mailbox_account_id, | ||
| source_email_id=email.id, | ||
| source_thread_id=email.thread_id, | ||
| source_message_id=email.message_id, | ||
| source_snippet=email.body[:200] + ("..." if len(email.body) > 200 else ""), | ||
| title=email.subject or "(제목 없음)", | ||
| sender=email.sender, | ||
| status="queued", | ||
| created_at=now, | ||
| updated_at=now, | ||
| ) | ||
| db.add(item) |
There was a problem hiding this comment.
ExecutionItem 생성 시 _snapshot_email_fields 미사용으로 로직 중복
_snapshot_email_fields 헬퍼가 스냅샷 필드 설정 책임을 집중하기 위해 정의되어 있음에도, 새 아이템 생성 경로에서는 같은 로직(email.body[:200] + "..." 등)을 직접 인라인으로 반복하고 있습니다. 이후 snippet 길이나 필드 목록이 변경될 경우 두 곳을 모두 수정해야 하는데 한 곳을 빠뜨리기 쉽습니다. 새 ExecutionItem 생성 후 _snapshot_email_fields(item, email)을 호출하는 방식으로 통일하는 것을 권장합니다.
| else: | |
| item = ExecutionItem( | |
| user_id=auth_context.user_id, | |
| organization_id=auth_context.organization_id, | |
| workspace_id=auth_context.workspace_id, | |
| source_mailbox_account_id=email.mailbox_account_id, | |
| source_email_id=email.id, | |
| source_thread_id=email.thread_id, | |
| source_message_id=email.message_id, | |
| source_snippet=email.body[:200] + ("..." if len(email.body) > 200 else ""), | |
| title=email.subject or "(제목 없음)", | |
| sender=email.sender, | |
| status="queued", | |
| created_at=now, | |
| updated_at=now, | |
| ) | |
| db.add(item) | |
| else: | |
| item = ExecutionItem( | |
| user_id=auth_context.user_id, | |
| organization_id=auth_context.organization_id, | |
| workspace_id=auth_context.workspace_id, | |
| status="queued", | |
| created_at=now, | |
| updated_at=now, | |
| ) | |
| _snapshot_email_fields(item, email) | |
| db.add(item) |
![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
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/api/llm.py (1)
39-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-raise
HTTPExceptionbefore the catch-all.The 400 raised on Line 44 and Line 76 is currently swallowed by
except Exceptionand returned as 500, so a missing API key looks like an internal error instead of a client/config error.Suggested fix
except LLMServiceError: raise HTTPException( status_code=500, detail="An internal server error occurred while processing the request.", ) + except HTTPException: + raise except Exception: raise HTTPException( status_code=500, detail="An internal server error occurred while processing the request.", )Apply the same change to both endpoint handlers.
Also applies to: 71-92
🤖 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 `@backend/api/llm.py` around lines 39 - 57, The HTTPException raised when tenant_config is missing or missing openai_api_key is being swallowed by the broad exception handlers; update the try/except blocks in the endpoint handlers (the blocks around the tenant_config lookup and call to extract_todos_and_summary) to explicitly re-raise HTTPException before the generic excepts — i.e., add an except HTTPException: raise to pass through client errors, keep the existing except LLMServiceError and the general except Exception to handle server errors; make the same change in both handler blocks that perform the tenant_config lookup and call extract_todos_and_summary.backend/scripts/import_fixtures.py (1)
147-148:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollback the session if the final commit fails.
If
session.commit()raises here, the session stays in an aborted transaction state and the rest of the import run cannot recover cleanly. Wrap this intry/except, callrollback(), then re-raise or return.Suggested fix
- await session.commit() + try: + await session.commit() + except Exception: + await session.rollback() + raise logger.info(f"Finished processing {zip_path}")🤖 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 `@backend/scripts/import_fixtures.py` around lines 147 - 148, Wrap the final await session.commit() and subsequent logger.info(f"Finished processing {zip_path}") in a try/except block so that if commit raises you call await session.rollback() and then re-raise the exception (or return) to avoid leaving the session in an aborted state; specifically modify the block that uses session.commit(), session.rollback(), and logger.info with zip_path so the exception path always rolls back the session before propagating the error.backend/tests/test_import_fixtures.py (1)
67-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProtect
settings.LEGACY_EMAIL_OWNER_USER_IDoverrides with guaranteed cleanup.These tests mutate global settings but restore without
finally. If an await/patch/assertion fails early, the leaked value can poison later tests.Suggested fix pattern
- previous_owner = settings.LEGACY_EMAIL_OWNER_USER_ID - settings.LEGACY_EMAIL_OWNER_USER_ID = "testuser" - ... - imported = await import_fixtures.import_eml_file(session, eml_file) - settings.LEGACY_EMAIL_OWNER_USER_ID = previous_owner + previous_owner = settings.LEGACY_EMAIL_OWNER_USER_ID + settings.LEGACY_EMAIL_OWNER_USER_ID = "testuser" + try: + ... + imported = await import_fixtures.import_eml_file(session, eml_file) + finally: + settings.LEGACY_EMAIL_OWNER_USER_ID = previous_ownerAlso applies to: 136-147, 201-215, 234-237, 295-309, 385-398
🤖 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 `@backend/tests/test_import_fixtures.py` around lines 67 - 80, The test overwrites the global settings.LEGACY_EMAIL_OWNER_USER_ID but restores it only after the await/patch block, risking leakage if an exception occurs; wrap the mutation in a try/finally (or use contextmanager) so the original value is always restored—move settings.LEGACY_EMAIL_OWNER_USER_ID = previous_owner into a finally block that encloses the patch.object(...)/await import_fixtures.import_eml_file(...) section and apply the same pattern to the other affected test blocks (lines referencing settings.LEGACY_EMAIL_OWNER_USER_ID and import_fixtures.import_eml_file).
🟠 Major comments (20)
backend/db/models.py-281-293 (1)
281-293:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftEnforce a single default mailbox per user
is_default_replyhas no DB-level invariant, so multiple rows can become default for the same user. That makes default credential resolution ambiguous and risks sending from the wrong mailbox.Please enforce this invariant (e.g., partial unique index on
user_id WHERE is_default_reply = trueplus migration/backfill handling).backend/services/email_client.py-129-135 (1)
129-135:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid masking primary SMTP failures in cleanup
await smtp.quit()infinallycan raise and overwrite the original failure fromconnect()/send_message(). Preserve the primary exception and treat quit errors as best-effort cleanup.Suggested fix
try: await smtp.connect() await smtp.send_message(message) finally: if smtp.is_connected: - await smtp.quit() + try: + await smtp.quit() + except Exception: + logger.warning("SMTP quit failed after send attempt", exc_info=True)🤖 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 `@backend/services/email_client.py` around lines 129 - 135, The current finally block may let await smtp.quit() raise and mask an earlier failure from await smtp.connect() or await smtp.send_message(); modify the flow so the primary exception is preserved: wrap connect/send_message in a try/except that captures the primary exception, then in finally check smtp.is_connected and call await smtp.quit() inside its own try/except that logs/suppresses any quit error (do not re-raise), and after cleanup re-raise the captured primary exception if present; reference the smtp object and its methods connect, send_message, quit, is_connected, and the message variable to locate the code to change.backend/services/mail_server_security.py-72-76 (1)
72-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
validate_mail_server_hostcurrently accepts unresolved hostsIf DNS resolution fails,
_resolve_safe_mail_server_ipsreturns an empty list and Line 75 doesn’t reject it. This allows invalid host config to persist and shifts failure to runtime.Suggested fix
def validate_mail_server_host(prefix: str, label: str, host: str, port: int) -> str: """Normalize and validate a mail server for persistence-time API checks.""" normalized_host = _validate_mail_server_syntax_and_port(prefix, label, host, port) - _resolve_safe_mail_server_ips(normalized_host, port) + safe_ips = _resolve_safe_mail_server_ips(normalized_host, port) + if not safe_ips: + raise MailServerValidationError(f"{label} 서버 주소를 확인할 수 없습니다.") return normalized_host🤖 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 `@backend/services/mail_server_security.py` around lines 72 - 76, The function validate_mail_server_host currently calls _resolve_safe_mail_server_ips(normalized_host, port) but ignores its return value so DNS failures (empty list) are allowed; modify validate_mail_server_host to capture the returned IP list from _resolve_safe_mail_server_ips and if it is empty raise a ValueError (or a specific config/validation exception used elsewhere) with a clear message including prefix/label/host/port, otherwise return the normalized_host; keep the initial syntax check via _validate_mail_server_syntax_and_port and use the same exception type/pattern used across the module for consistency.backend/api/network.py-39-43 (1)
39-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound
limitto prevent oversized graph queries.Line 39 accepts client-controlled
limitwithout a hard cap. Very large values can degrade DB/query latency and in-memory graph construction.Suggested patch
-from fastapi import APIRouter, Depends, HTTPException +from fastapi import APIRouter, Depends, HTTPException, Query @@ - limit: int = 500, + limit: int = Query(default=500, ge=1, le=2000),🤖 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 `@backend/api/network.py` around lines 39 - 43, The endpoint currently accepts a client-controlled limit parameter (signature with limit: int = 500, user_id: str | None = None, db: AsyncSession = Depends(get_db), current_user: str = Depends(get_current_user)) with no hard cap; add a MAX_LIMIT constant (e.g., MAX_NETWORK_LIMIT) and enforce it at the start of the handler by validating/clamping the incoming limit (or returning an HTTPException/400 when limit > MAX_LIMIT) to prevent oversized DB queries and in-memory graph construction, and ensure the documentation/response uses the validated value thereafter.frontend/src/app/login/page.tsx-40-45 (1)
40-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMask and normalize bearer token input before persistence.
The token is entered as visible plain text and persisted without trimming, which increases credential exposure risk and can save invalid whitespace-padded tokens.
🔐 Proposed fix
- if (!token.trim()) { + const normalizedToken = token.trim(); + if (!normalizedToken) { setMessage('Bearer 토큰을 입력하세요.'); return; } - apiClient.setBearerToken(token); + apiClient.setBearerToken(normalizedToken);- <Input value={token} onChange={(e) => setToken(e.target.value)} placeholder="eyJhbGciOi..." /> + <Input + type="password" + autoComplete="off" + spellCheck={false} + value={token} + onChange={(e) => setToken(e.target.value)} + placeholder="eyJhbGciOi..." + />Also applies to: 68-69
🤖 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 `@frontend/src/app/login/page.tsx` around lines 40 - 45, Trim and normalize the bearer token before passing it to apiClient.setBearerToken and avoid persisting the raw token in component state; specifically, in the handler that currently reads token and calls apiClient.setBearerToken(token) use a normalized value (const normalized = token.trim()), call apiClient.setBearerToken(normalized), then immediately clear the visible token input via setToken('') and, if you need to show a saved indicator, store only a masked placeholder (e.g., '••••••') or a boolean flag rather than the raw token; apply the same change to the other occurrence around the code referenced at lines 68-69 so all token updates use normalized values and do not retain plain-text tokens in state.frontend/src/app/ai-hub/context/page.tsx-14-16 (1)
14-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
useSearchParamsinstead ofwindow.location.searchin render.Reading
window.location.searchduring render causes hydration mismatches in App Router pages—the server renderswindowas undefined while the client renders the actual value, creating text content mismatches.🔧 Proposed fix
import { useEffect, useMemo, useState } from 'react'; +import { useSearchParams } from 'next/navigation'; @@ - const searchQuery = typeof window === 'undefined' - ? '' - : (new URLSearchParams(window.location.search).get('q') || '').trim(); + const searchParams = useSearchParams(); + const searchQuery = useMemo( + () => (searchParams.get('q') || '').trim(), + [searchParams], + );Note: Wrap the component using
useSearchParamsin a<Suspense>boundary, as it suspends in statically rendered routes.🤖 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 `@frontend/src/app/ai-hub/context/page.tsx` around lines 14 - 16, The code reads window.location.search during render to compute searchQuery, causing hydration mismatches; replace that logic with Next.js App Router's useSearchParams hook (import and call useSearchParams() and derive q via searchParams.get('q')?.trim() in the component) and remove direct window access; because useSearchParams can suspend in statically rendered routes, wrap the component (or the part that uses useSearchParams) in a React.Suspense boundary with a fallback to avoid runtime suspensions.backend/tests/test_runtime_config_api.py-27-42 (1)
27-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore mutated global settings in
finallyto prevent cross-test leakage.If
client.get(...)or interim code throws before restoration, later tests inherit dirty auth/OIDC config and become order-dependent.Suggested fix
def test_runtime_config_exposes_non_secret_auth_capability_flags(client): previous_auth_mode = settings.AUTH_MODE previous_trust = settings.TRUST_DEV_HEADERS previous_secret = settings.OIDC_SHARED_SECRET previous_jwks = settings.OIDC_JWKS_URL - settings.AUTH_MODE = "header" - settings.TRUST_DEV_HEADERS = True - settings.OIDC_SHARED_SECRET = None - settings.OIDC_JWKS_URL = None - - response = client.get("/api/runtime-config") - - settings.AUTH_MODE = previous_auth_mode - settings.TRUST_DEV_HEADERS = previous_trust - settings.OIDC_SHARED_SECRET = previous_secret - settings.OIDC_JWKS_URL = previous_jwks + try: + settings.AUTH_MODE = "header" + settings.TRUST_DEV_HEADERS = True + settings.OIDC_SHARED_SECRET = None + settings.OIDC_JWKS_URL = None + response = client.get("/api/runtime-config") + finally: + settings.AUTH_MODE = previous_auth_mode + settings.TRUST_DEV_HEADERS = previous_trust + settings.OIDC_SHARED_SECRET = previous_secret + settings.OIDC_JWKS_URL = previous_jwksApply the same
try/finallypattern to the second test that mutates OIDC fields.Also applies to: 52-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 `@backend/tests/test_runtime_config_api.py` around lines 27 - 42, The test mutates global settings (settings.AUTH_MODE, settings.TRUST_DEV_HEADERS, settings.OIDC_SHARED_SECRET, settings.OIDC_JWKS_URL) and restores them only after client.get("/api/runtime-config"), which can leak state if an exception occurs; wrap the mutation + request in a try/finally block so restoration always runs, and apply the same try/finally pattern to the other test that mutates OIDC fields (the one around lines 52-73) to ensure global settings are always restored.frontend/src/app/page.tsx-109-114 (1)
109-114:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
refreshExecutionQueue()failures and unmounts in the effect.The current fire-and-forget chain can emit unhandled rejections and may update state after unmount.
Suggested fix
useEffect(() => { + let active = true; const syncExecutionQueue = () => setExecutionQueue(listExecutionQueue()); syncExecutionQueue(); - void refreshExecutionQueue().then(setExecutionQueue); - return subscribeExecutionQueue(syncExecutionQueue); + void refreshExecutionQueue() + .then((items) => { + if (active) setExecutionQueue(items); + }) + .catch(() => { + if (active) syncExecutionQueue(); + }); + const unsubscribe = subscribeExecutionQueue(syncExecutionQueue); + return () => { + active = false; + unsubscribe(); + }; }, []);🤖 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 `@frontend/src/app/page.tsx` around lines 109 - 114, The effect calls refreshExecutionQueue() fire-and-forget which can produce unhandled rejections and may call setExecutionQueue after the component unmounts; update the effect in useEffect to (1) track mounted state (or use an AbortController) so you skip calling setExecutionQueue if unmounted, (2) attach a .catch handler to refreshExecutionQueue() to handle and log errors (or ignore them explicitly) to avoid unhandled rejections, and (3) ensure the cleanup returned from useEffect still calls subscribeExecutionQueue unsubscribe but also flips the mounted flag (or aborts) so pending refreshExecutionQueue promises do not update state; reference functions/identifiers: refreshExecutionQueue, syncExecutionQueue, subscribeExecutionQueue, listExecutionQueue, setExecutionQueue, useEffect.frontend/src/app/ai-hub/actions/page.tsx-38-43 (1)
38-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle queue refresh failures explicitly.
refreshExecutionQueue()is fire-and-forget with no.catch, so request failures can surface as unhandled rejections.Suggested fix
useEffect(() => { + let active = true; const syncQueue = () => setQueueItems(listExecutionQueue()); syncQueue(); - void refreshExecutionQueue().then(setQueueItems); - return subscribeExecutionQueue(syncQueue); + void refreshExecutionQueue() + .then((items) => { + if (active) setQueueItems(items); + }) + .catch(() => { + // keep local queue snapshot; optional: report metric/log + }); + const unsubscribe = subscribeExecutionQueue(syncQueue); + return () => { + active = false; + unsubscribe(); + }; }, []);🤖 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 `@frontend/src/app/ai-hub/actions/page.tsx` around lines 38 - 43, The current useEffect calls refreshExecutionQueue() fire-and-forget which can cause unhandled promise rejections; modify the effect so the promise returned by refreshExecutionQueue() is handled — e.g., call refreshExecutionQueue().then(setQueueItems).catch(err => { /* log or set error state */ }) or use an async wrapper to await it and handle errors; ensure the change references the existing syncQueue, setQueueItems, listExecutionQueue, refreshExecutionQueue, and subscribeExecutionQueue functions so failures are explicitly caught and logged or surfaced to UI instead of being unhandled.backend/api/search.py-49-53 (1)
49-53:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe mailbox filter still returns unrelated legacy mail.
mailbox_scope_predicate()widens every scoped query toEmail.mailbox_account_id IS NULL. For users with multiple mailbox accounts, any pre-migration/null rows from other mailboxes will appear in every mailbox-scoped search, and the same rows also inflatereply_count. If you need legacy thread context, only union null-mailbox rows that share a thread key with hits from the requested mailbox.Also applies to: 68-70, 127-159
🤖 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 `@backend/api/search.py` around lines 49 - 53, The current mailbox_scope_predicate(mailbox_account_id) incorrectly includes all Email.mailbox_account_id IS NULL rows; change it so NULL-mailbox rows are only included when their thread_key matches threads that exist in the requested mailbox. Replace the simple OR with a predicate that is either Email.mailbox_account_id == mailbox_account_id OR (Email.mailbox_account_id.is_(None) AND Email.thread_key.in_(subquery)), where subquery selects thread_key from Email filtered by mailbox_account_id; update callers (the other uses noted around the second occurrence and the block handling thread/aggregate queries) to pass the same mailbox_account_id and let the predicate use the thread-key subquery so only legacy null-mailbox rows that share a thread with hits in the target mailbox are included.backend/main.py-76-83 (1)
76-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConsolidate the duplicated CORS middleware.
This block is followed by a second
CORSMiddlewareregistration at Lines 103-109 that only allowshttp://localhost:3000, so the newly added127.0.0.1and:8000origins can still fail preflight depending on which layer handles the request. Keep a single middleware config here.🤖 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 `@backend/main.py` around lines 76 - 83, The project registers CORSMiddleware twice which can cause inconsistent preflight behavior; consolidate into a single app.add_middleware(CORSMiddleware, ...) registration that includes all required origins (e.g., "http://localhost:3000", "http://127.0.0.1:3000", "http://localhost:8000", "http://127.0.0.1:8000") and remove the duplicate registration further down; update the existing CORSMiddleware call (the one near app.add_middleware) rather than adding another, and ensure any other CORS options (allow_methods, allow_headers, allow_credentials) from the removed block are preserved in the single middleware config.backend/api/calendar.py-15-23 (1)
15-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis helper makes
/syncpermanently unavailable.
get_calendar_credentials_for_user()always returnsNone, so Line 33 always raises 503 in real traffic. If the server-side credential store is not ready yet, gate the endpoint or keep a working temporary path instead of shipping a route that cannot succeed.🤖 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 `@backend/api/calendar.py` around lines 15 - 23, get_calendar_credentials_for_user currently always returns None which makes /sync always 503; replace the stub with a real retrieval from the server-side store (e.g., call CalendarCredentialStore.get_credentials(auth_context.user_id) or equivalent) and return the credential dict when present; if the credential store is not yet ready, gate the endpoint by either (a) checking a fallback flag ALLOW_TEMP_CALENDAR_FALLBACK and returning credentials from TEMP_CALENDAR_CREDENTIALS when enabled, or (b) raising a clear ServiceUnavailable/HTTPException(503) so the route fails fast but intentionally, rather than silently forcing a permanent 503 for all traffic. Ensure you reference get_calendar_credentials_for_user, CalendarCredentialStore.get_credentials (or your store API), ALLOW_TEMP_CALENDAR_FALLBACK/TEMP_CALENDAR_CREDENTIALS, and the ServiceUnavailable/HTTPException path so the /sync handler can decide the fallback behavior.backend/services/mailbox_routing.py-10-19 (1)
10-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse RFC-compliant address parsing for mailbox routing.
The current implementation splits on
[,;]and manually extracts angle-bracket contents, which fails to parse valid RFC address-list syntax. For example:
Team: alice@example.com, bob@example.com;→ incorrectly extracts{'bob@example.com', 'team: alice@example.com'}instead of{'bob@example.com', 'alice@example.com'}"Doe, Jane" <jane@example.com>, Bob <bob@example.com>→ incorrectly extracts{'jane@example.com', 'bob@example.com', '"doe'}instead of{'jane@example.com', 'bob@example.com'}When parsing fails, the function falls back to default or sole account selection, routing mail to the wrong mailbox.
Replace with
email.utils.getaddresses, which correctly handles display names, quoted strings, and various separators:Suggested fix
-import re +from email.utils import getaddresses @@ def _extract_mailboxes(value: str | None) -> set[str]: if not value: return set() - mailboxes = set() - for token in re.split(r"[,;]", value): - match = re.search(r"<([^>]+)>", token) - mailbox = (match.group(1) if match else token).strip().lower() - if mailbox: - mailboxes.add(mailbox) - return mailboxes + return { + mailbox.strip().lower() + for _, mailbox in getaddresses([value]) + if mailbox.strip() + }🤖 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 `@backend/services/mailbox_routing.py` around lines 10 - 19, The _extract_mailboxes function currently splits addresses manually and misparses RFC address-lists; replace its tokenization with Python's RFC-compliant parser by using email.utils.getaddresses to parse the input string and collect the lowercased, stripped mailbox parts (the second element of each tuple) into a set; update references inside _extract_mailboxes to ignore empty results and preserve current return semantics (return set() when input falsy), ensuring quoted names and comma/semicolon separators are handled correctly.frontend/src/components/EmailList.tsx-289-289 (1)
289-289:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass the clicked email’s mailbox context, not only the current filter.
At Line 289, forwarding
selectedMailboxAccountIdcan lose the actual mailbox for that email when filter is “all” (or mismatch context), causing downstream actions to use the wrong sender account.Proposed fix
- onSelectEmail(email.id, email, selectedMailboxAccountId); + onSelectEmail( + email.id, + email, + email.mailbox_account_id ?? selectedMailboxAccountId ?? null, + );🤖 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 `@frontend/src/components/EmailList.tsx` at line 289, The call to onSelectEmail currently forwards only selectedMailboxAccountId which can misattribute the clicked email when the view is an "all" filter; change the argument to use the clicked email's own mailbox context (e.g. email.mailboxAccountId) instead of selectedMailboxAccountId, or fallback to selectedMailboxAccountId if the email field is missing (onSelectEmail(email.id, email, email.mailboxAccountId || selectedMailboxAccountId)); update any downstream expectations in onSelectEmail handlers to accept and use that mailbox value.frontend/src/lib/execution-queue.ts-119-123 (1)
119-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDisambiguate lookup to avoid completing the wrong queue item.
At Line 119, matching
sourceEmailId === emailId || id === emailIdin one pass can pick the wrong item when numeric IDs overlap across tables.Proposed fix
- let target = queue.find((item) => item.sourceEmailId === emailId || item.id === emailId) || null; + let target = + queue.find((item) => item.sourceEmailId === emailId) ?? + queue.find((item) => item.id === emailId) ?? + null; if (!target) { queue = await refreshExecutionQueue(); - target = queue.find((item) => item.sourceEmailId === emailId || item.id === emailId) || null; + target = + queue.find((item) => item.sourceEmailId === emailId) ?? + queue.find((item) => item.id === emailId) ?? + null; }🤖 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 `@frontend/src/lib/execution-queue.ts` around lines 119 - 123, The current lookup that combines sourceEmailId === emailId || id === emailId can return the wrong item when numeric ids overlap; change the lookup to first try finding by sourceEmailId only (queue.find(item => item.sourceEmailId === emailId)), and only if that yields null then try by id (queue.find(item => item.id === emailId)); apply the same two-step logic after calling refreshExecutionQueue() so both the initial and refreshed searches prefer sourceEmailId over id; refer to the variables/funcs queue, target, sourceEmailId, id, emailId and refreshExecutionQueue() when making the change.frontend/src/lib/runtime-config.ts-18-24 (1)
18-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear cached promise on fetch failure to allow retry.
At Line 19, a failed
/api/runtime-configrequest leavesruntimeConfigPromisein a permanently rejected state, so future calls cannot recover unlessresetRuntimeConfigCache()is called externally.Proposed fix
export function getRuntimeConfig() { if (!runtimeConfigPromise) { - runtimeConfigPromise = apiClient.get<RuntimeConfig>('/api/runtime-config').then((config) => { - apiClient.setDevHeaderAuthEnabled(config.features.dev_header_auth_enabled); - return config; - }); + runtimeConfigPromise = apiClient + .get<RuntimeConfig>('/api/runtime-config') + .then((config) => { + apiClient.setDevHeaderAuthEnabled(config.features.dev_header_auth_enabled); + return config; + }) + .catch((error) => { + runtimeConfigPromise = null; + throw error; + }); } return runtimeConfigPromise; }🤖 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 `@frontend/src/lib/runtime-config.ts` around lines 18 - 24, The cached promise runtimeConfigPromise can stay permanently rejected after apiClient.get('/api/runtime-config') fails; update the getRuntimeConfig flow so that when apiClient.get (the promise returned inside runtimeConfigPromise) rejects you clear runtimeConfigPromise (set it back to undefined/null) before rethrowing the error so subsequent calls can retry; implement this by attaching a .catch handler to the promise returned by apiClient.get in the block that assigns runtimeConfigPromise and in that handler reset runtimeConfigPromise and rethrow the original error (no external resetRuntimeConfigCache call required).frontend/src/components/EmailList.tsx-294-294 (1)
294-294:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not execute queue/complete actions on pointer cancel.
At Line 294,
onPointerCancelreuseshandlePointerEnd, so canceled gestures can still enqueue/complete items. Cancel should only reset swipe state.Proposed fix
+ const handlePointerCancel = (emailId: number) => (event: React.PointerEvent<HTMLButtonElement>) => { + const currentTarget = event.currentTarget; + setSwipeOffsets((prev) => ({ ...prev, [emailId]: 0 })); + swipeOffsetsRef.current[emailId] = 0; + pointerStateRef.current = null; + if ( + capturedPointerIdsRef.current[emailId] === event.pointerId && + (typeof currentTarget.hasPointerCapture !== 'function' || currentTarget.hasPointerCapture(event.pointerId)) + ) { + currentTarget.releasePointerCapture(event.pointerId); + } + delete capturedPointerIdsRef.current[emailId]; + }; ... - onPointerCancel={handlePointerEnd(email)} + onPointerCancel={handlePointerCancel(email.id)}🤖 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 `@frontend/src/components/EmailList.tsx` at line 294, The pointer-cancel currently calls handlePointerEnd(email) which triggers enqueue/complete actions; instead create a new handler (e.g., handlePointerCancel) that only resets the swipe/drag state (clear dragging flags, startX, translateX/offset, any active pointerId) without performing enqueue/complete logic, and replace onPointerCancel={handlePointerEnd(email)} with onPointerCancel={handlePointerCancel(email)} (or bind appropriately); keep handlePointerEnd(email) for actual pointer up/end logic that commits actions.frontend/src/components/EmailList.test.tsx-39-42 (1)
39-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset
apiClientsingleton state in teardown to avoid cross-test leakage.These tests enable dev-header auth globally but never restore it. That can create suite-order flakiness in other tests.
Suggested fix
afterEach(() => { if (root) { act(() => root?.unmount()); } + apiClient.setBaseUrl(""); + apiClient.setDevHeaderAuthEnabled(false); root = null; container?.remove(); container = null; vi.unstubAllGlobals(); });Also applies to: 44-52
🤖 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 `@frontend/src/components/EmailList.test.tsx` around lines 39 - 42, Tests enable dev-header auth on the apiClient singleton in beforeEach but never restore it, risking cross-test leakage; add an afterEach teardown that resets the singleton state by calling apiClient.setDevHeaderAuthEnabled(false) and apiClient.setBaseUrl('') (or restore any saved original values) so the apiClient global is returned to a clean state after each test.backend/api/mailbox_accounts.py-244-246 (1)
244-246:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate against the effective account state, not only patch fields.
A partial PATCH can break the server/port pairing invariant (e.g.,
smtp_server=nullwith existingsmtp_portretained). Validate using merged state (current account + update) before persisting.Suggested fix pattern
- update_data = _validate_mailbox_payload( - _sanitize_mailbox_payload(payload.model_dump(exclude_unset=True)) - ) + patch_data = _sanitize_mailbox_payload(payload.model_dump(exclude_unset=True)) + merged = { + "email_address": account.email_address, + "display_name": account.display_name, + "provider": account.provider, + "is_default_reply": account.is_default_reply, + "is_active": account.is_active, + "smtp_server": account.smtp_server, + "smtp_port": account.smtp_port, + "smtp_username": account.smtp_username, + "smtp_password": account.smtp_password, + "imap_server": account.imap_server, + "imap_port": account.imap_port, + "imap_username": account.imap_username, + "imap_password": account.imap_password, + "pop3_server": account.pop3_server, + "pop3_port": account.pop3_port, + "pop3_username": account.pop3_username, + "pop3_password": account.pop3_password, + } + merged.update(patch_data) + merged = _validate_mailbox_payload(merged) + update_data = {key: merged[key] for key in patch_data.keys()}Also applies to: 249-253
🤖 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 `@backend/api/mailbox_accounts.py` around lines 244 - 246, The current PATCH logic validates only the partial payload, which can break invariants (e.g., null smtp_server with existing smtp_port); instead, load the existing account state (e.g., mailbox_account or found account dict), merge it with payload.model_dump(exclude_unset=True) to produce the effective/merged account state, then pass that merged dict into _sanitize_mailbox_payload and _validate_mailbox_payload to produce update_data before persisting; apply the same merged-state validation pattern to the other similar block referenced (lines ~249-253) so all patch handling validates the effective account state rather than only the patch fields.backend/api/emails.py-212-220 (1)
212-220:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce active-state check for explicit mailbox sends.
At Line 214, the explicit mailbox lookup only checks owner/id. An inactive mailbox account can still be used to send mail when
mailbox_account_idis provided, which bypasses deactivation intent.🔧 Proposed fix
if request.mailbox_account_id is not None: mailbox_account = await db.scalar( select(MailboxAccount).where( MailboxAccount.id == request.mailbox_account_id, MailboxAccount.user_id == target_user_id, + MailboxAccount.is_active.is_(True), ) ) if not mailbox_account: - raise HTTPException(status_code=404, detail="Mailbox account not found") + raise HTTPException( + status_code=404, + detail="Mailbox account not found", + )🤖 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 `@backend/api/emails.py` around lines 212 - 220, Lookup for an explicit mailbox (when request.mailbox_account_id is set) currently only validates id/owner and can allow inactive accounts; update the validation in the mailbox lookup for MailboxAccount (or immediately after fetching mailbox_account) to enforce the active state for target_user_id by either adding MailboxAccount.active == True to the select().where(...) clause or checking mailbox_account.active and raising HTTPException(status_code=404, detail="Mailbox account not found") when false, so inactive mailbox accounts cannot be used for sends.
🟡 Minor comments (11)
backend/tests/test_email_client_smtp.py-44-58 (1)
44-58:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPatch/assert the actual send helper, not
aiosmtplib.sendThese checks are now disconnected from runtime behavior because
send_emailno longer callsaiosmtplib.send. Assert against_send_message_via_validated_smtpfor meaningful coverage.Suggested adjustment
- with patch( - "services.email_client.aiosmtplib.send", new_callable=AsyncMock - ) as smtp_send: + with patch.object( + email_client, "_send_message_via_validated_smtp", new_callable=AsyncMock + ) as send_mock: with pytest.raises(Exception, match="내부 네트워크"): await send_email( to_address="test@example.com", subject="Test Failure", body="Should fail before SMTP connection", smtp_server="smtp.rebind.example.com", smtp_port=587, smtp_username="testuser", ) - - smtp_send.assert_not_awaited() + send_mock.assert_not_awaited()Also applies to: 89-107
🤖 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 `@backend/tests/test_email_client_smtp.py` around lines 44 - 58, The test patches and asserts against aiosmtplib.send, but send_email no longer calls that helper; update the test to patch and assert the internal helper _send_message_via_validated_smtp instead so the assertion reflects runtime behavior. Specifically, replace the patch target "services.email_client.aiosmtplib.send" with the function/path for _send_message_via_validated_smtp (used by send_email) and assert that this mock was not awaited (or awaited) as appropriate; do the same replacement for the second occurrence covering lines 89–107 so both tests validate the actual send helper invoked by send_email.docs/plans/2026-05-14-naruon-full-scope-remediation-design.md-14-14 (1)
14-14:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove machine-specific absolute path from the plan
Line 14 hardcodes a local path (
/home/...), which is not portable for other contributors. Use a repo-relative reference (or a documented artifact location) instead.🤖 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 `@docs/plans/2026-05-14-naruon-full-scope-remediation-design.md` at line 14, Replace the machine-specific absolute path `/home/seongho/ai_email_client/frontend/branding/uiux` in the plan with a portable repo-relative or documented-artifact reference (for example `frontend/branding/uiux` or a pointer to a documented artifact location), and update the sentence that sets it as the "read-only truth source" accordingly in docs/plans/2026-05-14-naruon-full-scope-remediation-design.md so other contributors can locate the assets without relying on a local filesystem path.frontend/src/app/ai-hub/context/page.tsx-18-32 (1)
18-32:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset loading/error state on each query-driven fetch.
When
searchQuerychanges,loadingis not re-enabled and priorerroris not cleared, so stale error/loading UI can persist across successful subsequent loads.🩹 Proposed fix
useEffect(() => { let active = true; const loadEmails = async () => { + setLoading(true); + setError(null); try { const data = searchQuery ? await apiClient.post<{ results: WorkspaceInsightEmail[] }>('/api/search', { query: searchQuery }) : await apiClient.get<{ emails: WorkspaceInsightEmail[] }>('/api/emails?limit=24'); if (!active) return; setEmails('results' in data ? data.results || [] : data.emails || []);🤖 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 `@frontend/src/app/ai-hub/context/page.tsx` around lines 18 - 32, The effect's fetch (loadEmails inside useEffect) doesn't reset loading/error when a new searchQuery triggers it, so call setLoading(true) and setError(undefined) (or '') at the start of loadEmails (or immediately in the useEffect before invoking loadEmails) to re-enable the loading state and clear any prior error; keep the existing active guard and existing setEmails/setLoading/setError usage so the rest of the logic (try/catch/finally) remains unchanged.frontend/src/lib/execution-queue.test.ts-134-137 (1)
134-137:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid redefining
window.locationdirectly in this test.Overriding
window.locationwithObject.definePropertyis brittle in jsdom and can leak across cases. Preferhistory.replaceState(...)and restore URL in teardown.Suggested fix
- Object.defineProperty(window, 'location', { - configurable: true, - value: new URL('http://localhost/settings'), - }); + const originalPath = window.location.pathname; + window.history.replaceState({}, '', '/settings'); ... + window.history.replaceState({}, '', originalPath);🤖 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 `@frontend/src/lib/execution-queue.test.ts` around lines 134 - 137, The test currently redefines window.location with Object.defineProperty (setting window.location to new URL('http://localhost/settings')), which is brittle; replace that line with history.replaceState(null, '', '/settings') to change the URL within jsdom safely and add a teardown step that restores the original URL (save location.href before change and call history.replaceState(null, '', originalHref) in afterEach/teardown). Update the test where window.location is set and ensure any references to window.location continue to work with the replaced history URL.frontend/src/components/EmailDetail.test.tsx-104-107 (1)
104-107:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset
apiClienttest mutations in teardown.
beforeEachenables dev-header auth on a shared singleton, but it isn’t restored inafterEach, which can leak state into other suites.Suggested fix
afterEach(() => { if (root) { act(() => root?.unmount()); } root = null; container?.remove(); container = null; + apiClient.setDevHeaderAuthEnabled(false); + apiClient.setBaseUrl(''); vi.unstubAllGlobals(); });🤖 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 `@frontend/src/components/EmailDetail.test.tsx` around lines 104 - 107, The tests enable dev-header auth on the shared singleton in the beforeEach (apiClient.setDevHeaderAuthEnabled(true)) but never restore it; add an afterEach that resets any mutated apiClient state (call apiClient.setDevHeaderAuthEnabled(false) and restore base URL to '' via apiClient.setBaseUrl('') or other defaults) so the singleton is returned to its original state for other suites; target the teardown near the existing beforeEach in EmailDetail.test.tsx.backend/tests/test_imap_worker_sync.py-53-55 (1)
53-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEscape regex metacharacters in
pytest.raises(match=...).The
matchparameter uses regex; unescaped dots inalpha@example.comallow the assertion to match unintended strings likealpha@exampleXcom(where X is any character).Suggested fix
- with pytest.raises( - Exception, match="IMAP Sync failed for user testuser mailbox alpha@example.com" - ): + with pytest.raises( + Exception, match=r"IMAP Sync failed for user testuser mailbox alpha@example\.com" + ):🤖 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 `@backend/tests/test_imap_worker_sync.py` around lines 53 - 55, The test's pytest.raises(match=...) uses an unescaped literal email which is treated as a regex; update the assertion to escape regex metacharacters (e.g. use re.escape on "IMAP Sync failed for user testuser mailbox alpha@example.com") so dots and other special chars are treated literally, and add the necessary import for the re module; target the pytest.raises(...) call and replace the raw literal match with an escaped string.backend/scripts/import_fixtures.py-51-58 (1)
51-58:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLook up the API key for the configured legacy owner.
After introducing
owner_user_id, this query still hardcodes"default". Imports for any other legacy owner will ignore that owner'sTenantConfigand silently fall back to the env var or an empty key.Suggested fix
tenant_config = await session.scalar( - select(TenantConfig).where(TenantConfig.user_id == "default") + select(TenantConfig).where( + TenantConfig.user_id == owner_user_id + ) )🤖 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 `@backend/scripts/import_fixtures.py` around lines 51 - 58, The TenantConfig lookup is hardcoded to "default" so imports for other legacy owners ignore their configured key; change the select that populates tenant_config to filter on TenantConfig.user_id == owner_user_id (use the existing owner_user_id variable instead of the literal "default") so tenant_config = await session.scalar(select(TenantConfig).where(TenantConfig.user_id == owner_user_id)); keep the existing fallback logic to os.getenv("OPENAI_API_KEY") or "" so imports still fall back when a per-owner key is absent.frontend/src/app/compose/ComposePageClient.tsx-41-42 (1)
41-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid defaulting to an inactive reply account.
At Line 41, fallback currently uses
data.items[0], which can preselect an inactive account and lead to immediate send failures. Prefer the first active account as fallback.Proposed fix
- const defaultAccount = data.items.find((item) => item.is_default_reply && item.is_active) || data.items[0] || null; + const defaultAccount = + data.items.find((item) => item.is_default_reply && item.is_active) ?? + data.items.find((item) => item.is_active) ?? + null;🤖 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 `@frontend/src/app/compose/ComposePageClient.tsx` around lines 41 - 42, The fallback currently uses data.items[0] which may be inactive; change the selection logic so defaultAccount is set to the item that is both is_default_reply and is_active, or if none, the first item where is_active is true, or null; update the code that computes defaultAccount (the expression using data.items.find(...) and the subsequent setSelectedMailboxAccountId call) to use data.items.find(item => item.is_active) as the fallback instead of data.items[0].backend/api/auth.py-125-130 (1)
125-130:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle bearer scheme case-insensitively.
The current check rejects valid
authorization: bearer <token>headers. This can cause avoidable 401s with some clients/proxies.Suggested fix
- if not authorization.startswith("Bearer "): - raise HTTPException(status_code=401, detail="Invalid bearer token") - - token = authorization.split(" ", 1)[1].strip() + scheme, _, raw_token = authorization.partition(" ") + if scheme.lower() != "bearer": + raise HTTPException(status_code=401, detail="Invalid bearer token") + + token = raw_token.strip() if not token: raise HTTPException(status_code=401, detail="Invalid bearer token")🤖 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 `@backend/api/auth.py` around lines 125 - 130, The bearer-scheme check on the authorization header is case-sensitive and rejects headers like "bearer <token>"; update the logic that inspects the variable authorization to compare the scheme case-insensitively (e.g., lowercasing the scheme before comparing or splitting into scheme and token and checking scheme.lower() == "bearer"), then extract the token (token variable) safely (split once or use partition) and keep the existing 401 on missing token. Ensure you update the block containing authorization and token so valid mixed-case "Bearer"/"bearer" headers are accepted.frontend/src/lib/api-client.ts-96-109 (1)
96-109:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep localhost fallback identity in sync with the dev-header gate.
These getters fall back to local-dev user/role/org values even when
ensureDevHeaderAuthGate()has not enabled dev-header auth. On localhost with dev headers disabled, claim-gated UI can render as if a user or admin session exists while every request still goes out unauthenticated.Suggested fix
+ private canUseLocalDevIdentity() { + return this.devHeaderAuthEnabled && this.isLocalDevOverrideAllowed(); + } + private getLocalDevUserId() { - if (!this.isLocalDevOverrideAllowed() || typeof window === 'undefined') return null; + if (!this.canUseLocalDevIdentity() || typeof window === 'undefined') return null; const stored = localStorage.getItem('naruon_dev_user')?.trim(); return stored || 'testuser'; } @@ private getLocalDevOrganizationId() { - if (!this.isLocalDevOverrideAllowed()) return null; + if (!this.canUseLocalDevIdentity()) return null; return 'org-local-dev'; }Also applies to: 147-193
🤖 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 `@frontend/src/lib/api-client.ts` around lines 96 - 109, The local-dev getters currently return fallbacks even when dev-header auth is not enabled, causing UI to appear authenticated; update getLocalDevUserId, getLocalDevRole, and getLocalDevOrganizationId to only return values when the dev-header gate allows overrides (use isLocalDevOverrideAllowed() and the same window check), and make getLocalDevRole return null if getLocalDevUserId() is null (instead of defaulting to 'member'); apply the same guard pattern to any other local-dev getters referenced elsewhere (the ones around getLocalDevUserId/getLocalDevRole/getLocalDevOrganizationId) so the UI and request auth state stay in sync.frontend/src/lib/api-client.ts-11-23 (1)
11-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject malformed bearer tokens before treating them as a valid session.
parts.length < 2still accepts two-segment strings, so a malformed pasted token can survive client-side validation and drive the logged-in UI until the backend rejects it.Suggested fix
function decodeBearerClaims(token: string): Record<string, unknown> | null { const parts = token.split('.'); - if (parts.length < 2) return null; + if (parts.length !== 3) return null; try {🤖 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 `@frontend/src/lib/api-client.ts` around lines 11 - 23, The function decodeBearerClaims currently allows tokens with two segments (parts.length < 2) which lets malformed tokens pass; update the validation to require exactly three JWT segments by replacing the check with parts.length !== 3 and return null for any non-3-segment input, keeping the rest of decodeBearerClaims (base64url decode, JSON.parse, exp check) unchanged; reference: decodeBearerClaims.
| google-auth-oauthlib==1.2.0 | ||
| email-validator==2.1.0 | ||
| cryptography==47.0.0 | ||
| PyJWT==2.10.1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify PyJWT 2.10.1 exists and check for security advisories
# Check if version 2.10.1 exists on PyPI
echo "=== Checking PyJWT version 2.10.1 on PyPI ==="
curl -s https://pypi.org/pypi/PyJWT/json | jq -r '.releases | keys[] | select(. == "2.10.1")'
# Check latest version
echo -e "\n=== Latest PyJWT version ==="
curl -s https://pypi.org/pypi/PyJWT/json | jq -r '.info.version'
# Check for security advisories
echo -e "\n=== Security advisories for PyJWT ==="
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: PIP, package: "PyJWT") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Repository: Seongho-Bae/naruon
Length of output: 1090
Upgrade PyJWT to version 2.12.0 or later to fix HIGH severity security vulnerability.
PyJWT 2.10.1 is valid but contains a HIGH severity vulnerability ("PyJWT accepts unknown crit header extensions") affecting all versions up to 2.11.0. This vulnerability is critical for JWT/OIDC authentication. The fix is available in version 2.12.0+. Upgrade to PyJWT>=2.12.0 (latest: 2.12.1).
🤖 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 `@backend/requirements.txt` at line 20, requirements.txt pins PyJWT to 2.10.1
which is vulnerable; update the PyJWT entry to a secure release (e.g., change
"PyJWT==2.10.1" to "PyJWT>=2.12.0" or "PyJWT==2.12.1") and then regenerate any
dependency lock files (pip-tools/poetry/poetry.lock) and run your test suite to
ensure no breaking changes; search for the literal "PyJWT==2.10.1" in the repo
to locate the exact requirement line to edit.
1 similar comment
| if mailbox_account_id is not None: | ||
| statement = statement.where( | ||
| or_( | ||
| Email.mailbox_account_id == mailbox_account_id, | ||
| Email.mailbox_account_id.is_(None), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
mailbox_account_id를 지정해도 mailbox_account_id IS NULL인 레거시 이메일이 함께 반환됩니다. get_emails는 동일 파라미터에 등호 조건만 사용하므로, 스레드 뷰에서만 다른 메일함의 레거시 메일이 섞여 나와 두 엔드포인트 간 필터링 결과가 일치하지 않습니다.
| if mailbox_account_id is not None: | |
| statement = statement.where( | |
| or_( | |
| Email.mailbox_account_id == mailbox_account_id, | |
| Email.mailbox_account_id.is_(None), | |
| ) | |
| ) | |
| if mailbox_account_id is not None: | |
| statement = statement.where( | |
| Email.mailbox_account_id == mailbox_account_id | |
| ) |
1 similar comment
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/DashboardLayout.test.tsx (1)
28-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset
sessionStoragein teardown to keep tests isolated.Line 297 sets
naruon.sidebarScrollTop, but teardown never clearssessionStorage. That can leak state into later tests and make failures order-dependent.Proposed fix
afterEach(() => { if (root) { act(() => root?.unmount()); } root = null; container?.remove(); container = null; + window.sessionStorage.clear(); vi.clearAllMocks(); apiClientMock.canManageWorkspaceSettings.mockReturnValue(false); });Also applies to: 296-314
🤖 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 `@frontend/src/components/DashboardLayout.test.tsx` around lines 28 - 37, The afterEach teardown in DashboardLayout.test.tsx currently resets root/container and mocks but does not clear sessionStorage, so add a sessionStorage cleanup (e.g., removeItem for "naruon.sidebarScrollTop" or sessionStorage.clear()) to the afterEach block that contains root?.unmount(), container?.remove(), vi.clearAllMocks(), and apiClientMock.canManageWorkspaceSettings.mockReturnValue(false); ensure you remove the specific key set by the tests (naruon.sidebarScrollTop) to avoid leaking state between tests.
🧹 Nitpick comments (1)
backend/tests/test_prompts_api.py (1)
251-258: ⚡ Quick winStrengthen SQL scoping assertions to avoid false positives.
Line 257 currently checks for
"created_by"anywhere in SQL text, which can still pass even if the actual scoping predicate regresses. Prefer asserting against theWHEREclause and expected scoped params.Suggested tightening
statement = build_prompt_list_statement(auth_context) - sql = str(statement).lower() - params = statement.compile().params + compiled_sql = str(statement.compile()).lower() + params = statement.compile().params + where_clause = compiled_sql.split(" where ", 1)[1] - assert "prompt_templates.organization_id" in sql - assert "prompt_templates.is_shared" in sql - assert "created_by" in sql + assert "prompt_templates.organization_id" in where_clause + assert "prompt_templates.is_shared" in where_clause + assert "prompt_templates.created_by" in where_clause assert "org-current" in params.values() + assert "testuser" in params.values()🤖 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 `@backend/tests/test_prompts_api.py` around lines 251 - 258, The test's SQL assertions are too loose: instead of asserting "created_by" anywhere in sql, extract the WHERE clause from the generated SQL (use the string from statement or str(statement).lower() and split/find " where ") and assert the specific scoping predicate is present (e.g. "prompt_templates.created_by =" or "prompt_templates.created_by = :created_by") and that the corresponding param key exists in statement.compile().params; keep the existing checks for "prompt_templates.organization_id" and "prompt_templates.is_shared" but replace the loose "created_by" assertion with a targeted WHERE-clause assertion referencing build_prompt_list_statement, statement, sql, and params.
🤖 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 `@ARCHITECTURE.md`:
- Around line 142-144: Replace the unhyphenated phrase "org scoped APIs" with
the hyphenated compound adjective "org-scoped APIs" in the ARCHITECTURE.md text
(search for the exact snippet "org scoped APIs" near the sentence mentioning
`LEGACY_LLM_PROVIDER_ORGANIZATION_ID` and update it to "org-scoped APIs") to
ensure consistent technical wording.
In `@backend/api/prompts.py`:
- Around line 144-146: The error message is misleading when
auth_context.organization_id is missing; update the HTTPException raised in the
branch that checks auth_context.organization_id to use an org-scoped message
(e.g. "organization context not configured" or "organization ID not provided")
instead of "LLM API key not configured" so clients can distinguish missing org
scope; locate the check referencing auth_context.organization_id and change the
HTTPException detail accordingly while keeping status_code=400.
- Around line 61-68: The except block in backend/api/prompts.py that currently
does "except Exception as e" and logs via
logging.getLogger(__name__).error(f"Prompt execution failed: {e}") should
preserve the original traceback and chain the exception; update the logging call
to include exc_info=True (or use logger.exception) so the traceback is recorded,
and re-raise the HTTPException using "raise HTTPException(... ) from e" to
maintain exception chaining for callers; target the except block surrounding the
prompt execution logic and adjust the logger/error and raise statements
accordingly.
In `@docs/plans/2026-05-15-dashboard-mobile-rbac-product-remediation.md`:
- Line 21: The document contains a machine-local absolute path
'/home/seongho/ai_email_client/frontend/branding/uiux/' which is not portable;
replace that string with a repo-relative path (for example
'frontend/branding/uiux/' or './frontend/branding/uiux/') or use a documented
variable/placeholder (e.g., '<repo-root>/frontend/branding/uiux/') so
contributors can resolve it in any environment and update the reference in the
same line where the absolute path appears.
In `@frontend/src/app/prompt-studio/page.tsx`:
- Around line 36-45: The call to apiClient.ensureWorkspaceSettingsAccessReady()
inside loadAccessState can reject and is currently uncaught, which may leave the
UI stuck; wrap the await in a try/catch (or try/catch/finally) around
ensureWorkspaceSettingsAccessReady() in loadAccessState, and in the catch set a
safe accessState (e.g., ready: apiClient.isWorkspaceSettingsAccessReady() ||
true/false and canManage: apiClient.canManageWorkspaceSettings() || false) or
otherwise ensure setAccessState is always called so the loading UI clears;
reference the loadAccessState function and the methods
apiClient.ensureWorkspaceSettingsAccessReady,
apiClient.isWorkspaceSettingsAccessReady, apiClient.canManageWorkspaceSettings,
and the setAccessState call when making the change.
---
Outside diff comments:
In `@frontend/src/components/DashboardLayout.test.tsx`:
- Around line 28-37: The afterEach teardown in DashboardLayout.test.tsx
currently resets root/container and mocks but does not clear sessionStorage, so
add a sessionStorage cleanup (e.g., removeItem for "naruon.sidebarScrollTop" or
sessionStorage.clear()) to the afterEach block that contains root?.unmount(),
container?.remove(), vi.clearAllMocks(), and
apiClientMock.canManageWorkspaceSettings.mockReturnValue(false); ensure you
remove the specific key set by the tests (naruon.sidebarScrollTop) to avoid
leaking state between tests.
---
Nitpick comments:
In `@backend/tests/test_prompts_api.py`:
- Around line 251-258: The test's SQL assertions are too loose: instead of
asserting "created_by" anywhere in sql, extract the WHERE clause from the
generated SQL (use the string from statement or str(statement).lower() and
split/find " where ") and assert the specific scoping predicate is present (e.g.
"prompt_templates.created_by =" or "prompt_templates.created_by = :created_by")
and that the corresponding param key exists in statement.compile().params; keep
the existing checks for "prompt_templates.organization_id" and
"prompt_templates.is_shared" but replace the loose "created_by" assertion with a
targeted WHERE-clause assertion referencing build_prompt_list_statement,
statement, sql, and params.
🪄 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: 47c04d9f-d00d-430d-8805-1a7d57d77822
📒 Files selected for processing (18)
ARCHITECTURE.mdbackend/api/prompts.pybackend/tests/test_prompts_api.pydocs/operations/auth-key-management.mddocs/plans/2026-05-14-synthetic-mailbox-platform-program.mddocs/plans/2026-05-15-dashboard-mobile-rbac-product-remediation.mdfrontend/src/app/layout.test.tsxfrontend/src/app/layout.tsxfrontend/src/app/prompt-studio/page.test.tsxfrontend/src/app/prompt-studio/page.tsxfrontend/src/components/DashboardLayout.test.tsxfrontend/src/components/DashboardLayout.tsxfrontend/src/components/DevAuthSwitcher.test.tsxfrontend/src/components/DevAuthSwitcher.tsxfrontend/src/components/EmailList.test.tsxfrontend/src/lib/api-client.test.tsfrontend/src/lib/api-client.tsfrontend/tests/e2e/dashboard-branding.spec.ts
✅ Files skipped from review due to trivial changes (1)
- frontend/src/app/layout.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/src/components/DevAuthSwitcher.tsx
- frontend/src/components/DashboardLayout.tsx
- frontend/src/lib/api-client.ts
- frontend/tests/e2e/dashboard-branding.spec.ts
- frontend/src/components/EmailList.test.tsx
| except Exception as e: | ||
| import logging | ||
|
|
||
| logging.getLogger(__name__).error(f"Prompt execution failed: {e}") | ||
| raise HTTPException(status_code=502, detail="Failed to execute prompt with AI provider. Check provider status.") | ||
| raise HTTPException( | ||
| status_code=502, | ||
| detail="Failed to execute prompt with AI provider. Check provider status.", | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "prompts.py" | head -20Repository: Seongho-Bae/naruon
Length of output: 87
🏁 Script executed:
cat -n ./backend/api/prompts.py | sed -n '50,75p'Repository: Seongho-Bae/naruon
Length of output: 1175
🏁 Script executed:
rg "except Exception" ./backend/api/prompts.py -A 5Repository: Seongho-Bae/naruon
Length of output: 246
🏁 Script executed:
rg "logging\.exception|from e:|from err:" ./backend --type py | head -20Repository: Seongho-Bae/naruon
Length of output: 44
🏁 Script executed:
rg "except Exception" ./backend --type py -A 3 | head -50Repository: Seongho-Bae/naruon
Length of output: 3156
🏁 Script executed:
cat -n ./backend/services/llm_service.py | sed -n '40,70p'Repository: Seongho-Bae/naruon
Length of output: 1253
Use exception chaining and proper exception logging to preserve debugging context in provider failure path.
Lines 61-68 catch exceptions and rethrow without explicit chaining or proper exception logging. The current code uses .error() with string formatting, which loses the original traceback. This makes production debugging harder.
The codebase already establishes a pattern of using from e for exception chaining (see llm_service.py and calendar_service.py) and exc_info=True for traceback logging (see workers). Align this handler with those patterns:
Proposed fix
- except Exception as e:
+ except Exception as err:
import logging
- logging.getLogger(__name__).error(f"Prompt execution failed: {e}")
+ logging.getLogger(__name__).exception("Prompt execution failed")
raise HTTPException(
status_code=502,
detail="Failed to execute prompt with AI provider. Check provider status.",
- )
+ ) from err📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as e: | |
| import logging | |
| logging.getLogger(__name__).error(f"Prompt execution failed: {e}") | |
| raise HTTPException(status_code=502, detail="Failed to execute prompt with AI provider. Check provider status.") | |
| raise HTTPException( | |
| status_code=502, | |
| detail="Failed to execute prompt with AI provider. Check provider status.", | |
| ) | |
| except Exception as err: | |
| import logging | |
| logging.getLogger(__name__).exception("Prompt execution failed") | |
| raise HTTPException( | |
| status_code=502, | |
| detail="Failed to execute prompt with AI provider. Check provider status.", | |
| ) from err |
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 61-61: Do not catch blind exception: Exception
(BLE001)
[warning] 65-68: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 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 `@backend/api/prompts.py` around lines 61 - 68, The except block in
backend/api/prompts.py that currently does "except Exception as e" and logs via
logging.getLogger(__name__).error(f"Prompt execution failed: {e}") should
preserve the original traceback and chain the exception; update the logging call
to include exc_info=True (or use logger.exception) so the traceback is recorded,
and re-raise the HTTPException using "raise HTTPException(... ) from e" to
maintain exception chaining for callers; target the except block surrounding the
prompt execution logic and adjust the logger/error and raise statements
accordingly.
| if not auth_context.organization_id: | ||
| raise HTTPException(status_code=400, detail="LLM API key not configured") | ||
|
|
There was a problem hiding this comment.
Use an org-scope error message for missing organization context.
Line 145 returns "LLM API key not configured" when the actual failure is missing organization scope. This makes client handling ambiguous.
Proposed fix
- if not auth_context.organization_id:
- raise HTTPException(status_code=400, detail="LLM API key not configured")
+ if not auth_context.organization_id:
+ raise HTTPException(status_code=400, detail="Organization scope is required")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not auth_context.organization_id: | |
| raise HTTPException(status_code=400, detail="LLM API key not configured") | |
| if not auth_context.organization_id: | |
| raise HTTPException(status_code=400, detail="Organization scope is required") |
🤖 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 `@backend/api/prompts.py` around lines 144 - 146, The error message is
misleading when auth_context.organization_id is missing; update the
HTTPException raised in the branch that checks auth_context.organization_id to
use an org-scoped message (e.g. "organization context not configured" or
"organization ID not provided") instead of "LLM API key not configured" so
clients can distinguish missing org scope; locate the check referencing
auth_context.organization_id and change the HTTPException detail accordingly
while keeping status_code=400.
| - PR #200은 `613cc58c475b4f15bd06368ff923c5412500d461` 기준 checks는 green이고 CodeRabbit status도 success이지만, repository ruleset `14316398`이 `required_approving_review_count=1` 및 `require_last_push_approval=true`로 남아 GitHub merge gate가 `REVIEW_REQUIRED`를 반환한다. | ||
| - repo policy의 default merge source는 current-head CodeRabbit evidence이며 human review 대기는 기본값이 아니다. 단, `--admin` bypass는 명시 요청 없이 사용하지 않는다. | ||
| - 기존 `2026-05-14-dashboard-rbac-mobile-oidc-remediation.md`는 넓은 감사 계획이다. 이 문서는 실제로 닫을 작은 slice와 후속 에픽 경계를 다시 고정한다. | ||
| - 디자인 원본은 `/home/seongho/ai_email_client/frontend/branding/uiux/`이고, 사용자-facing copy는 AI 자체보다 맥락 종합, 판단 포인트, 실행 항목을 앞세운다. |
There was a problem hiding this comment.
Replace machine-local absolute path with repo-relative path.
Line 21 hardcodes /home/seongho/..., which is not portable across contributors/environments.
Proposed fix
-- 디자인 원본은 `/home/seongho/ai_email_client/frontend/branding/uiux/`이고, 사용자-facing copy는 AI 자체보다 맥락 종합, 판단 포인트, 실행 항목을 앞세운다.
+- 디자인 원본은 `frontend/branding/uiux/`이고, 사용자-facing copy는 AI 자체보다 맥락 종합, 판단 포인트, 실행 항목을 앞세운다.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - 디자인 원본은 `/home/seongho/ai_email_client/frontend/branding/uiux/`이고, 사용자-facing copy는 AI 자체보다 맥락 종합, 판단 포인트, 실행 항목을 앞세운다. | |
| - 디자인 원본은 `frontend/branding/uiux/`이고, 사용자-facing copy는 AI 자체보다 맥락 종합, 판단 포인트, 실행 항목을 앞세운다. |
🤖 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 `@docs/plans/2026-05-15-dashboard-mobile-rbac-product-remediation.md` at line
21, The document contains a machine-local absolute path
'/home/seongho/ai_email_client/frontend/branding/uiux/' which is not portable;
replace that string with a repo-relative path (for example
'frontend/branding/uiux/' or './frontend/branding/uiux/') or use a documented
variable/placeholder (e.g., '<repo-root>/frontend/branding/uiux/') so
contributors can resolve it in any environment and update the reference in the
same line where the absolute path appears.
| const loadAccessState = async () => { | ||
| await apiClient.ensureWorkspaceSettingsAccessReady(); | ||
| if (!active) return; | ||
| setAccessState({ | ||
| ready: apiClient.isWorkspaceSettingsAccessReady(), | ||
| canManage: apiClient.canManageWorkspaceSettings(), | ||
| }); | ||
| }; | ||
|
|
||
| void loadAccessState(); |
There was a problem hiding this comment.
Handle readiness-fetch failures so the page doesn’t get stuck in perpetual loading.
At Line 37, a rejection from ensureWorkspaceSettingsAccessReady() is uncaught. That can leave accessState.ready false forever and trap users on “권한을 확인하고 있습니다.”.
Proposed fix
const loadAccessState = async () => {
- await apiClient.ensureWorkspaceSettingsAccessReady();
- if (!active) return;
- setAccessState({
- ready: apiClient.isWorkspaceSettingsAccessReady(),
- canManage: apiClient.canManageWorkspaceSettings(),
- });
+ try {
+ await apiClient.ensureWorkspaceSettingsAccessReady();
+ if (!active) return;
+ setAccessState({
+ ready: apiClient.isWorkspaceSettingsAccessReady(),
+ canManage: apiClient.canManageWorkspaceSettings(),
+ });
+ } catch {
+ if (!active) return;
+ // Fail closed on permission, but avoid infinite loading state.
+ setAccessState({
+ ready: true,
+ canManage: 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 `@frontend/src/app/prompt-studio/page.tsx` around lines 36 - 45, The call to
apiClient.ensureWorkspaceSettingsAccessReady() inside loadAccessState can reject
and is currently uncaught, which may leave the UI stuck; wrap the await in a
try/catch (or try/catch/finally) around ensureWorkspaceSettingsAccessReady() in
loadAccessState, and in the catch set a safe accessState (e.g., ready:
apiClient.isWorkspaceSettingsAccessReady() || true/false and canManage:
apiClient.canManageWorkspaceSettings() || false) or otherwise ensure
setAccessState is always called so the loading UI clears; reference the
loadAccessState function and the methods
apiClient.ensureWorkspaceSettingsAccessReady,
apiClient.isWorkspaceSettingsAccessReady, apiClient.canManageWorkspaceSettings,
and the setAccessState call when making the change.
|
|
||
| async def _create_connection(self, timeout: float | None): | ||
| if self.loop is None: | ||
| raise RuntimeError("No event loop set") | ||
|
|
||
| protocol = SMTPProtocol(loop=self.loop) |
There was a problem hiding this comment.
_PinnedSMTP._create_connection — aiosmtplib 3.0.1 호환성 미검증
_create_connection은 self.loop에 접근하고 SMTPProtocol(loop=self.loop)를 사용합니다. aiosmtplib 3.0.x에서는 Python 3.10의 loop 파라미터 제거 흐름에 따라 self.loop 속성이 없거나 SMTPProtocol이 loop= 인자를 받지 않을 수 있습니다.
더 결정적인 문제는, test_send_email_uses_validated_smtp_connect_ip가 _send_message_via_validated_smtp를 monkeypatch로 교체하기 때문에 _create_connection은 테스트 스위트에서 단 한 번도 실행되지 않습니다. SMTP 자격증명이 설정된 운영 환경에서 첫 번째 실제 발송 시도에서야 오류가 드러날 수 있습니다.
self.loop 대신 asyncio.get_running_loop()를 사용하고, _send_message_via_validated_smtp를 mocking하지 않는 통합 테스트를 추가하는 것을 권장합니다.
|
|
||
| async def require_provider_admin( | ||
| auth_context: AuthContext = Depends(get_auth_context), | ||
| ) -> AuthContext: | ||
| if auth_context.role not in {"platform_admin", "organization_admin"}: | ||
| raise HTTPException(status_code=403, detail="Organization admin access required") | ||
| return auth_context.user_id | ||
| raise HTTPException( | ||
| status_code=403, detail="Organization admin access required" | ||
| ) |
There was a problem hiding this comment.
require_provider_admin가 platform_admin의 조직 범위 없는 접근을 차단
platform_admin 역할을 가진 사용자도 organization_id가 없으면 "Organization scope is required" 오류로 차단됩니다. OIDC 토큰에 organization_id 클레임이 없는 platform admin(예: 전역 관리자)은 이 엔드포인트에 접근할 수 없게 됩니다.
기존 check_admin_access는 역할만 확인했는데 이 추가 조건이 의도적인 변경인지 확인이 필요합니다. platform_admin에 대해서는 organization_id 체크를 면제하거나, 의도가 맞다면 주석으로 명시하는 것이 좋습니다.
1 similar comment
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@backend/tests/test_repo_hygiene.py`:
- Around line 73-78: The test currently defines bracket_routes as a fixed list
of two exact file paths and asserts none exist; change it to detect any route
path that contains a bracketed path segment (e.g., [id], [...slug],
[slug]/nested) by scanning the app route tree and filtering paths whose parts
contain a segment that starts with '[' and ends with ']' (or use a regex like
r'\[.*\]')—replace the static REPO_ROOT /
"frontend/src/app/labels/[slug]/page.tsx" entries with a dynamic search (e.g.,
REPO_ROOT.joinpath("frontend","src","app").rglob("**/*")) and set bracket_routes
to the list of matches, then keep the existing assert not bracket_routes to fail
when any bracketed segment is present.
In `@frontend/src/app/labels/page.tsx`:
- Line 15: The direct call to decodeURIComponent(label) can throw a URIError on
malformed percent-encoding; replace the expression that returns
labelNames[label] ?? decodeURIComponent(label) with a guarded decode: try to run
decodeURIComponent(label) and return that result, but catch any exception and
fall back to the raw label (or labelNames[label] if present). Locate the return
expression using the labelNames and label symbols and wrap the decode in a
try-catch that returns a safe fallback on error.
In `@frontend/src/app/projects/page.tsx`:
- Line 13: The code calls decodeURIComponent(workspace) directly (used to render
workspace name via workspaceNames[workspace] ?? decodeURIComponent(workspace)),
which can throw for malformed percent-encoding from searchParams; wrap the
decode in a small safe-decoding guard (e.g., try/catch or a helper like
safeDecodeURIComponent) that returns the decoded string on success and falls
back to the original workspace (or a sanitized fallback) on error, then use that
safe value in place of decodeURIComponent(workspace).
🪄 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: 12da1d60-a5b8-4f0b-a8ef-dc29ba472668
📒 Files selected for processing (5)
backend/tests/test_repo_hygiene.pyfrontend/src/app/labels/page.tsxfrontend/src/app/projects/page.tsxfrontend/src/app/route-contract.test.tsxfrontend/src/components/DashboardLayout.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/app/route-contract.test.tsx
- frontend/src/components/DashboardLayout.tsx
| bracket_routes = [ | ||
| REPO_ROOT / "frontend/src/app/labels/[slug]/page.tsx", | ||
| REPO_ROOT / "frontend/src/app/projects/[slug]/page.tsx", | ||
| ] | ||
|
|
||
| assert not [path for path in bracket_routes if path.exists()] |
There was a problem hiding this comment.
Broaden bracket-route detection to catch any bracket segment, not just [slug].
Lines 73–76 only check two exact files, so paths like [id], [...slug], or nested bracket segments under the same areas would bypass this hygiene guard.
💡 Proposed fix
- bracket_routes = [
- REPO_ROOT / "frontend/src/app/labels/[slug]/page.tsx",
- REPO_ROOT / "frontend/src/app/projects/[slug]/page.tsx",
- ]
-
- assert not [path for path in bracket_routes if path.exists()]
+ app_targets = [
+ REPO_ROOT / "frontend/src/app/labels",
+ REPO_ROOT / "frontend/src/app/projects",
+ ]
+ bracket_routes = [
+ path
+ for base in app_targets
+ for path in base.rglob("page.tsx")
+ if any("[" in part and "]" in part for part in path.relative_to(base).parts)
+ ]
+
+ assert not bracket_routes, f"Bracket-path placeholders are not allowed: {bracket_routes}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bracket_routes = [ | |
| REPO_ROOT / "frontend/src/app/labels/[slug]/page.tsx", | |
| REPO_ROOT / "frontend/src/app/projects/[slug]/page.tsx", | |
| ] | |
| assert not [path for path in bracket_routes if path.exists()] | |
| app_targets = [ | |
| REPO_ROOT / "frontend/src/app/labels", | |
| REPO_ROOT / "frontend/src/app/projects", | |
| ] | |
| bracket_routes = [ | |
| path | |
| for base in app_targets | |
| for path in base.rglob("page.tsx") | |
| if any("[" in part and "]" in part for part in path.relative_to(base).parts) | |
| ] | |
| assert not bracket_routes, f"Bracket-path placeholders are not allowed: {bracket_routes}" |
🤖 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 `@backend/tests/test_repo_hygiene.py` around lines 73 - 78, The test currently
defines bracket_routes as a fixed list of two exact file paths and asserts none
exist; change it to detect any route path that contains a bracketed path segment
(e.g., [id], [...slug], [slug]/nested) by scanning the app route tree and
filtering paths whose parts contain a segment that starts with '[' and ends with
']' (or use a regex like r'\[.*\]')—replace the static REPO_ROOT /
"frontend/src/app/labels/[slug]/page.tsx" entries with a dynamic search (e.g.,
REPO_ROOT.joinpath("frontend","src","app").rglob("**/*")) and set bracket_routes
to the list of matches, then keep the existing assert not bracket_routes to fail
when any bracketed segment is present.
| if (!label) { | ||
| return null; | ||
| } | ||
| return labelNames[label] ?? decodeURIComponent(label); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript, does decodeURIComponent throw URIError for malformed percent-encoded input (e.g. "%E0%A4%A")? Please provide MDN/spec references.
💡 Result:
Yes. In JavaScript, decodeURIComponent throws a URIError with the message “malformed URI sequence” when the input contains malformed percent-encoding—specifically when a “%” is not followed by two hexadecimal digits, or when the percent-encoded bytes do not form a valid UTF-8 sequence [1]. MDN also gives the exact example decodeURIComponent("%E0%A4%A") throwing “URIError: malformed URI sequence” [1][2]. MDN/spec references: - decodeURIComponent MDN: “Thrown if encodedURI contains a % not followed by two hexadecimal digits, or if the escape sequence does not encode a valid UTF-8 character.” It also shows the try/catch example with "%E0%A4%A" producing URIError [1]. - URIError: malformed URI sequence MDN: explains that this exception occurs when URI encoding/decoding wasn’t successful and gives decodeURIComponent("%E0%A4%A") as a decoding example that throws [2].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/decodeURIComponent
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Malformed_URI
🏁 Script executed:
cat -n frontend/src/app/labels/page.tsx | head -30Repository: Seongho-Bae/naruon
Length of output: 1212
Guard URL decoding to prevent runtime errors on malformed query input.
Line 15 calls decodeURIComponent(label) directly without error handling. Malformed percent-encoding in query parameters (e.g., ?label=%E0%A4%A) will throw a URIError and cause the page to fail with a 500 error. Wrap the call in a try-catch block.
💡 Proposed fix
function resolveLabelName(label?: string) {
if (!label) {
return null;
}
- return labelNames[label] ?? decodeURIComponent(label);
+ if (labelNames[label]) {
+ return labelNames[label];
+ }
+ try {
+ return decodeURIComponent(label);
+ } catch {
+ return label;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return labelNames[label] ?? decodeURIComponent(label); | |
| function resolveLabelName(label?: string) { | |
| if (!label) { | |
| return null; | |
| } | |
| if (labelNames[label]) { | |
| return labelNames[label]; | |
| } | |
| try { | |
| return decodeURIComponent(label); | |
| } catch { | |
| return label; | |
| } | |
| } |
🤖 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 `@frontend/src/app/labels/page.tsx` at line 15, The direct call to
decodeURIComponent(label) can throw a URIError on malformed percent-encoding;
replace the expression that returns labelNames[label] ??
decodeURIComponent(label) with a guarded decode: try to run
decodeURIComponent(label) and return that result, but catch any exception and
fall back to the raw label (or labelNames[label] if present). Locate the return
expression using the labelNames and label symbols and wrap the decode in a
try-catch that returns a safe fallback on error.
| if (!workspace) { | ||
| return null; | ||
| } | ||
| return workspaceNames[workspace] ?? decodeURIComponent(workspace); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript, does decodeURIComponent throw URIError for malformed percent-encoded strings? Please cite MDN/spec.
💡 Result:
Yes. In JavaScript, decodeURIComponent throws a URIError (specifically “malformed URI sequence”) when the input contains malformed percent-encoded sequences—e.g., a “%” not followed by two hexadecimal digits, or an escape sequence that doesn’t encode a valid UTF-8 character.[1][2] MDN states that decodeURIComponent throws if the encoded string contains a “%” not followed by two hex digits or if the escape sequence does not encode a valid UTF-8 character.[1] MDN’s URIError (“malformed URI sequence”) page further explains that this error occurs when URI decoding wasn’t successful, showing an example decodeURIComponent("%E0%A4%A") that throws that URIError.[2] So for malformed percent-encoded strings, decodeURIComponent does throw URIError.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/decodeURIComponent
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Malformed_URI
🏁 Script executed:
cat -n frontend/src/app/projects/page.tsxRepository: Seongho-Bae/naruon
Length of output: 1444
Protect workspace decoding from malformed query values.
Line 13 uses decodeURIComponent(workspace) without a guard; invalid percent-encoded query parameters throw URIError and crash the page. The workspace value comes from searchParams, which is untrusted user input.
Proposed fix
function resolveWorkspaceName(workspace?: string) {
if (!workspace) {
return null;
}
- return workspaceNames[workspace] ?? decodeURIComponent(workspace);
+ if (workspaceNames[workspace]) {
+ return workspaceNames[workspace];
+ }
+ try {
+ return decodeURIComponent(workspace);
+ } catch {
+ return workspace;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return workspaceNames[workspace] ?? decodeURIComponent(workspace); | |
| function resolveWorkspaceName(workspace?: string) { | |
| if (!workspace) { | |
| return null; | |
| } | |
| if (workspaceNames[workspace]) { | |
| return workspaceNames[workspace]; | |
| } | |
| try { | |
| return decodeURIComponent(workspace); | |
| } catch { | |
| return workspace; | |
| } | |
| } |
🤖 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 `@frontend/src/app/projects/page.tsx` at line 13, The code calls
decodeURIComponent(workspace) directly (used to render workspace name via
workspaceNames[workspace] ?? decodeURIComponent(workspace)), which can throw for
malformed percent-encoding from searchParams; wrap the decode in a small
safe-decoding guard (e.g., try/catch or a helper like safeDecodeURIComponent)
that returns the decoded string on success and falls back to the original
workspace (or a sanitized fallback) on error, then use that safe value in place
of decodeURIComponent(workspace).
| ) | ||
|
|
||
| if not smtp_server or not smtp_port: | ||
| logger.info("Simulating sending email to %s (no SMTP server configured)", safe_to_address) | ||
| logger.info( | ||
| "Simulating sending email to %s (no SMTP server configured)", | ||
| safe_to_address, | ||
| ) | ||
| return {"status": "simulated", "simulated": True} | ||
|
|
||
| try: | ||
| send_kwargs: dict[str, object] = { | ||
| "hostname": smtp_server, | ||
| "port": smtp_port, | ||
| "use_tls": True, | ||
| } | ||
| if smtp_username: | ||
| send_kwargs["username"] = smtp_username | ||
| if smtp_password: | ||
| send_kwargs["password"] = smtp_password | ||
|
|
||
| await aiosmtplib.send(message, **send_kwargs) | ||
| logger.info("Successfully sent email to %s via %s", safe_to_address, smtp_server) | ||
| try: | ||
| target = resolve_mail_server_connect_target( | ||
| "smtp", "SMTP", smtp_server, smtp_port |
There was a problem hiding this comment.
send_email에서 이벤트 루프 블로킹 DNS 조회 발생
resolve_mail_server_connect_target은 내부적으로 socket.getaddrinfo를 동기적으로 호출합니다. 이 함수는 send_email이라는 async 함수 안에서 await 없이 직접 호출되므로, DNS 응답이 수백 ms~수 초 지연될 경우 전체 asyncio 이벤트 루프가 정지해 다른 모든 요청이 응답을 받지 못하게 됩니다.
mailbox_accounts.py 핸들러의 동일 문제(백로그 참조)와 같은 원인이며, 이 파트는 HTTP 요청 경로에서 직접 호출됩니다. asyncio.to_thread(resolve_mail_server_connect_target, "smtp", "SMTP", smtp_server, smtp_port)로 감싸 스레드풀로 분리해야 합니다.
| target = resolve_mail_server_connect_target( | ||
| "imap", "IMAP", imap_server, imap_port | ||
| ) | ||
| except MailServerValidationError as exc: |
There was a problem hiding this comment.
_sync_mailbox_account에서 동기 DNS 조회로 이벤트 루프 블로킹
resolve_mail_server_connect_target은 _resolve_safe_mail_server_ips → socket.getaddrinfo를 동기적으로 호출합니다. 이 코드가 async def _sync_mailbox_account 안에서 직접 실행되므로 DNS 응답이 지연되면 이벤트 루프 전체가 멈춥니다.
pop3_worker.py는 동일한 함수를 _do_pop3_sync라는 동기 메서드로 분리한 뒤 await asyncio.to_thread(self._do_pop3_sync, account)로 호출하는 올바른 패턴을 사용합니다. IMAP 워커도 동일한 패턴으로 수정하거나, 이 호출만 await asyncio.to_thread(resolve_mail_server_connect_target, ...) 로 감싸야 합니다.
1 similar comment
1 similar comment
| smtp = _PinnedSMTP( | ||
| hostname=target.host, | ||
| port=target.port, | ||
| connect_host=target.connect_host, | ||
| tls_server_hostname=target.host, | ||
| use_tls=True, | ||
| username=username, | ||
| password=password, | ||
| ) |
There was a problem hiding this comment.
use_tls=True 하드코딩
_send_message_via_validated_smtp는 항상 use_tls=True로 _PinnedSMTP를 생성합니다. 이는 포트 465(implicit TLS)에서만 유효하며, ALLOWED_MAIL_PORTS["smtp"]에 허용된 포트 587(STARTTLS), 25, 2525에서는 연결에 실패합니다. 포트 587은 현재 가장 널리 사용되는 SMTP submission 포트이므로, 587로 설정한 모든 메일함에서 실제 발송이 불가능해집니다. 포트 번호에 따라 use_tls/start_tls 플래그를 분기해야 합니다.
| smtp = _PinnedSMTP( | |
| hostname=target.host, | |
| port=target.port, | |
| connect_host=target.connect_host, | |
| tls_server_hostname=target.host, | |
| use_tls=True, | |
| username=username, | |
| password=password, | |
| ) | |
| _IMPLICIT_TLS_PORTS = {465} | |
| use_implicit_tls = target.port in _IMPLICIT_TLS_PORTS | |
| smtp = _PinnedSMTP( | |
| hostname=target.host, | |
| port=target.port, | |
| connect_host=target.connect_host, | |
| tls_server_hostname=target.host, | |
| use_tls=use_implicit_tls, | |
| start_tls=not use_implicit_tls, | |
| username=username, | |
| password=password, | |
| ) |
1 similar comment
| from fastapi import HTTPException | ||
| from api.auth import AuthContext, ensure_organization_access, get_auth_context, get_current_user | ||
| from jwt.algorithms import RSAAlgorithm | ||
| import api.auth as auth_module |
| def sanitize_email_body_for_response(body: str) -> str: | ||
| decoded_body = html.unescape(body) | ||
| if "<" not in decoded_body or ">" not in decoded_body: | ||
| return body | ||
| return sanitize_email_html_to_text(decoded_body) |
There was a problem hiding this comment.
sanitize_email_body_for_response가 HTML 태그가 없는 경우 decoded_body 대신 원본 body를 반환합니다. 예를 들어 "AT&T" 같이 HTML 엔티티는 있지만 <> 태그가 없는 평문 이메일 바디는 & 가 그대로 노출됩니다. 분기 조건은 html.unescape 결과를 기준으로 하는데 반환값은 원본으로 되어 있어 불일치합니다.
| def sanitize_email_body_for_response(body: str) -> str: | |
| decoded_body = html.unescape(body) | |
| if "<" not in decoded_body or ">" not in decoded_body: | |
| return body | |
| return sanitize_email_html_to_text(decoded_body) | |
| def sanitize_email_body_for_response(body: str) -> str: | |
| decoded_body = html.unescape(body) | |
| if "<" not in decoded_body or ">" not in decoded_body: | |
| return decoded_body | |
| return sanitize_email_html_to_text(decoded_body) |
|
Stale: OIDC integration has conflicts and CHANGES_REQUESTED. The auth boundary work has been redesigned in Phase 10+. |
|
PR governance metadata gate is not ready for
|
|
Closing this PR as stale/unmergeable against current Current evidence:
Preserved follow-up:
This close is stale PR queue cleanup, not a rejection of mailbox-aware scope. |
1 participant
No linked issue.
요약
검증
보안/운영 메모
후속 작업
Summary by CodeRabbit
New Features
Bug Fixes
Chores