fix: correct Strix model to github_models/gpt-4o - #222
Merged
11 commits merged intoMay 24, 2026
Merged
Conversation
Contributor
|
Warning Review limit reached
Your plan includes 5 reviews of capacity. Refill in 23 minutes and 14 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
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 |
|---|---|
Strix CI model governance .github/workflows/strix.yml, AGENTS.md |
Strix model identifier changed to github_models/gpt-4o in workflow and governance doc. |
Self-hosted WebSocket connector with optional dependency backend/runner/connector.py, backend/requirements.txt |
Adds SelfHostedConnector (optional websockets import) with bearer-token auth, async connect/listen loop, message handler stub, response sender, and websockets==14.1 dependency. |
Connector registration and gateway config schemas backend/schema/connector.py |
Pydantic models for connector registration request/response and OIDC gateway config with strict extra-field handling and typed fields. |
Email processing helpers and ontology service backend/services/email_service.py, backend/services/ontology_service.py, backend/tests/test_ontology.py |
Adds fingerprinting, reply-tracking, and self-to-self detection; OntologyService.analyze_sender_relationship classifies Newsletter/Colleague/Unknown and is unit-tested. |
CalDAV writeback target routing backend/services/caldav_service.py, backend/tests/test_caldav.py |
CalDavService.determine_writeback_target matches exact source email domain among connected accounts, falls back to primary account or default_system_caldav; tests cover substring-collision and no-accounts cases. |
WebDAV API, service, database models, and tests backend/api/webdav.py, backend/services/webdav_service.py, backend/db/models.py, backend/main.py, backend/tests/test_webdav_api.py |
Adds /api/webdav router with /accounts and /folders endpoints backed by a mocked WebDavService; introduces WebdavAccount and ProjectFolder ORM models (encrypted credentials) and tests validating responses. |
Email list and detail UI metadata badges frontend/src/components/EmailDetail.tsx, frontend/src/components/EmailList.tsx |
Extends email data shapes with requires_reply and schedule_conflict; renders conditional Korean badges and localizes search input labels. |
Frontend DataLayout WebDAV wiring and TasksLayout view implementations frontend/src/components/DataLayout.tsx, frontend/src/components/TasksLayout.tsx |
DataLayout fetches and displays WebDAV accounts and project folders; TasksLayout enriches mock tasks and implements three concrete non-Kanban views with filtering and status UI. |
Mobile hamburger menu and safe-area E2E tests frontend/tests/e2e/mobile-hamburger.spec.ts |
Playwright spec for mobile hamburger menu open/close behavior and bottom-navigation safe-area padding. |
Keycloak service, implementation plans, observability tests, and metadata docker-compose.infra.yml, docs/plans/2026-05-24-architecture-implementation.md, docs/plans/2026-05-24-branding-implementation.md, .vooster/project.json, backend/tests/test_apm_observability.py |
Adds Keycloak dev service to infra compose, new implementation plan docs, project metadata file, and fixes APM observability test path resolution. |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
🐰 I nibble logs and badges in a breeze,
websockets hum and WebDAV leaves,
CalDAV paths I gently trace,
tests and docs in tidy place.
Hop — merge, then I snack on keys.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 23.53% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The PR title 'fix: correct Strix model to github_models/gpt-4o' accurately reflects the primary change in the changeset, which is updating the Strix model identifier from gpt-5.4 to gpt-4o across workflow and documentation files. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
fix/strix-model-gpt4o
Comment @coderabbitai help to get the list of available commands and usage tips.
Contributor
|
PR governance metadata gate is not ready for
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| @@ -0,0 +1,18 @@ | |||
| import pytest | |||
| @@ -0,0 +1,11 @@ | |||
| import pytest | |||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
backend/tests/test_caldav.py (1)
4-18: ⚡ Quick winAdd regression cases for exact-domain matching and empty-account fallback.
Current coverage misses the two highest-risk branches: rejecting substring domain collisions and returning
"default_system_caldav"when no accounts exist.Suggested additions
def test_determine_writeback_target(): @@ assert target_2 == "account1" + + # Should not match substring domain collisions + task_context_3 = {"source_email": "attacker@evilcompany.com"} + target_3 = caldav_service.determine_writeback_target(task_context_3, connected_accounts) + assert target_3 == "account1" # fallback, not domain match + +def test_determine_writeback_target_no_accounts(): + task_context = {"source_email": "boss@company.com"} + target = caldav_service.determine_writeback_target(task_context, []) + assert target == "default_system_caldav"🤖 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_caldav.py` around lines 4 - 18, Add two regression tests for caldav_service.determine_writeback_target: one to verify exact-domain matching (create connected_accounts with an account for "company.com" and another for "mycompany.com" or "notcompany.com", call determine_writeback_target with source_email "user@company.com" and assert it returns the "company.com" account_id to avoid substring collisions), and one to verify empty-account fallback (call determine_writeback_target with connected_accounts = [] and any task_context and assert it returns "default_system_caldav"); update the test_determine_writeback_target in backend/tests/test_caldav.py to include these assertions referencing caldav_service.determine_writeback_target.backend/tests/test_ontology.py (1)
4-11: ⚡ Quick winAdd a mixed-case domain regression assertion.
This test currently won’t catch case-related domain classification regressions.
Proposed test addition
def test_analyze_sender_relationship(): @@ result2 = ontology_service.analyze_sender_relationship("seongho@company.com", "boss@company.com", "Hello") assert result2["type"] == "Colleague" assert result2["confidence"] == 0.85 + + result3 = ontology_service.analyze_sender_relationship("seongho@company.com", "Boss@Company.com", "Hello") + assert result3["type"] == "Colleague"🤖 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_ontology.py` around lines 4 - 11, The test should include a mixed-case domain input to ensure domain classification is case-insensitive: update test_analyze_sender_relationship to call ontology_service.analyze_sender_relationship with a sender like "SeongHo@Company.COM" or recipient "Newsletter@Marketing.COM" (matching the existing newsletter case) and assert the same expected outputs (e.g., type "Newsletter" and confidence 0.9) as the lowercase case; reference the existing test function name test_analyze_sender_relationship and the method ontology_service.analyze_sender_relationship to locate where to add this assertion.
🤖 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/api/caldav_sync.py`:
- Around line 6-15: The new register_writeback_intent endpoint duplicates the
/writeback-intent contract and bypasses the existing typed/authorized flow;
remove this function or refactor it to reuse the existing contract and
auth/context from backend/api/calendar.py by accepting the same Pydantic request
model and the same auth dependency (owner/source-scoped context), perform
validation using that model instead of raw Dict[str, Any], and return the same
typed response shape; reference the register_writeback_intent function and the
/writeback-intent route here to either delete the duplicate or wire it into the
existing calendar.py request/response and authorization utilities.
- Around line 4-7: The router currently exposes private endpoints (e.g.,
register_writeback_intent) without the required signed-session auth; update the
router to enforce get_auth_context as a default dependency (e.g., construct
APIRouter with dependencies including Depends(get_auth_context)) and ensure
get_auth_context (and fastapi.Depends) is imported; this will apply the
signed-session auth to all handlers on router (including
register_writeback_intent) per backend API auth baseline.
In `@backend/runner/connector.py`:
- Around line 31-33: The except block in the connect/listen path that currently
uses "except Exception as e:" should be replaced by catching expected connection
errors (e.g., ConnectionRefusedError, OSError, asyncio.TimeoutError, and
websocket-specific exceptions such as
websockets.exceptions.InvalidURI/InvalidHandshake) and handling them by setting
self.is_connected = False and logging the full traceback with
logger.exception(...) (instead of logger.error). For any truly unexpected
exceptions re-raise them (or let them propagate) so programming errors aren't
swallowed; update the block around the connect/listen code that contains
logger.error(...) and self.is_connected to implement these specific exception
handlers.
- Line 27: Change the websockets.connect call in the connector (where
self.connection is assigned) to use additional_headers=headers instead of
extra_headers to be compatible with websockets==14.1, and replace the bare
"except Exception" block with targeted handling: re-raise
asyncio.CancelledError, and catch only expected connection errors (e.g.,
websockets.InvalidHandshake, websockets.InvalidStatusCode,
websockets.WebSocketException and OSError) to log/handle them while letting
other programming errors surface.
In `@backend/services/caldav_service.py`:
- Around line 16-19: The current loop uses substring matching
(account.get("domain") in source_email) which can misroute accounts; instead
extract and normalize the sender domain from source_email (e.g., split on '@',
lower() and strip) and compare it for exact equality against a normalized
account domain (normalize account.get("domain") similarly) when iterating
connected_accounts; return account.get("account_id") only on exact match and
handle malformed or missing source_email by returning None/empty as appropriate.
In `@backend/services/email_service.py`:
- Around line 12-15: Normalize nullable or non-string email fields before
performing string ops: ensure email_data.get("body") is coerced to a string (or
default to "") before slicing to create body_snippet, and similarly coerce date
and other fields (sender, subject) before calling str() or other string methods;
update the assignments for body_snippet, date, sender, and subject to
validate/type-coerce the values (e.g., use a safe_str(value) pattern or explicit
checks) so slicing and str() cannot raise when body is None or non-string.
- Around line 31-35: The current self-to-self detection uses substring matching
on sender and recipients (variables sender, recipients, user_email) which
misclassifies addresses; change it to parse exact email addresses using
email.utils.getaddresses (or similar) to extract normalized addresses from
sender and recipients, then check user_email against the parsed sender address
and the set of parsed recipient addresses (use equality/membership, not
substring). Update the conditional in the function/method where logger.info is
called (the block referencing sender, recipients, user_email, logger) to use the
parsed addresses and remove the old substring check so the Ruff F541 lint error
is resolved.
In `@backend/services/ontology_service.py`:
- Around line 24-27: Normalize the email domains to a consistent case before
comparing them so domain comparison is case-insensitive: when extracting
user_domain and sender_domain in the function where those variables are set
(user_domain = user_email.split("@")[1], sender_domain =
sender_email.split("@")[1]), call a case-normalizing method (e.g., .lower() or
.casefold()) on both domains and then compare them; if equal, set
relationship_type = "Colleague" as before.
In `@docker-compose.infra.yml`:
- Around line 56-61: Remove the hardcoded KEYCLOAK_ADMIN and
KEYCLOAK_ADMIN_PASSWORD values and require external injection by replacing the
literals with compose variable substitutions (e.g., KEYCLOAK_ADMIN:
"${KEYCLOAK_ADMIN:?KEYCLOAK_ADMIN is not set}" and KEYCLOAK_ADMIN_PASSWORD:
"${KEYCLOAK_ADMIN_PASSWORD:?KEYCLOAK_ADMIN_PASSWORD is not set}") so the service
will fail fast if not provided; update the environment block that currently sets
KEYCLOAK_ADMIN and KEYCLOAK_ADMIN_PASSWORD and ensure any deployment/CI docs or
secrets management supplies these variables instead of committing credentials.
---
Nitpick comments:
In `@backend/tests/test_caldav.py`:
- Around line 4-18: Add two regression tests for
caldav_service.determine_writeback_target: one to verify exact-domain matching
(create connected_accounts with an account for "company.com" and another for
"mycompany.com" or "notcompany.com", call determine_writeback_target with
source_email "user@company.com" and assert it returns the "company.com"
account_id to avoid substring collisions), and one to verify empty-account
fallback (call determine_writeback_target with connected_accounts = [] and any
task_context and assert it returns "default_system_caldav"); update the
test_determine_writeback_target in backend/tests/test_caldav.py to include these
assertions referencing caldav_service.determine_writeback_target.
In `@backend/tests/test_ontology.py`:
- Around line 4-11: The test should include a mixed-case domain input to ensure
domain classification is case-insensitive: update
test_analyze_sender_relationship to call
ontology_service.analyze_sender_relationship with a sender like
"SeongHo@Company.COM" or recipient "Newsletter@Marketing.COM" (matching the
existing newsletter case) and assert the same expected outputs (e.g., type
"Newsletter" and confidence 0.9) as the lowercase case; reference the existing
test function name test_analyze_sender_relationship and the method
ontology_service.analyze_sender_relationship to locate where to add this
assertion.
🪄 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: a78f85a9-344c-4bce-908e-6adb66db8dec
⛔ Files ignored due to path filters (50)
backend/__pycache__/import_fixtures.cpython-310.pycis excluded by!**/*.pycbackend/__pycache__/main.cpython-310.pycis excluded by!**/*.pycbackend/api/__pycache__/accounts.cpython-310.pycis excluded by!**/*.pycbackend/api/__pycache__/dav.cpython-310.pycis excluded by!**/*.pycbackend/api/__pycache__/emails.cpython-310.pycis excluded by!**/*.pycbackend/api/__pycache__/ontology.cpython-310.pycis excluded by!**/*.pycbackend/api/__pycache__/runner_ws.cpython-310.pycis excluded by!**/*.pycbackend/core/__pycache__/config.cpython-310.pycis excluded by!**/*.pycbackend/db/__pycache__/models.cpython-310.pycis excluded by!**/*.pycbackend/scripts/__pycache__/bootstrap_db.cpython-310.pycis excluded by!**/*.pycbackend/scripts/__pycache__/import_fixtures.cpython-310.pycis excluded by!**/*.pycbackend/services/__pycache__/access_policy.cpython-310.pycis excluded by!**/*.pycbackend/services/__pycache__/archive.cpython-310.pycis excluded by!**/*.pycbackend/services/__pycache__/calendar_sync.cpython-310.pycis excluded by!**/*.pycbackend/services/__pycache__/knowledge_extractor.cpython-310.pycis excluded by!**/*.pycbackend/services/__pycache__/text_safety.cpython-310.pycis excluded by!**/*.pycbackend/services/__pycache__/threading_service.cpython-310.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_access_policy.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_accounts_api.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_apm_observability.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_archive.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_auth_real.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_bootstrap_db.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_calendar_api.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_calendar_service.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_calendar_sync.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_config.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_dav_api.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_email_client.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_email_client_smtp.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_email_parser.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_emails_api.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_import_fixtures.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_knowledge_extractor.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_llm_api.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_llm_providers_api.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_llm_service.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_main.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_network_api.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_ontology_api.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_prompts_api.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_release_governance.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_repo_hygiene.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_runtime_config_api.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_search.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_tasks_api.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_tenant_config_api.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_tenant_config_model.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_text_safety.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_threading_service.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pyc
📒 Files selected for processing (13)
.github/workflows/strix.ymlAGENTS.mdbackend/api/caldav_sync.pybackend/requirements.txtbackend/runner/connector.pybackend/services/caldav_service.pybackend/services/email_service.pybackend/services/ontology_service.pybackend/tests/test_caldav.pybackend/tests/test_ontology.pydocker-compose.infra.ymlfrontend/src/components/EmailDetail.tsxfrontend/src/components/EmailList.tsx
| import pytest | ||
| from fastapi.testclient import TestClient | ||
| from main import app | ||
| from db.session import get_db |
| @@ -0,0 +1,28 @@ | |||
| from fastapi import APIRouter, Depends, HTTPException, status | |||
…, auth, and docker compose
| async def handle_message(self, message: str | bytes): | ||
| # Dispatch message to internal SMTP/IMAP proxy handlers | ||
| logger.debug(f"Received instruction from gateway: {message}") | ||
| pass |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
seonghobae
enabled auto-merge
May 24, 2026 21:00
Contributor
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 (2)
backend/tests/test_ontology.py (1)
13-15:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert confidence for the case-insensitive colleague scenario as well.
Line 14 validates only
type, so a confidence regression in the same path would pass unnoticed. Addassert result3["confidence"] == 0.85.🤖 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_ontology.py` around lines 13 - 15, Add an assertion for the confidence value in the case-insensitive colleague test: after calling ontology_service.analyze_sender_relationship (the call assigned to result3), add an assertion that result3["confidence"] == 0.85 so the test validates both type and confidence for the "seongho@company.com" vs "Boss@Company.com" scenario and will catch regressions to the confidence score.frontend/src/components/TasksLayout.tsx (1)
37-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDragged tasks keep their old
statusvalue after column move.Line 49 pushes
movedTaskas-is into the new column, sotask.statusbecomes inconsistent with its actual column and downstream views render stale status badges.Minimal fix
const [movedTask] = sourceList.splice(taskIndex, 1); - targetList.push(movedTask); + targetList.push({ ...movedTask, status: targetCol });🤖 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/TasksLayout.tsx` around lines 37 - 55, The drop handler currently pushes movedTask unchanged so its status remains the old column; in handleDrop update the task's status to the target column before adding it to targetList (e.g. create a newTask = { ...movedTask, status: targetCol } rather than pushing movedTask) to avoid stale status badges and to avoid mutating the original object; then push newTask into targetList and return the updated state via setTasks as you already do.
♻️ Duplicate comments (3)
backend/services/caldav_service.py (1)
17-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse right-split for safer domain extraction.
Line 18 uses
split("@")[1], which can select the wrong segment when multiple@exist and cause misrouting. Usersplit("@", 1)[-1]on normalized input.Proposed fix
- if isinstance(source_email, str) and "@" in source_email: - source_domain = source_email.split("@")[1].lower().strip() + if isinstance(source_email, str) and "@" in source_email: + source_domain = source_email.strip().lower().rsplit("@", 1)[-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/services/caldav_service.py` around lines 17 - 18, The domain extraction using source_email.split("@")[1] is unsafe for emails containing multiple '@'; update the logic that computes source_domain (where source_email is checked with isinstance(source_email, str) and "@" in source_email) to normalize the input (lower().strip()) and then use rsplit("@", 1)[-1] to reliably take the rightmost domain segment (retain the existing isinstance and "@" guard and assign back to source_domain).backend/services/email_service.py (1)
33-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle recipient lists without stringifying and normalize before compare.
Line 34 converts
recipientsto a single string, which can break parsing for list inputs. Line 39 also compares addresses case-sensitively. Normalize both sender/recipient/user addresses and preserve list inputs forgetaddresses.Proposed fix
def process_self_to_self(email_data: Dict[str, Any], user_email: str) -> bool: @@ - sender_raw = str(email_data.get("sender") or "") - recipients_raw = str(email_data.get("recipients") or "") + sender_raw = str(email_data.get("sender") or "") + recipients_raw = email_data.get("recipients") or [] + recipient_inputs = recipients_raw if isinstance(recipients_raw, list) else [recipients_raw] + recipient_inputs = [str(v) for v in recipient_inputs] @@ - _, sender_addr = email.utils.parseaddr(sender_raw) - parsed_recipients = [addr for _, addr in email.utils.getaddresses([recipients_raw])] + _, sender_addr = email.utils.parseaddr(sender_raw) + normalized_user = user_email.strip().lower() + normalized_sender = sender_addr.strip().lower() + parsed_recipients = { + addr.strip().lower() + for _, addr in email.utils.getaddresses(recipient_inputs) + if addr + } @@ - if user_email == sender_addr and user_email in parsed_recipients: + if normalized_user and normalized_user == normalized_sender and normalized_user in parsed_recipients: logger.info("Self-to-self email detected. Organizing as knowledge node.") return TrueAlso applies to: 36-40
🤖 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_service.py` around lines 33 - 34, The code currently stringifies recipients and sender (sender_raw, recipients_raw) which breaks list inputs and uses case-sensitive comparisons; update the email parsing in email_service.py to avoid converting recipients to a single string—keep the original list if present or join safely only for display—and pass the preserved list into email.utils.getaddresses; normalize all addresses (sender, each recipient, and the stored user address) to a canonical form (trim whitespace and lowercase the mailbox part) before comparing in the logic that determines ownership, and replace direct equality checks with comparisons against the normalized forms so comparisons are case-insensitive and robust to list inputs.backend/tests/test_apm_observability.py (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused import.
The
import osstatement is not used anywhere in the file.🧹 Proposed fix
-import os from pathlib import 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/tests/test_apm_observability.py` at line 1, Remove the unused import statement "import os" from the top of the test file (backend/tests/test_apm_observability.py); simply delete the line that declares the os import so there are no unused imports in the module.
🧹 Nitpick comments (3)
backend/tests/test_webdav_api.py (1)
16-30: ⚡ Quick winThese tests are coupled to service fixture data, not just API contract.
If service internals change (e.g., DB-backed values), these tests may fail for the wrong reason. Prefer stubbing
webdav_servicemethods in the test to keep API tests deterministic.Example refactor
+from services.webdav_service import webdav_service + +@pytest.fixture(autouse=True) +def stub_webdav_service(monkeypatch): + monkeypatch.setattr( + webdav_service, + "get_connected_accounts", + lambda user_id: [{"account_id": 1, "server_url": "https://example.test", "username": "alice"}], + ) + monkeypatch.setattr( + webdav_service, + "get_project_folders", + lambda user_id: [{"folder_id": 1, "project_name": "Project A", "webdav_path": "/Projects/A"}], + )🤖 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_webdav_api.py` around lines 16 - 30, The tests test_get_webdav_accounts and test_get_project_folders are tightly coupled to service fixture data; replace that coupling by stubbing the webdav_service methods used by the API (e.g., webdav_service.get_accounts and webdav_service.get_folders) so the tests assert the API contract only. In each test, patch the service methods (via monkeypatch or a pytest fixture) to return deterministic payloads matching the existing assertions, call the endpoints with auth_client, and keep the same response assertions; ensure you revert/override the real webdav_service usage so DB or internal changes won't affect these API contract tests.frontend/tests/e2e/mobile-hamburger.spec.ts (2)
6-34: 💤 Low valueConsider testing backdrop-click-to-close behavior.
The test validates closing via the dedicated close button, but the backdrop element also has an
onClickhandler that should close the menu. Consider adding a test case that clicks the backdrop to verify this alternative close path works correctly.🧪 Suggested additional test case
test('Hamburger menu closes when backdrop is clicked', async ({ page }) => { await page.goto('/'); const hamburgerBtn = page.getByRole('button', { name: '워크스페이스 메뉴 열기' }); await hamburgerBtn.click(); const menu = page.locator('`#mobile-workspace-menu`'); await expect(menu).toBeVisible(); // Close by clicking backdrop const backdrop = page.getByTestId('mobile-workspace-backdrop'); await backdrop.click(); await expect(menu).not.toBeVisible(); await expect(hamburgerBtn).toHaveAttribute('aria-expanded', '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/tests/e2e/mobile-hamburger.spec.ts` around lines 6 - 34, Add a new E2E test that verifies the backdrop click closes the mobile hamburger menu: navigate to the root, use the existing hamburgerBtn locator (page.getByRole('button', { name: '워크스페이스 메뉴 열기' })) to open the menu, assert menu (locator '`#mobile-workspace-menu`') is visible, then click the backdrop (page.getByTestId('mobile-workspace-backdrop')) and assert the menu is hidden and hamburgerBtn has aria-expanded='false'; mirror the style and assertions used in the existing test to keep consistency.
26-27: 💤 Low valueRemove or implement the TODO comment.
The comment mentions an optional body scroll check. Either implement this validation or remove the comment to keep the test file clean.
🤖 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/tests/e2e/mobile-hamburger.spec.ts` around lines 26 - 27, In the mobile-hamburger.spec.ts test remove the TODO-style comment or implement the body-scroll validation: either delete the two comment lines about the optional body scroll check, or add an assertion after opening the hamburger/popover that the page scrolling is prevented (for example assert document.body.style.overflow contains 'hidden' or attempt a programmatic scroll and verify document.body.scrollTop/scrollY doesn't change) and then proceed to close via the close button; update any selectors referenced in the test to the existing popover/hamburger and close button helpers so the new check runs reliably.
🤖 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 @.github/workflows/strix.yml:
- Line 85: The workflow sets the Strix model variable strix_llm to
"github_models/gpt-5.4" (appearing in the Gate Strix secrets, Gate model auth
prerequisites, and Prepare Strix model input file steps); change each occurrence
of the value for strix_llm from "github_models/gpt-5.4" to
"github_models/gpt-4o" so the pipeline uses the required gpt-4o model for Strix
scans and respects the PR intent and coding guidelines.
In `@backend/runner/connector.py`:
- Around line 54-59: The broad except Exception in
SelfHostedConnector._listen_loop is masking programming errors from
handle_message; narrow it to catch only expected shutdown exceptions (e.g.,
websockets.exceptions.ConnectionClosed and asyncio.CancelledError) and handle
them by logging the connection-close path and setting self.is_connected = False,
but for any other Exception log the full traceback (use logger.exception) and
re-raise so bugs aren't suppressed; update _listen_loop to import asyncio if
needed and ensure self.is_connected is cleared in the shutdown path or a finally
block.
In `@backend/services/webdav_service.py`:
- Around line 10-40: Both get_connected_accounts and get_project_folders
currently ignore the user_id parameter and always return shared hardcoded data,
which can cause cross-tenant exposure; update the two functions
(get_connected_accounts and get_project_folders) to scope results to the
provided user_id by using a per-user mock store or filtering the existing mock
list by a user identifier (e.g., add a user_id field to each mock entry or
maintain a dict mapping user_id -> list), return an empty list or raise a clear
error when no data exists for that user, and ensure the returned shapes remain
the same so callers (e.g., /api/webdav/accounts and /api/webdav/folders) receive
user-scoped results.
In `@frontend/src/components/TasksLayout.tsx`:
- Around line 159-205: The detail pane is not bound to the clicked task;
clicking items only calls setViewMode('작업 상세') and the "작업 상세" block renders
hardcoded values. Add a piece of state (e.g. selectedTaskId via useState) and
update the click handlers in both task lists to call setSelectedTaskId(task.id)
before setViewMode('작업 상세'); then, inside the "작업 상세" rendering, look up the
task from the tasks collection (e.g. const task =
Object.values(tasks).flat().find(t => t.id === selectedTaskId) or
tasks[selectedTaskId]) and render its fields (title, id, priority, due,
assignee, status, source) instead of hardcoded strings; also handle null
selectedTaskId (render a placeholder or return) and clear selectedTaskId when
leaving the detail view or switching views.
In `@frontend/tests/e2e/mobile-hamburger.spec.ts`:
- Around line 44-45: The test is checking paddingBottom on the bottomNav element
which is a false positive due to the p-2 class; instead evaluate the element's
positioning: replace the paddingBottom check with reading
window.getComputedStyle(el).bottom (using the bottomNav variable used in the
spec) and assert that the bottom value reflects the safe-area calc (e.g.
contains "calc" or is not "0px"); alternatively assert the element's classList
contains "bottom-[calc(0.75rem+env(safe-area-inset-bottom))]" to ensure the
safe-area positioning is applied.
---
Outside diff comments:
In `@backend/tests/test_ontology.py`:
- Around line 13-15: Add an assertion for the confidence value in the
case-insensitive colleague test: after calling
ontology_service.analyze_sender_relationship (the call assigned to result3), add
an assertion that result3["confidence"] == 0.85 so the test validates both type
and confidence for the "seongho@company.com" vs "Boss@Company.com" scenario and
will catch regressions to the confidence score.
In `@frontend/src/components/TasksLayout.tsx`:
- Around line 37-55: The drop handler currently pushes movedTask unchanged so
its status remains the old column; in handleDrop update the task's status to the
target column before adding it to targetList (e.g. create a newTask = {
...movedTask, status: targetCol } rather than pushing movedTask) to avoid stale
status badges and to avoid mutating the original object; then push newTask into
targetList and return the updated state via setTasks as you already do.
---
Duplicate comments:
In `@backend/services/caldav_service.py`:
- Around line 17-18: The domain extraction using source_email.split("@")[1] is
unsafe for emails containing multiple '@'; update the logic that computes
source_domain (where source_email is checked with isinstance(source_email, str)
and "@" in source_email) to normalize the input (lower().strip()) and then use
rsplit("@", 1)[-1] to reliably take the rightmost domain segment (retain the
existing isinstance and "@" guard and assign back to source_domain).
In `@backend/services/email_service.py`:
- Around line 33-34: The code currently stringifies recipients and sender
(sender_raw, recipients_raw) which breaks list inputs and uses case-sensitive
comparisons; update the email parsing in email_service.py to avoid converting
recipients to a single string—keep the original list if present or join safely
only for display—and pass the preserved list into email.utils.getaddresses;
normalize all addresses (sender, each recipient, and the stored user address) to
a canonical form (trim whitespace and lowercase the mailbox part) before
comparing in the logic that determines ownership, and replace direct equality
checks with comparisons against the normalized forms so comparisons are
case-insensitive and robust to list inputs.
In `@backend/tests/test_apm_observability.py`:
- Line 1: Remove the unused import statement "import os" from the top of the
test file (backend/tests/test_apm_observability.py); simply delete the line that
declares the os import so there are no unused imports in the module.
---
Nitpick comments:
In `@backend/tests/test_webdav_api.py`:
- Around line 16-30: The tests test_get_webdav_accounts and
test_get_project_folders are tightly coupled to service fixture data; replace
that coupling by stubbing the webdav_service methods used by the API (e.g.,
webdav_service.get_accounts and webdav_service.get_folders) so the tests assert
the API contract only. In each test, patch the service methods (via monkeypatch
or a pytest fixture) to return deterministic payloads matching the existing
assertions, call the endpoints with auth_client, and keep the same response
assertions; ensure you revert/override the real webdav_service usage so DB or
internal changes won't affect these API contract tests.
In `@frontend/tests/e2e/mobile-hamburger.spec.ts`:
- Around line 6-34: Add a new E2E test that verifies the backdrop click closes
the mobile hamburger menu: navigate to the root, use the existing hamburgerBtn
locator (page.getByRole('button', { name: '워크스페이스 메뉴 열기' })) to open the menu,
assert menu (locator '`#mobile-workspace-menu`') is visible, then click the
backdrop (page.getByTestId('mobile-workspace-backdrop')) and assert the menu is
hidden and hamburgerBtn has aria-expanded='false'; mirror the style and
assertions used in the existing test to keep consistency.
- Around line 26-27: In the mobile-hamburger.spec.ts test remove the TODO-style
comment or implement the body-scroll validation: either delete the two comment
lines about the optional body scroll check, or add an assertion after opening
the hamburger/popover that the page scrolling is prevented (for example assert
document.body.style.overflow contains 'hidden' or attempt a programmatic scroll
and verify document.body.scrollTop/scrollY doesn't change) and then proceed to
close via the close button; update any selectors referenced in the test to the
existing popover/hamburger and close button helpers so the new check runs
reliably.
🪄 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: b8d2dd03-a0c4-4704-8ec0-128cb3a05f09
⛔ Files ignored due to path filters (5)
backend/__pycache__/main.cpython-310.pycis excluded by!**/*.pycbackend/db/__pycache__/models.cpython-310.pycis excluded by!**/*.pycbackend/scripts/__pycache__/bootstrap_db.cpython-310.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_apm_observability.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_search.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pyc
📒 Files selected for processing (22)
.github/workflows/strix.yml.vooster/project.jsonbackend/api/webdav.pybackend/db/models.pybackend/main.pybackend/runner/connector.pybackend/schema/connector.pybackend/services/caldav_service.pybackend/services/email_service.pybackend/services/ontology_service.pybackend/services/webdav_service.pybackend/tests/test_apm_observability.pybackend/tests/test_caldav.pybackend/tests/test_ontology.pybackend/tests/test_webdav_api.pydocker-compose.infra.ymldocs/plans/2026-05-24-architecture-implementation.mddocs/plans/2026-05-24-branding-implementation.mdfrontend/src/components/DataLayout.tsxfrontend/src/components/EmailList.tsxfrontend/src/components/TasksLayout.tsxfrontend/tests/e2e/mobile-hamburger.spec.ts
Comment on lines
+54
to
+59
| except Exception as e: | ||
| if websockets and isinstance(e, websockets.exceptions.ConnectionClosed): | ||
| logger.warning("Connection closed by remote gateway.") | ||
| else: | ||
| logger.warning(f"Connection loop ended: {e}") | ||
| self.is_connected = False |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect broad catches in connector:"
rg -nP --type=py -C2 '\bexcept\s+Exception\b' backend/runner/connector.py
echo
echo "Inspect listen loop region:"
cat -n backend/runner/connector.py | sed -n '47,70p'Repository: Seongho-Bae/naruon
Length of output: 1890
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Top of backend/runner/connector.py (imports + class header):"
cat -n backend/runner/connector.py | sed -n '1,120p'
echo
echo "Confirm whether asyncio is imported:"
rg -n --type=py '\bimport\s+asyncio\b|\bfrom\s+asyncio\s+import\b' backend/runner/connector.py
echo
echo "Confirm websockets import:"
rg -n --type=py '\bimport\s+websockets\b|\bfrom\s+websockets\s+import\b' backend/runner/connector.pyRepository: Seongho-Bae/naruon
Length of output: 3720
Narrow SelfHostedConnector._listen_loop exception handling to avoid masking logic bugs.
In backend/runner/connector.py lines 54-59, _listen_loop catches all Exception, logs a warning, and stops the loop—this suppresses failures from handle_message and can hide programming errors.
♻️ Proposed fix
async def _listen_loop(self):
if not self.connection:
return
try:
while self.is_connected:
message = await self.connection.recv()
await self.handle_message(message)
- except Exception as e:
- if websockets and isinstance(e, websockets.exceptions.ConnectionClosed):
+ except asyncio.CancelledError:
+ raise
+ except (OSError, asyncio.TimeoutError) as e:
+ logger.warning(f"Connection loop ended: {e}")
+ self.is_connected = False
+ except Exception as e:
+ if websockets and isinstance(e, websockets.exceptions.ConnectionClosed):
logger.warning("Connection closed by remote gateway.")
+ self.is_connected = False
else:
- logger.warning(f"Connection loop ended: {e}")
- self.is_connected = False
+ raise🧰 Tools
🪛 Ruff (0.15.13)
[warning] 54-54: Do not catch blind exception: Exception
(BLE001)
🤖 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/runner/connector.py` around lines 54 - 59, The broad except Exception
in SelfHostedConnector._listen_loop is masking programming errors from
handle_message; narrow it to catch only expected shutdown exceptions (e.g.,
websockets.exceptions.ConnectionClosed and asyncio.CancelledError) and handle
them by logging the connection-close path and setting self.is_connected = False,
but for any other Exception log the full traceback (use logger.exception) and
re-raise so bugs aren't suppressed; update _listen_loop to import asyncio if
needed and ensure self.is_connected is cleared in the shutdown path or a finally
block.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/tests/e2e/mobile-hamburger.spec.ts (1)
33-54:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the unused
overflowvariable.Line 43 assigns
overflowbut never uses it. Remove the dead code to clean up the test.🧹 Proposed fix
- // Ensure it prevents body scroll - const overflow = await page.evaluate(() => document.body.style.overflow); - // Depending on implementation, it may use popover or explicit overflow hidden - // We will just verify the close button works - // Close by clicking the close button🤖 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/tests/e2e/mobile-hamburger.spec.ts` around lines 33 - 54, In the "Hamburger menu closes when close button is clicked" test remove the unused overflow variable and its evaluation: delete the const overflow = await page.evaluate(() => document.body.style.overflow); line (and any immediately related dead comment if you want) so the test no longer defines an unused symbol; keep the rest of the assertions (menu, closeBtn, hamburgerBtn) unchanged.
🤖 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 `@frontend/tests/e2e/mobile-hamburger.spec.ts`:
- Around line 63-64: The assertion is too weak; instead of checking bottomVal
!== '0px', parse the computed bottom into a numeric pixel value and assert it
meets the minimum safe-area offset (0.75rem ≈ 12px). Update the test around
bottomNav.evaluate so that bottomVal is parsed (e.g., parseFloat or Number) with
a fallback to 0 for non-numeric values like 'auto', then expect the numeric
value to be >= 12 (pixels).
---
Outside diff comments:
In `@frontend/tests/e2e/mobile-hamburger.spec.ts`:
- Around line 33-54: In the "Hamburger menu closes when close button is clicked"
test remove the unused overflow variable and its evaluation: delete the const
overflow = await page.evaluate(() => document.body.style.overflow); line (and
any immediately related dead comment if you want) so the test no longer defines
an unused symbol; keep the rest of the assertions (menu, closeBtn, hamburgerBtn)
unchanged.
🪄 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: f37302cf-9aa7-43ec-8842-08e175b13e91
⛔ Files ignored due to path filters (1)
backend/tests/__pycache__/test_apm_observability.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pyc
📒 Files selected for processing (8)
backend/services/caldav_service.pybackend/services/email_service.pybackend/services/webdav_service.pybackend/tests/test_apm_observability.pybackend/tests/test_ontology.pybackend/tests/test_webdav_api.pyfrontend/src/components/TasksLayout.tsxfrontend/tests/e2e/mobile-hamburger.spec.ts
💤 Files with no reviewable changes (1)
- backend/tests/test_apm_observability.py
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes Strix scanner failure caused by unsupported gpt-5.4 model.
Summary by CodeRabbit
New Features
Tests
Documentation