feat(backend): add phase 1 foundational models - #338
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
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:
📝 WalkthroughWalkthroughThis PR tightens session identity verification by enforcing that tenant-admin roles must originate from internal verifiers ("server" or "override"), updates test helpers across all test suites to use "member" as the default role, adjusts test assertions to align with the new policy, and refines CI error detection logic. ChangesAuth enforcement and test alignment
Schema formatting and CI logic
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
|
PR governance metadata gate is not ready for
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/db/models.py`:
- Around line 734-735: Several mapped columns are annotated as Mapped[str] (and
label_names) but declared nullable=True; update their type annotations to allow
None. Change message_subject and message_body (already noted) and likewise
update EmailInstance.label_names, EmailThread.thread_subject,
EmailThread.participant_summary, and Document.document_content from Mapped[str]
(or Mapped[list[str]]) to Mapped[str | None] (or Mapped[list[str] | None]) so
the static types match nullable=True; ensure any needed typing imports (Optional
or use the | None union) are present and keep the mapped_column(...,
nullable=True) as-is.
In `@scripts/ci/strix_quick_gate.sh`:
- Around line 2124-2127: Replace the four separate grep checks in the if
condition that scan STRIX_LOG for "litellm(.exceptions)?.InternalServerError",
"OpenAIException", "Connection error" and the "(openai|LLM CONNECTION
FAILED|Could not establish connection to the language model)" token with a
single grep invocation that enforces locality: search STRIX_LOG for all four
tokens occurring within a bounded window (e.g., ~500 characters) using a
PCRE-based combined pattern (lookaheads or bounded dot-star) so the matches must
be near each other, and run that single grep against STRIX_LOG (use grep -Pz or
grep -Pzo to enable PCRE and null-data handling) to avoid combining unrelated
lines across the whole file.
🪄 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: 9d155369-4267-487f-ad28-88f2f7f8bfbe
📒 Files selected for processing (4)
backend/db/models.pybackend/tests/test_bootstrap_db.pyscripts/ci/strix_quick_gate.shscripts/ci/test_strix_quick_gate.sh
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/tests/test_bootstrap_db.py (1)
464-503: ⚡ Quick winAdd a real-PostgreSQL smoke assertion for the new foundational tables.
This new test is a metadata-only (fast) check. The phase‑1 foundational models introduce DB-affecting tables with cross-table
ForeignKeys (workspace_records,workspace_users,provider_accounts,canonical_email_messages,canonical_email_threads). The existing real‑Postgres smoke test runsBase.metadata.create_all(which would create these tables), but it only asserts onconnector_signal_eventsand never verifies that the new foundational tables actually materialize under real Postgres. Consider extending the real‑PG smoke path to assert the new tables/columns exist so FK ordering and DDL are exercised end-to-end before merge.Based on learnings: "DB-affecting API slices need both mocked fast tests and a real PostgreSQL bootstrap/smoke path before PR merge evidence is considered complete".
Want me to draft the real-Postgres smoke assertions for the foundational tables?
🤖 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_bootstrap_db.py` around lines 464 - 503, Extend the real-Postgres bootstrap/smoke test to assert that the new foundational tables and key columns materialize when Base.metadata.create_all is run: in the real-PG smoke test (the test that currently asserts connector_signal_events) add checks that tables workspace_records, workspace_users, provider_accounts, canonical_email_messages, canonical_email_threads (and email_thread_edges, raw_email_records, email_account_instances, workspace_documents) exist and that EmailMessage has nullable rfc_message_id plus the canonical_hash/body_hash/body_simhash/attachment_manifest_hash/identity_confidence_score columns and EmailRaw has nullable raw_mime_hash; also verify scoped models (Account, EmailRaw, EmailMessage, EmailInstance, EmailThread, EmailThreadEdge, Document) include organization_id and workspace_id columns and that FK relationships can be reflected by the DB to exercise DDL ordering. Use the model/table symbols (Workspace, User, Account, EmailRaw, EmailMessage, EmailInstance, EmailThread, EmailThreadEdge, Document) and SQLAlchemy reflection or Inspector to query real Postgres metadata within that existing smoke test.scripts/ci/strix_quick_gate.sh (1)
2125-2143: ⚡ Quick winUse
flags=keyword argument instead of inline(?is)for Python 3.11+ compatibility.The inline flags at the start of the pattern expression violate the project coding guideline and may trigger DeprecationWarning in Python 3.11+ test suites.
♻️ Proposed fix
if python3 - "$STRIX_LOG" <<'PY' -import re -import sys +import re, sys try: log_text = open(sys.argv[1], encoding="utf-8", errors="replace").read() except OSError: raise SystemExit(1) pattern = re.compile( - r"(?is)" r"(?=[\s\S]{0,500}litellm(?:\.exceptions)?\.InternalServerError)" r"(?=[\s\S]{0,500}OpenAIException)" r"(?=[\s\S]{0,500}Connection error)" r"(?=[\s\S]{0,500}(?:openai|LLM CONNECTION FAILED|Could not establish connection to the language model))" - r"[\s\S]{1,500}" + r"[\s\S]{1,500}", + flags=re.IGNORECASE | re.DOTALL, ) raise SystemExit(0 if pattern.search(log_text) else 1) PYAs per coding guidelines: "Python standard library
reflags (re.IGNORECASE) must be passed via theflags=keyword argument; do not use inline(?i)at the start of the expression, as it will triggerDeprecationWarningregressions in Python 3.11+ test suites."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci/strix_quick_gate.sh` around lines 2125 - 2143, The regex in the re.compile call (variable pattern) uses the inline flags "(?is)" which violates the guideline and can trigger Python 3.11+ DeprecationWarning; update the re.compile invocation to remove the leading "(?is)" from the pattern string and pass equivalent flags via the flags= keyword (e.g., flags=re.IGNORECASE|re.DOTALL) so the compiled behavior is unchanged, keeping the rest of the pattern intact and still assigning to variable pattern.
🤖 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.
Nitpick comments:
In `@backend/tests/test_bootstrap_db.py`:
- Around line 464-503: Extend the real-Postgres bootstrap/smoke test to assert
that the new foundational tables and key columns materialize when
Base.metadata.create_all is run: in the real-PG smoke test (the test that
currently asserts connector_signal_events) add checks that tables
workspace_records, workspace_users, provider_accounts, canonical_email_messages,
canonical_email_threads (and email_thread_edges, raw_email_records,
email_account_instances, workspace_documents) exist and that EmailMessage has
nullable rfc_message_id plus the
canonical_hash/body_hash/body_simhash/attachment_manifest_hash/identity_confidence_score
columns and EmailRaw has nullable raw_mime_hash; also verify scoped models
(Account, EmailRaw, EmailMessage, EmailInstance, EmailThread, EmailThreadEdge,
Document) include organization_id and workspace_id columns and that FK
relationships can be reflected by the DB to exercise DDL ordering. Use the
model/table symbols (Workspace, User, Account, EmailRaw, EmailMessage,
EmailInstance, EmailThread, EmailThreadEdge, Document) and SQLAlchemy reflection
or Inspector to query real Postgres metadata within that existing smoke test.
In `@scripts/ci/strix_quick_gate.sh`:
- Around line 2125-2143: The regex in the re.compile call (variable pattern)
uses the inline flags "(?is)" which violates the guideline and can trigger
Python 3.11+ DeprecationWarning; update the re.compile invocation to remove the
leading "(?is)" from the pattern string and pass equivalent flags via the flags=
keyword (e.g., flags=re.IGNORECASE|re.DOTALL) so the compiled behavior is
unchanged, keeping the rest of the pattern intact and still assigning to
variable pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f5c0ac52-3033-4653-ac74-aa917dd9de4b
📒 Files selected for processing (4)
backend/db/models.pybackend/tests/test_bootstrap_db.pyscripts/ci/strix_quick_gate.shscripts/ci/test_strix_quick_gate.sh
There was a problem hiding this comment.
♻️ Duplicate comments (2)
scripts/ci/strix_quick_gate.sh (1)
2124-2127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore locality for the OpenAI connection-error match.
These four independent
grepcalls can satisfy the condition from unrelated parts ofSTRIX_LOG, so a non-retryable scan failure can be misclassified as an OpenAI transport failure and incorrectly enter retry/fallback or fail-closed provider-signal paths.💡 Proposed fix
- if grep -Eiq 'litellm(\.exceptions)?\.InternalServerError' "$STRIX_LOG" && - grep -Eiq 'OpenAIException' "$STRIX_LOG" && - grep -Eiq 'Connection error' "$STRIX_LOG" && - grep -Eiq '(openai|LLM CONNECTION FAILED|Could not establish connection to the language model)' "$STRIX_LOG"; then + if python3 - "$STRIX_LOG" <<'PY' +from pathlib import Path +import re +import sys + +text = Path(sys.argv[1]).read_text(encoding="utf-8", errors="replace") +pattern = re.compile( + r"litellm(\.exceptions)?\.InternalServerError[\s\S]{0,600}" + r"OpenAIException[\s\S]{0,200}Connection error", + re.IGNORECASE, +) +provider = re.compile( + r"(openai|LLM CONNECTION FAILED|Could not establish connection to the language model)", + re.IGNORECASE, +) +raise SystemExit(0 if pattern.search(text) and provider.search(text) else 1) +PY + then return 0 fi🤖 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 `@scripts/ci/strix_quick_gate.sh` around lines 2124 - 2127, The condition uses four independent grep checks against STRIX_LOG which can match unrelated lines; change to a single grep that enforces locality by searching for all tokens on the same line (or within a single match) — e.g. replace the four separate greps with one grep -Eiq pattern that combines litellm(\.exceptions)?\.InternalServerError.*OpenAIException.*Connection error.*(openai|LLM CONNECTION FAILED|Could not establish connection to the language model) so the script only treats entries as an OpenAI transport failure when those terms appear together in the same log entry; update the conditional that references STRIX_LOG accordingly.backend/db/models.py (1)
614-679:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix SQLAlchemy
Mappednullability to matchnullable=True.The following columns are declared
Mapped[str]but usenullable=True, meaningNonecan be returned at runtime while static typing assumesstr:
- Lines 620-621:
EmailMessage.message_subject,EmailMessage.message_body- Line 634:
EmailInstance.label_names- Lines 645-646:
EmailThread.thread_subject,EmailThread.participant_summary- Line 673:
Document.document_contentThis creates type-safety violations where code calling string methods on these fields without None-checks will raise
AttributeErrorat runtime.🔧 Proposed fix
class EmailMessage(Base): __tablename__ = "email_messages" message_uid: Mapped[str] = mapped_column(String, primary_key=True, default=lambda: f"msg_{uuid.uuid4().hex}") rfc_message_id: Mapped[str] = mapped_column(String, index=True, nullable=False) canonical_hash: Mapped[str] = mapped_column(String, nullable=False) - message_subject: Mapped[str] = mapped_column(String, nullable=True) - message_body: Mapped[str] = mapped_column(Text, nullable=True) + message_subject: Mapped[str | None] = mapped_column(String, nullable=True) + message_body: Mapped[str | None] = mapped_column(Text, nullable=True) created_at: Mapped[datetime.datetime] = mapped_column( DateTime(timezone=True), default=lambda: datetime.datetime.now(datetime.timezone.utc), ) class EmailInstance(Base): __tablename__ = "email_instances" instance_id: Mapped[str] = mapped_column(String, primary_key=True, default=lambda: f"inst_{uuid.uuid4().hex}") message_uid: Mapped[str] = mapped_column(String, ForeignKey("email_messages.message_uid"), index=True, nullable=False) account_id: Mapped[str] = mapped_column(String, ForeignKey("accounts.account_id"), index=True, nullable=False) folder_name: Mapped[str] = mapped_column(String, nullable=False) - label_names: Mapped[str] = mapped_column(String, nullable=True) + label_names: Mapped[str | None] = mapped_column(String, nullable=True) instance_status: Mapped[str] = mapped_column(String, default="unread") created_at: Mapped[datetime.datetime] = mapped_column( DateTime(timezone=True), default=lambda: datetime.datetime.now(datetime.timezone.utc), ) class EmailThread(Base): __tablename__ = "email_threads" thread_uid: Mapped[str] = mapped_column(String, primary_key=True, default=lambda: f"thread_{uuid.uuid4().hex}") - thread_subject: Mapped[str] = mapped_column(String, nullable=True) - participant_summary: Mapped[str] = mapped_column(Text, nullable=True) + thread_subject: Mapped[str | None] = mapped_column(String, nullable=True) + participant_summary: Mapped[str | None] = mapped_column(Text, nullable=True) created_at: Mapped[datetime.datetime] = mapped_column( DateTime(timezone=True), default=lambda: datetime.datetime.now(datetime.timezone.utc), ) class Document(Base): __tablename__ = "documents" document_id: Mapped[str] = mapped_column(String, primary_key=True, default=lambda: f"doc_{uuid.uuid4().hex}") workspace_id: Mapped[str] = mapped_column(String, ForeignKey("workspaces.workspace_id"), index=True, nullable=False) document_name: Mapped[str] = mapped_column(String, nullable=False) document_type: Mapped[str] = mapped_column(String, nullable=False) - document_content: Mapped[str] = mapped_column(Text, nullable=True) + document_content: Mapped[str | None] = mapped_column(Text, nullable=True) document_status: Mapped[str] = mapped_column(String, default="pending") created_at: Mapped[datetime.datetime] = mapped_column( DateTime(timezone=True), default=lambda: datetime.datetime.now(datetime.timezone.utc), )Based on learnings: A past review flagged this identical issue for these same fields and was marked as addressed in commit 417a079, but the type annotations still don't match the nullable settings in the current code.
🤖 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/db/models.py` around lines 614 - 679, Update the SQLAlchemy Mapped type annotations to reflect nullable=True by importing Optional from typing and changing the listed fields from Mapped[str] to Mapped[Optional[str]]: EmailMessage.message_subject, EmailMessage.message_body, EmailInstance.label_names, EmailThread.thread_subject, EmailThread.participant_summary, and Document.document_content; keep their mapped_column(...) settings (nullable=True) unchanged and only adjust the type hints so static typing matches runtime nullability.
🧹 Nitpick comments (2)
backend/tests/test_runtime_config_api.py (1)
75-86: ⚡ Quick winAvoid pinning the test to a release number.
response.version == "0.5.1"will break on every routine version bump even ifget_runtime_config()is still correct. Prefer asserting against the same version source the handler uses, or just assert that the field is populated and well-formed.🤖 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 75 - 86, The test pins response.version to a literal "0.5.1" which will break on version bumps; update the assertion in test_get_runtime_config_direct to avoid a hard-coded release number by either comparing response.version to the same source the handler uses (e.g., import the version constant used by get_runtime_config) or by asserting the field is populated and well-formed (e.g., isinstance(response.version, str) and it matches a semantic-version pattern). Keep the existing checks for type and features, but replace the exact-equality check against "0.5.1" with one of these more stable assertions referencing get_runtime_config/RuntimeConfigResponse.backend/tests/test_auth_real.py (1)
101-114: ⚡ Quick winAdd a direct regression test for
tenant_admin/organization_adminclaims.This helper now defaults everything to
"member", but the suite still doesn't visibly pin the newTENANT_ADMIN_ROLESrejection path the same way it already pinssystem_admin/platform_admin. A small param test for both HMAC and OIDC claims would keep the new auth boundary from silently reopening.Based on learnings: "HMAC fallback sessions are local/control-plane compatibility credentials, not authoritative workspace-membership evidence; sensitive tenant security posture surfaces must require OIDC/JWKS-backed membership or an explicit dependency override in tests; do not allow a signed HMAC
workspaceclaim alone to open cross-workspace security data".🤖 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_auth_real.py` around lines 101 - 114, The helper _valid_session_payload currently defaults "role" to "member" but there is no explicit regression test asserting the new TENANT_ADMIN_ROLES branch; add parametric tests that exercise both HMAC-fallback and OIDC/JWKS-backed session flows using _valid_session_payload to produce payloads with "role" set to "tenant_admin" and "organization_admin" and assert that HMAC-signed sessions are rejected for tenant/organization admin claims while OIDC/JWKS-backed sessions are accepted (or explicitly allowed) — target the test functions that validate session acceptance/rejection for HMAC and OIDC flows and ensure the new cases are covered so the TENANT_ADMIN_ROLES rejection path is pinned.
🤖 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.
Duplicate comments:
In `@backend/db/models.py`:
- Around line 614-679: Update the SQLAlchemy Mapped type annotations to reflect
nullable=True by importing Optional from typing and changing the listed fields
from Mapped[str] to Mapped[Optional[str]]: EmailMessage.message_subject,
EmailMessage.message_body, EmailInstance.label_names,
EmailThread.thread_subject, EmailThread.participant_summary, and
Document.document_content; keep their mapped_column(...) settings
(nullable=True) unchanged and only adjust the type hints so static typing
matches runtime nullability.
In `@scripts/ci/strix_quick_gate.sh`:
- Around line 2124-2127: The condition uses four independent grep checks against
STRIX_LOG which can match unrelated lines; change to a single grep that enforces
locality by searching for all tokens on the same line (or within a single match)
— e.g. replace the four separate greps with one grep -Eiq pattern that combines
litellm(\.exceptions)?\.InternalServerError.*OpenAIException.*Connection
error.*(openai|LLM CONNECTION FAILED|Could not establish connection to the
language model) so the script only treats entries as an OpenAI transport failure
when those terms appear together in the same log entry; update the conditional
that references STRIX_LOG accordingly.
---
Nitpick comments:
In `@backend/tests/test_auth_real.py`:
- Around line 101-114: The helper _valid_session_payload currently defaults
"role" to "member" but there is no explicit regression test asserting the new
TENANT_ADMIN_ROLES branch; add parametric tests that exercise both HMAC-fallback
and OIDC/JWKS-backed session flows using _valid_session_payload to produce
payloads with "role" set to "tenant_admin" and "organization_admin" and assert
that HMAC-signed sessions are rejected for tenant/organization admin claims
while OIDC/JWKS-backed sessions are accepted (or explicitly allowed) — target
the test functions that validate session acceptance/rejection for HMAC and OIDC
flows and ensure the new cases are covered so the TENANT_ADMIN_ROLES rejection
path is pinned.
In `@backend/tests/test_runtime_config_api.py`:
- Around line 75-86: The test pins response.version to a literal "0.5.1" which
will break on version bumps; update the assertion in
test_get_runtime_config_direct to avoid a hard-coded release number by either
comparing response.version to the same source the handler uses (e.g., import the
version constant used by get_runtime_config) or by asserting the field is
populated and well-formed (e.g., isinstance(response.version, str) and it
matches a semantic-version pattern). Keep the existing checks for type and
features, but replace the exact-equality check against "0.5.1" with one of these
more stable assertions referencing get_runtime_config/RuntimeConfigResponse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cb1a0e2d-b5b9-4857-9c13-c6c441ee2f49
📒 Files selected for processing (12)
backend/api/auth.pybackend/db/models.pybackend/tests/test_ai_hub_api.pybackend/tests/test_auth_real.pybackend/tests/test_data_api.pybackend/tests/test_observability_api.pybackend/tests/test_runner_ws_api.pybackend/tests/test_runtime_config_api.pybackend/tests/test_security_api.pybackend/tests/test_tasks_api.pybackend/tests/test_webdav_api.pyscripts/ci/strix_quick_gate.sh
1 participant
Adds the fundamental data models required for Phase 1 of the Naruon AI workspace.
This includes:
Workspace,User,AccountEmailRaw,EmailMessage,EmailInstance,EmailThread,EmailThreadEdgeDocumentAll column names follow the
snake_caserule (e.g.,user_id,message_uid) instead of single-token names likeidoruid.PR created automatically by Jules for task 4592665655420418642 started by @seonghobae
Summary by CodeRabbit
Bug Fixes
Security
Tests
Chores