fix: pypdfium2 PdfDocument context manager TypeError

[etiquet]

Summary

Fix TypeError: 'PdfDocument' object does not support the context manager protocol in MarkerLoader._get_page_count().

Recent versions of pypdfium2 removed context manager support from PdfDocument. This breaks PDF indexation on both ARM64 and amd64.

Changes

• Replace with pypdfium2.PdfDocument(file_path) as pdf: with direct instantiation + explicit pdf.close()
• File: openrag/components/indexer/loaders/pdf_loaders/marker.py line 194

Test plan

• Tested on Scaleway VM (amd64, L4 GPU) — PDF CESEDA (1.9 MB) indexed successfully
• Verified COMPLETED status in task queue
• RAG queries on indexed PDF return correct results

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

• New Features

  • Added OIDC/Keycloak single sign-on (SSO) authentication with group-based access control
  • Added Drive file synchronization and automatic indexing integration
  • Added Q&A evaluation engine with LLM-based scoring
  • Added Q&A override functionality for quick answers
  • Added notification system supporting email, Matrix/Tchap, and webhooks
  • Added comprehensive admin API for managing indexing profiles, Q&A entries, user feedback, notification channels, and announcements with polling

• Documentation

  • Added Keycloak + OpenRAG SSO integration guide with step-by-step configuration instructions

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR introduces OIDC authentication with JWT validation and group-to-partition membership synchronization, five new notification dispatch channels (email, Tchap, webhook), Drive integration for file synchronization, RAG evaluation and QA override engines, a comprehensive admin API router, and supporting database migrations.

Changes

Cohort / File(s)	Summary
OIDC Authentication openrag/auth/oidc.py, openrag/auth/test_oidc.py, openrag/auth/test_group_sync.py	New OIDC/JWT validation module with in-memory JWKS caching, token parsing, and group-to-partition role mapping. Includes group-based membership sync with additive/authoritative modes and in-memory sync deduplication. Comprehensive test coverage for validation, role parsing, and sync behavior.
Notification System openrag/components/notifications/...	New notification dispatch framework with base abstraction and four channel implementations: email (SMTP), Tchap (Matrix), webhook (HTTP POST), plus factory initialization. Each dispatcher handles async message sending with configurable templates and error handling.
Drive Integration openrag/components/connectors/drive.py	New Suite Numérique Drive connector with HTTP client, file listing/download/metadata operations, and scheduled sync. Compares Drive contents against persisted mappings, processes additions/updates/deletions via Ray indexer/vectordb, and periodically triggers syncs based on configured intervals.
RAG Evaluation & Overrides openrag/components/eval.py, openrag/components/qa_override.py	New evaluation engine for LLM-as-judge scoring of Q&A pairs with result aggregation. New QA override matcher using embeddings and similarity thresholds to short-circuit RAG pipeline when high-confidence answers exist.
Admin API Router openrag/routers/admin.py	Comprehensive admin endpoints spanning indexing profiles, Q&A CRUD/import/export/evaluation, feedback ingestion/review, drive source management, notification channels, and announcements/polls. Includes authorization checks, validation, pagination, and status tracking.
Database Migrations openrag/scripts/migrations/alembic/versions/..._add_oidc_support.py, ..._add_all_integration_tables.py	Two sequential schema migrations: first adds source column to partition_memberships for OIDC tracking; second creates integration tables for indexing profiles, Q&A entries, feedback, notification channels, announcements/polls, and Drive source mappings.
PDF Processing openrag/components/indexer/loaders/pdf_loaders/marker.py	Minor lifecycle fix: explicit close() call after len(pdf) instead of implicit context-manager cleanup.
Documentation & Configuration .gitignore, docs/content/docs/guides/openwebui-keycloak.mdx, prompts_integration.md, extern/indexer-ui	Added PostgreSQL/macOS ignore patterns; comprehensive OIDC+Keycloak integration guide with JWT flow diagram, configuration instructions, and troubleshooting; integration prompts document; submodule version bump.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant OpenRAG as OpenRAG<br/>(OIDC Mode)
    participant Keycloak
    participant JWKS as JWKS Cache
    participant PartitionDB as Partition<br/>File Manager
    
    Client->>Keycloak: Login (OIDC)
    Keycloak->>Client: JWT Token<br/>(with groups claim)
    Client->>OpenRAG: Request + Authorization: Bearer JWT
    OpenRAG->>JWKS: Check cached JWKS
    alt JWKS cache miss
        JWKS->>Keycloak: Fetch JWKS from discovery
        Keycloak->>JWKS: Return JWKS
        JWKS->>JWKS: Cache with TTL
    end
    OpenRAG->>OpenRAG: Validate JWT signature<br/>& claims (sub, iss, aud)
    OpenRAG->>OpenRAG: Extract sub, email,<br/>groups from token
    OpenRAG->>OpenRAG: Parse groups → partition:role map<br/>(e.g., /rag-query/finance)
    OpenRAG->>PartitionDB: Sync user memberships<br/>(additive/authoritative)
    PartitionDB->>PartitionDB: Create/update/remove<br/>PartitionMembership rows
    OpenRAG->>Client: Process request<br/>with auth context

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• feat(auth): add OpenID Connect authentication mode #308: Implements core OIDC authentication features with JWT validation, group claim handling, and membership synchronization—directly overlapping with this PR's auth infrastructure.

Suggested reviewers

• Ahmath-Gadji

Poem

🐰 A cryptic leap through OIDC's door,
Groups sync to partitions, we asked for more,
Notifications whisper in Tchap's soft care,
Drive files dance where the RAG threads share,
Evaluation judges with wisdom so clear! ✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The PR title describes a fix to the pypdfium2 PdfDocument context manager issue, but the raw_summary shows extensive changes across multiple files (OIDC auth, Drive connector, notifications, eval, admin router, migrations, docs, etc.) that are unrelated to the title's claim.	The title should reflect the primary scope of changes. If the PR is truly about the pypdfium2 fix only, remove unrelated changes. Otherwise, update the title to summarize the actual major changes (e.g., 'feat: Add OIDC auth, Drive connector, notifications, eval, and admin endpoints').
Docstring Coverage	⚠️ Warning	Docstring coverage is 24.35% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Warning

⚠️ This pull request might be slop. It has been flagged by CodeRabbit slop detection and should be reviewed carefully.

[coderabbitai]

Actionable comments posted: 17

♻️ Duplicate comments (1)

openrag/components/eval.py (1)

42-42: ⚠️ Potential issue | 🔴 Critical

Same Pydantic subscripting issue as qa_override.py.

config.llm["base_url"] / ["api_key"] / ["model"] will raise TypeError if LLMConfig is a Pydantic BaseModel (it almost certainly is, mirroring EmbedderConfig). See the verification request on qa_override.py — the same fix (attribute access) applies here and in line 72/86.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@openrag/components/eval.py` at line 42, The code is subscripting a Pydantic
model (config.llm["base_url"], ["api_key"], ["model"]) which raises TypeError;
update uses of config.llm[...] to attribute access (e.g., config.llm.base_url,
config.llm.api_key, config.llm.model) when constructing AsyncOpenAI (llm_client)
and at the other occurrences in this file (the spots that currently pass
config.llm["api_key"] / ["model"]); ensure all references to config.llm[...] are
replaced with attribute access so AsyncOpenAI and related calls receive values
correctly.

🧹 Nitpick comments (13)

.gitignore (1)

92-93: Optional cleanup: remove duplicate .DS_Store rule.

Line 92 duplicates an existing .DS_Store ignore at Line 48. This is harmless, but removing one copy keeps the file cleaner.

Suggested cleanup

 # macOS
-.DS_Store
 **/.DS_Store
 .AppleDouble

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.gitignore around lines 92 - 93, Remove the duplicate .DS_Store ignore entry
by deleting one of the two identical patterns (either ".DS_Store" or
"**/.DS_Store") so only a single rule remains in .gitignore; ensure the
remaining rule still covers macOS Finder files (keep the more general
"**/.DS_Store" if you prefer recursive matching).

openrag/auth/test_oidc.py (1)

105-109: Prefer a normal import over __import__(...).

__import__("jose.exceptions", fromlist=["ExpiredSignatureError"]).ExpiredSignatureError is hard to read and bypasses linters. Just add from jose.exceptions import ExpiredSignatureError at the top of the file and use it directly.

🛠️ Proposed fix

 import time
 from unittest.mock import AsyncMock, MagicMock, patch

 import pytest
+from jose.exceptions import ExpiredSignatureError
@@
-            with patch("jose.jwt.decode", side_effect=__import__("jose.exceptions", fromlist=["ExpiredSignatureError"]).ExpiredSignatureError("Token expired")):
+            with patch("jose.jwt.decode", side_effect=ExpiredSignatureError("Token expired")):

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@openrag/auth/test_oidc.py` around lines 105 - 109, Replace the dynamic
__import__ call with a direct import of the exception: add "from jose.exceptions
import ExpiredSignatureError" at the top of the test file and change the patch
in the test that sets jose.jwt.decode's side_effect to
ExpiredSignatureError("Token expired"); keep the other patches and the
validate_jwt call as-is to ensure the test still raises OIDCValidationError with
"Token has expired".

openrag/components/notifications/webhook.py (1)

28-39: Consider escaping untrusted fields when building markup.

title, body, and url are interpolated directly into HTML (<h2>{title}</h2>…<a href="{url}">) and Markdown templates. If any of these ever flow from user-generated content (e.g., an announcement edited by a non-admin, or a URL provided by a connector), you can get malformed markup or HTML injection when the webhook target renders it. Consider html.escape(...) for the HTML branch and escaping ]/) (or at least validating the URL) for the Markdown branch.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@openrag/components/notifications/webhook.py` around lines 28 - 39, Escape
untrusted fields before interpolating into markup: when building the HTML
payload in the webhook code path (the branch that constructs html_body and
payload = {"html": html_body}), apply html.escape to title and body and ensure
the URL is validated or percent-encoded before inserting into the href;
similarly, in the Markdown branch (where md and payload = {"text": md} are
created) escape or sanitize characters used by Markdown (e.g., ] and )) and
validate the url before adding the [Open](url) link; update the code that
constructs payload in webhook.py to perform these escapes/validations so
title/body/url are never inserted raw into HTML or Markdown.

openrag/components/notifications/tchap.py (1)

31-46: HTML-escape title, body, and url when building formatted_body.

Matrix clients generally sanitize incoming HTML, but relying on that is fragile — especially since body may contain Markdown-source characters (<, &, ") coming from an announcement. Run the interpolated fields through html.escape(...) and urllib.parse.quote (for href) to produce well-formed HTML.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@openrag/components/notifications/tchap.py` around lines 31 - 46, Escape
interpolated HTML and href parts before building formatted_body: call
html.escape on title and body (and also on the text added to plain_text if
desired) and use urllib.parse.quote (or urllib.parse.quote_plus) on url when
embedding it into the <a href="..."> attribute so that formatted_body contains
well-formed, safe HTML; update the code that builds html_body/ formatted_body
(the variables title, body, url and the final formatted_body string) in tchap.py
to use these escaped values.

openrag/components/notifications/__init__.py (1)

9-16: Keep dispatcher registry and ChannelCreate type regex in sync.

ChannelCreate in openrag/routers/admin.py validates type against ^(webhook|email_smtp|tchap_bot)$. If a new channel type is added to that regex without updating this dict (or vice versa), persisted rows will raise ValueError at dispatch time. Consider deriving both from a single constant/enum.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@openrag/components/notifications/__init__.py` around lines 9 - 16, The
dispatchers dict and the ChannelCreate type regex are duplicated sources of
truth; replace the hardcoded keys with values derived from a single
enum/constant used by ChannelCreate to keep them in sync: create or reuse an
enum (e.g., ChannelType) referenced by ChannelCreate in
openrag/routers/admin.py, then build the dispatchers mapping in
openrag/components/notifications/__init__.py using ChannelType members (mapping
ChannelType.WEBHOOK -> WebhookDispatcher, ChannelType.EMAIL_SMTP ->
EmailDispatcher, ChannelType.TCHAP_BOT -> TchapDispatcher) and use that map to
resolve dispatcher_cls, so adding a new channel only requires updating the enum
used by ChannelCreate.

openrag/components/connectors/drive.py (2)

17-26: Unused imports / constants.

load_config (line 17) and the resulting config global (line 21) are never referenced. DRIVE_DEFAULT_BASE_URL (line 23) is also unused — every call site reads source.drive_base_url directly. Safe to drop.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@openrag/components/connectors/drive.py` around lines 17 - 26, The module
imports and globals include unused symbols: remove the unused import load_config
and the unused global config, and delete the unused constant
DRIVE_DEFAULT_BASE_URL; specifically remove the load_config import and the
assignment config = load_config(), and drop DRIVE_DEFAULT_BASE_URL since call
sites use source.drive_base_url directly, leaving only get_logger and used
environment constants (DRIVE_SERVICE_ACCOUNT_CLIENT_ID,
DRIVE_SERVICE_ACCOUNT_CLIENT_SECRET, OIDC_ISSUER_URL) intact.

---

117-141: Token endpoint is hardcoded to Keycloak's path; use OIDC discovery.

f"{issuer.rstrip('/')}/protocol/openid-connect/token" only works for Keycloak. Other OIDC providers (Dex, Authentik, Entra ID, Auth0…) expose a different token endpoint. Since the rest of this PR introduces OIDC support, resolve the token_endpoint from {issuer}/.well-known/openid-configuration (cache it) so the same code works for non-Keycloak IdPs.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@openrag/components/connectors/drive.py` around lines 117 - 141, The
get_access_token function currently constructs token_url as
f"{issuer.rstrip('/')}/protocol/openid-connect/token" which only works for
Keycloak; change it to resolve the token_endpoint via OIDC discovery by fetching
"{issuer.rstrip('/')}/.well-known/openid-configuration" (use httpx.AsyncClient
and resp.json()["token_endpoint"]) and then POST to that token_endpoint;
implement a simple cache keyed by issuer (in-memory dict or lru cache) to avoid
fetching discovery on every call, keep using client_id/client_secret and
resp.raise_for_status()/resp.json()["access_token"] semantics, and update
references to token_url to use the discovered token_endpoint and to handle
missing token_endpoint with a clear ValueError (referencing get_access_token,
token_url, OIDC_ISSUER_URL).

openrag/routers/admin.py (2)

229-265: Partition-scoped endpoints: consider require_partition_owner over blanket require_admin.

GET/PUT /partitions/{partition_name}/indexing are partition-scoped operations. As per coding guidelines: "User authentication uses token-based RBAC with partition memberships; check partition access via require_partition_viewer, require_partition_editor, or require_partition_owner from openrag/routers/utils.py". Gating only on require_admin prevents partition owners from managing their own partition's indexing, which contradicts the per-partition RBAC model documented elsewhere (admins should still pass via the is_admin bypass inside those dependencies).

-@router.get("/partitions/{partition_name}/indexing")
-async def get_partition_indexing(partition_name: str, user=Depends(require_admin)):
+@router.get("/partitions/{partition_name}/indexing")
+async def get_partition_indexing(partition_name: str, user=Depends(require_partition_owner)):

(Same for the PUT.)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@openrag/routers/admin.py` around lines 229 - 265, The endpoints
get_partition_indexing and set_partition_indexing currently use
user=Depends(require_admin) which blocks partition owners; change both
dependencies to user=Depends(require_partition_owner) (or the appropriate
partition-scoped dependency per the operation) so partition owners can manage
their partition while admins still pass via the admin bypass inside that
dependency; also update imports to bring require_partition_owner from
openrag.routers.utils and keep the rest of the handler logic unchanged.

---

207-207: Use timezone-aware UTC timestamps.

Every datetime.now() call in this file produces a naive local-time value. Stored alongside created_at/updated_at/sent_at/closed_at/responded_at, this breaks comparisons across timezones and migrations, and silently shifts data when the host TZ changes. Use datetime.now(timezone.utc) (or define a small helper) consistently.

-from datetime import datetime
+from datetime import datetime, timezone
@@
-        profile.updated_at = datetime.now()
+        profile.updated_at = datetime.now(timezone.utc)

Also applies to: 595-595, 628-628, 875-875, 905-905, 918-918, 149-149, 982-982

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@openrag/routers/admin.py` at line 207, Replace all naive datetime.now()
usages with timezone-aware UTC timestamps—e.g., set profile.updated_at =
datetime.now(timezone.utc) or call a small helper like utc_now()—and ensure you
import timezone from datetime (or centralize in a helper function) so every
field mentioned (profile.updated_at and other timestamp fields such as
created_at/updated_at/sent_at/closed_at/responded_at) consistently stores
UTC-aware datetimes.

openrag/auth/test_group_sync.py (1)

114-185: Missing authoritative-mode downgrade coverage.

TestAdditiveSync asserts "never downgrade", which is correct for additive. However, in authoritative mode, the expected semantic per the guide ("Adds/updates/removes to match the JWT groups exactly") implies that going from owner → viewer in successive syncs should downgrade the role, not preserve it. This case is not covered here. Consider adding:

def test_downgrades_role_in_authoritative(self, pfm_with_partitions):
    pfm = pfm_with_partitions
    pfm.sync_oidc_memberships_authoritative(10, {"finance": "owner"})
    pfm.sync_oidc_memberships_authoritative(10, {"finance": "viewer"})
    with pfm.Session() as s:
        m = s.query(PartitionMembership).filter_by(user_id=10, partition_name="finance").first()
        assert m.role == "viewer"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@openrag/auth/test_group_sync.py` around lines 114 - 185, Add an
authoritative-mode downgrade test to TestAuthoritativeSync: create a new test
method (e.g., test_downgrades_role_in_authoritative) that uses
pfm.sync_oidc_memberships_authoritative to set partition "finance" to "owner"
then runs it again to set "finance" to "viewer", and then opens pfm.Session()
and queries PartitionMembership (filter_by user_id and partition_name) to assert
the stored role is "viewer"; this validates that
sync_oidc_memberships_authoritative performs downgrades rather than preserving
higher roles or being additive.

prompts_integration.md (1)

1-374: Internal planning notes — consider keeping out of the product repo.

This file is a multi-prompt planning document in French mixing implementation status, ports, Scaleway credentials placeholders, and Keycloak sync scripting. It isn't referenced from the published docs tree and isn't wired into the build. Two small points:

• It contains plausibly-real deployment values (Scaleway API hosts, internal port allocations, admin scripts). Any leakage of this into images or public docs will inform attackers about the environment layout.
• markdownlint flagged MD040 (missing language on fenced blocks at lines 69 and 248) — trivial to fix but the larger question is whether this belongs under docs/ or in a private ops wiki.

Recommend moving under an ops/ or private location (or .gitignore) rather than shipping alongside product code.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@prompts_integration.md` around lines 1 - 374, The file prompts_integration.md
contains sensitive deployment placeholders (EMBEDDER_API_KEY, API_KEY,
VLM_API_KEY) and markdown lint issues (MD040: missing language on fenced blocks
around the Scaleway env snippet and the script snippet); fix by: 1) replacing
real-looking secrets with clearly-named placeholders or referencing a
.env.example and removing any actual secret strings (search for
EMBEDDER_API_KEY, API_KEY, VLM_API_KEY in the document), 2) add explicit
language tags to the fenced code blocks (e.g. ```bash or ```env) where MD040 was
flagged, and 3) move the document out of the product tree into a private ops
location (e.g. ops/ or a non-shipped directory) or add it to .gitignore so it is
not shipped with product code.

openrag/components/qa_override.py (1)

73-79: Recompute norm_q once outside the loop.

norm_q is invariant across iterations and can be hoisted; also prefer numpy for vector math if available.

♻️ Suggested tightening

-        best_match = None
-        best_similarity = 0.0
-
-        for i, override in enumerate(overrides):
-            override_embedding = response.data[i + 1].embedding
-            # Cosine similarity
-            dot_product = sum(a * b for a, b in zip(question_embedding, override_embedding))
-            norm_q = sum(a * a for a in question_embedding) ** 0.5
-            norm_o = sum(a * a for a in override_embedding) ** 0.5
-            similarity = dot_product / (norm_q * norm_o) if (norm_q * norm_o) > 0 else 0
-
-            if similarity > best_similarity:
-                best_similarity = similarity
-                best_match = override
+        best_match = None
+        best_similarity = 0.0
+        norm_q = sum(a * a for a in question_embedding) ** 0.5
+        for i, override in enumerate(overrides):
+            override_embedding = response.data[i + 1].embedding
+            dot_product = sum(a * b for a, b in zip(question_embedding, override_embedding))
+            norm_o = sum(a * a for a in override_embedding) ** 0.5
+            denom = norm_q * norm_o
+            similarity = dot_product / denom if denom > 0 else 0
+            if similarity > best_similarity:
+                best_similarity = similarity
+                best_match = override

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@openrag/components/qa_override.py` around lines 73 - 79, Move the invariant
norm_q calculation outside the loop: compute the norm of question_embedding once
(using either sum(a*a)**0.5 or numpy.linalg.norm(question_embedding) if numpy is
available) before the for i, override in enumerate(overrides) loop, then inside
the loop keep computing override_embedding, dot_product and norm_o and use the
precomputed norm_q when calculating similarity; update references to
question_embedding, override_embedding, similarity and response accordingly.

openrag/auth/oidc.py (1)

220-228: Use a non-cryptographic hash (or flag MD5) for the sync cache key.

hashlib.md5(...) here is just a stable key, not a security primitive — but it trips FIPS-restricted environments and security scanners (Bandit B303/B324). Either switch to blake2b(digest_size=16) / sha256 or pass usedforsecurity=False (Python ≥3.9).

-    return hashlib.md5(f"{user_id}:{groups_str}".encode()).hexdigest()
+    return hashlib.blake2b(f"{user_id}:{groups_str}".encode(), digest_size=16).hexdigest()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@openrag/auth/oidc.py` around lines 220 - 228, The cache key generator
_sync_cache_key currently uses hashlib.md5 which triggers FIPS/security
scanners; update it to a non-cryptographic or FIPS-friendly hash (e.g.,
hashlib.blake2b(digest_size=16)) to produce the same fixed-size hex key, e.g.
replace the md5 call with hashlib.blake2b(f"{user_id}:{groups_str}".encode(),
digest_size=16).hexdigest(); alternatively, if you must keep md5 on Python >=3.9
you can call hashlib.md5(..., usedforsecurity=False). Ensure the rest of the
code using _sync_cache and _SYNC_CACHE_TTL is unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/content/docs/guides/openwebui-keycloak.mdx`:
- Around line 75-82: The docs currently instruct users to set
ENABLE_FORWARD_OAUTH_TOKEN=true to forward Keycloak JWTs, but that env var was
never merged; remove the reference to ENABLE_FORWARD_OAUTH_TOKEN from the guide
and replace it with an accurate recommendation: either mention the supported
ENABLE_FORWARD_USER_INFO_HEADERS feature (which only forwards user info headers,
not JWTs) or describe a validated alternative (e.g., configuring oauth2-proxy or
a custom request pipe/middleware to forward the JWT to OpenRAG), and update any
example env block and explanatory sentence to reflect the supported approach
(referencing ENABLE_FORWARD_OAUTH_TOKEN, ENABLE_FORWARD_USER_INFO_HEADERS,
OpenRAG, Keycloak, and “forward JWT” to locate and update the affected lines).

In `@openrag/auth/oidc.py`:
- Around line 178-188: The groups claim can contain non-string entries causing
parse_partition_roles to call string methods and crash; update the handling in
openrag/auth/oidc.py where groups_raw is extracted (the variable groups_raw and
the OIDCIdentity construction) to normalize it to a list[str] by converting or
filtering entries—e.g., if groups_raw is a str wrap it in a list, otherwise
iterate the list and keep only items that are instances of str (or safely cast
entries to str as appropriate) before passing into OIDCIdentity so
parse_partition_roles receives only strings.

In `@openrag/components/connectors/drive.py`:
- Around line 188-189: The direct Ray actor invocations
indexer.add_file.remote(...) and vectordb.delete_file.remote(...) can hang the
scheduler; replace those direct .remote calls with the centralized
call_ray_actor_with_timeout(...) utility from components.ray_utils so each actor
call gets timeout/cancellation handling. Locate where indexer and vectordb
actors are obtained (ray.get_actor("Indexer", namespace="openrag") and
ray.get_actor("Vectordb", namespace="openrag")) and wrap the calls to the actor
methods (indexer.add_file and vectordb.delete_file) by invoking
call_ray_actor_with_timeout(actor, "method_name", *args, **kwargs) (or the
project’s call signature) so the loop over Drive files cannot be stalled by a
hung Ray call.
- Around line 332-385: The scheduler's run loop (DriveSyncScheduler.run) is
never started so periodic syncs never occur; instantiate and start the actor
during application startup by creating the actor with
DriveSyncScheduler.options(name="drive_sync_scheduler").remote() and immediately
call its run via scheduler.run.remote() (or call trigger_sync remotely for
single-run requests), and replace the TODO in the admin endpoint
/drive-sources/{source_id}/sync to either invoke
scheduler.trigger_sync.remote(source_id) or start the scheduler if not already
running; ensure the startup/init code (app startup hook) obtains the actor via
options(...).remote() so the periodic loop executes.
- Around line 183-186: The code compares an offset-aware datetime
(item.updated_at parsed with fromisoformat) to a DB-loaded offset-naive datetime
(mapping.drive_item_updated_at), causing TypeError; fix by normalizing both
sides to the same tz-ness before comparing: parse item.updated_at as you do,
then call .astimezone(timezone.utc).replace(tzinfo=None) on that parsed datetime
and likewise convert mapping.drive_item_updated_at (if not None) to UTC-naive
before the comparison that leads to updated_ids.add(item_id); also apply the
same normalization when writing/storing drive timestamps (the places that assign
mapping.drive_item_updated_at / insert the drive item timestamp) so stored
values remain UTC-naive and add "from datetime import timezone" to imports.
- Around line 153-319: The sync logic leaks DriveClient and temp files and risks
data loss on updates: ensure drive_client.close() is always called by moving the
DriveClient lifecycle into a try/finally (or closing it in the outer finally)
around the block where drive_client is used; for both new-file and updated-file
paths (where tempfile.NamedTemporaryFile(delete=False, ...) is created) wrap the
download/index steps in try/finally and unlink(tmp_path) in the finally on
failure so temp files are not left behind; and avoid delete-then-readd for
updates—remove the explicit vectordb.delete_file call in the updated-files loop
and call indexer.add_file.remote(..., replace=True) (using the existing
mapping.file_id in metadata) so replacement is atomic and prevents index data
loss.
- Around line 162-168: The ORM calls inside the actor are blocking: wrap the
database work that uses session_factory(), the query for DriveFileMapping (the
s.query(...).filter_by(...).all() that produces existing_mappings and the
construction of existing_by_drive_id) and any subsequent s.commit() calls in
asyncio.to_thread() so they run off the event loop; e.g., move the with
session_factory() as s: ... block into a synchronous helper that returns
existing_mappings/existing_by_drive_id (and performs commits) and call that
helper via await asyncio.to_thread(helper) where session_factory,
DriveFileMapping, existing_mappings, existing_by_drive_id and any commit logic
are referenced.

In `@openrag/components/eval.py`:
- Around line 84-106: The judge parsing is brittle: in the try block where
llm_client.chat.completions.create is called and judge_response / judge_text are
used, add response_format={"type":"json_object"} to the chat.completions.create
call (when supported) and move the import json to module scope; additionally
implement a robust fallback before calling json.loads(judge_text) that strips
surrounding code fences and prose then extracts the first {...} JSON object
block (or raises a clear parse error) so json.loads gets valid JSON, and keep
existing logger.warning and judge_score/judge_reason handling for exceptions
(refer to llm_client.chat.completions.create, judge_response, judge_text,
json.loads, logger.warning).
- Around line 74-78: The call to pipeline._prepare_for_chat_completion is being
unpacked into two variables but actually returns three; update the unpacking to
capture all three values (prepared_payload, docs, web_results) from
_prepare_for_chat_completion, then pass prepared_payload into
pipeline.llm_client.chat_completion as before and keep using response.choices to
set actual_answer; ensure references to prepared_payload, docs and web_results
are used/kept consistent so the outer exception no longer swallows a ValueError
from incorrect unpacking.

In `@openrag/components/indexer/loaders/pdf_loaders/marker.py`:
- Around line 194-197: The PdfDocument is being instantiated and closed
unconditionally but if len(pdf) raises an exception the native resource won't be
released; wrap the pairing of pypdfium2.PdfDocument(file_path) and the page
count retrieval (the len(pdf) call) in a try/finally so that pdf.close() is
always executed (create pdf, try: page_count = len(pdf); finally: pdf.close();
return the page_count after successful read).

In `@openrag/components/notifications/email.py`:
- Around line 39-56: The HTML body is built by interpolating unescaped title,
body and url and the SMTP calls (smtplib.SMTP / sendmail) run blocking inside an
async function; fix by escaping text and moving blocking I/O to a thread with a
timeout. Specifically: replace direct interpolation into html_body with
html.escape(title) and html.escape(body) and validate/sanitize the url before
adding it (ensure scheme is http/https, reject or percent-encode unsafe chars)
when constructing the html anchor; and run the entire SMTP session (the block
that creates MIMEMultipart, smtplib.SMTP(...), server.starttls(),
server.login(...), server.sendmail(...)) inside asyncio.to_thread (or use
aiosmtplib) and set a smtplib timeout via smtplib.SMTP(host, port, timeout=...)
so the event loop is not blocked and connections time out. Ensure you reference
and update uses of html_body, MIMEMultipart, smtplib.SMTP, starttls, login, and
sendmail in the function that sends the email.

In `@openrag/routers/admin.py`:
- Line 39: The FEEDBACK_SERVICE_KEY default of "" and the _require_service_key
behavior leaves /feedback/ingest effectively open and allows user-impersonation
via the body external_user_id; change authorization so that when
FEEDBACK_SERVICE_KEY is unset the service fails closed (reject requests) or
accepts requests only if the caller is an admin/partition editor, and update
_require_service_key to perform a constant-time comparison using
hmac.compare_digest against the header value; additionally, in the ingest
handler (the function that reads external_user_id and stores it) do not trust
the body: validate that the claimed external_user_id matches request.state.user
(or drop the field and derive the actor from request.state.user) before
persisting or promoting feedback to QAEntry and when keying poll votes to
prevent spoofing.
- Around line 400-426: start_eval_run creates a QAEvalRun record but never
dispatches the worker; after s.refresh(run) call the evaluation dispatcher
(e.g., run_evaluation.remote(run.id) or the appropriate Ray task/actor
invocation) to actually start the job, update run.status to "running" (and
commit) if dispatch succeeds, and on dispatch failure set run.status="failed"
with error details; use the QAEvalRun and run_evaluation symbols to locate where
to add the dispatch and error handling so the run does not remain perpetually
"pending".
- Around line 324-394: The static export route is shadowed by the parametric
route; move the `@router.get`("/qa/export") endpoint (export_qa_entries) so it is
declared before `@router.get`("/qa/{qa_id}") (get_qa_entry), or alternatively
constrain the parametric path to avoid matching static names; update the source
so router.get("/qa/export") appears above router.get("/qa/{qa_id}") and ensure
the single export_qa_entries definition is kept (delete any duplicate) and apply
the same ordering rule for any future static /qa/<name> routes.

In
`@openrag/scripts/migrations/alembic/versions/e1f2a3b4c5d6_add_oidc_support.py`:
- Around line 18-27: The PartitionMembership ORM model is missing the new
database column added by the migration; update the PartitionMembership class in
openrag/components/indexer/vectordb/models.py (around the existing class
definition) to declare the field so SQLAlchemy maps it: add a model attribute
named source using Column and String with nullable=False and
server_default="manual" (ensure Column and String are imported if not already)
so the ORM will read/write the new partition_memberships.source column.

In
`@openrag/scripts/migrations/alembic/versions/f2a3b4c5d6e7_add_all_integration_tables.py`:
- Around line 161-184: Update the DateTime columns that store Drive timestamps
to be timezone-aware: change the Column definitions for
drive_sources.last_synced_at and the drive_file_mappings columns
drive_item_updated_at and last_synced_at to use sa.DateTime(timezone=True).
Locate these in the op.create_table calls that build "drive_sources" (look for
Column("last_synced_at", sa.DateTime, ...)) and "drive_file_mappings" (look for
Column("drive_item_updated_at", sa.DateTime, ...) and Column("last_synced_at",
sa.DateTime, ...)) and adjust their types to sa.DateTime(timezone=True) so
values produced by datetime.fromisoformat("…Z") remain tz-aware and avoid
tz-naive/tz-aware TypeError.
- Around line 24-36: The migration defines chunk_overlap_rate and
similarity_threshold as sa.String columns but they represent numeric settings;
update the columns in the migration (file
f2a3b4c5d6e7_add_all_integration_tables.py) to use a numeric type (e.g.,
sa.Numeric(precision=3, scale=2) or sa.Float) instead of sa.String for the
chunk_overlap_rate and similarity_threshold columns, and change their
server_default values to numeric defaults (use sa.text('0.2') and sa.text('0.6')
or literal numeric defaults compatible with your DB) so the DB enforces numeric
typing and avoids downstream float(...) conversions.

---

Duplicate comments:
In `@openrag/components/eval.py`:
- Line 42: The code is subscripting a Pydantic model (config.llm["base_url"],
["api_key"], ["model"]) which raises TypeError; update uses of config.llm[...]
to attribute access (e.g., config.llm.base_url, config.llm.api_key,
config.llm.model) when constructing AsyncOpenAI (llm_client) and at the other
occurrences in this file (the spots that currently pass config.llm["api_key"] /
["model"]); ensure all references to config.llm[...] are replaced with attribute
access so AsyncOpenAI and related calls receive values correctly.

---

Nitpick comments:
In @.gitignore:
- Around line 92-93: Remove the duplicate .DS_Store ignore entry by deleting one
of the two identical patterns (either ".DS_Store" or "**/.DS_Store") so only a
single rule remains in .gitignore; ensure the remaining rule still covers macOS
Finder files (keep the more general "**/.DS_Store" if you prefer recursive
matching).

In `@openrag/auth/oidc.py`:
- Around line 220-228: The cache key generator _sync_cache_key currently uses
hashlib.md5 which triggers FIPS/security scanners; update it to a
non-cryptographic or FIPS-friendly hash (e.g., hashlib.blake2b(digest_size=16))
to produce the same fixed-size hex key, e.g. replace the md5 call with
hashlib.blake2b(f"{user_id}:{groups_str}".encode(), digest_size=16).hexdigest();
alternatively, if you must keep md5 on Python >=3.9 you can call
hashlib.md5(..., usedforsecurity=False). Ensure the rest of the code using
_sync_cache and _SYNC_CACHE_TTL is unchanged.

In `@openrag/auth/test_group_sync.py`:
- Around line 114-185: Add an authoritative-mode downgrade test to
TestAuthoritativeSync: create a new test method (e.g.,
test_downgrades_role_in_authoritative) that uses
pfm.sync_oidc_memberships_authoritative to set partition "finance" to "owner"
then runs it again to set "finance" to "viewer", and then opens pfm.Session()
and queries PartitionMembership (filter_by user_id and partition_name) to assert
the stored role is "viewer"; this validates that
sync_oidc_memberships_authoritative performs downgrades rather than preserving
higher roles or being additive.

In `@openrag/auth/test_oidc.py`:
- Around line 105-109: Replace the dynamic __import__ call with a direct import
of the exception: add "from jose.exceptions import ExpiredSignatureError" at the
top of the test file and change the patch in the test that sets
jose.jwt.decode's side_effect to ExpiredSignatureError("Token expired"); keep
the other patches and the validate_jwt call as-is to ensure the test still
raises OIDCValidationError with "Token has expired".

In `@openrag/components/connectors/drive.py`:
- Around line 17-26: The module imports and globals include unused symbols:
remove the unused import load_config and the unused global config, and delete
the unused constant DRIVE_DEFAULT_BASE_URL; specifically remove the load_config
import and the assignment config = load_config(), and drop
DRIVE_DEFAULT_BASE_URL since call sites use source.drive_base_url directly,
leaving only get_logger and used environment constants
(DRIVE_SERVICE_ACCOUNT_CLIENT_ID, DRIVE_SERVICE_ACCOUNT_CLIENT_SECRET,
OIDC_ISSUER_URL) intact.
- Around line 117-141: The get_access_token function currently constructs
token_url as f"{issuer.rstrip('/')}/protocol/openid-connect/token" which only
works for Keycloak; change it to resolve the token_endpoint via OIDC discovery
by fetching "{issuer.rstrip('/')}/.well-known/openid-configuration" (use
httpx.AsyncClient and resp.json()["token_endpoint"]) and then POST to that
token_endpoint; implement a simple cache keyed by issuer (in-memory dict or lru
cache) to avoid fetching discovery on every call, keep using
client_id/client_secret and resp.raise_for_status()/resp.json()["access_token"]
semantics, and update references to token_url to use the discovered
token_endpoint and to handle missing token_endpoint with a clear ValueError
(referencing get_access_token, token_url, OIDC_ISSUER_URL).

In `@openrag/components/notifications/__init__.py`:
- Around line 9-16: The dispatchers dict and the ChannelCreate type regex are
duplicated sources of truth; replace the hardcoded keys with values derived from
a single enum/constant used by ChannelCreate to keep them in sync: create or
reuse an enum (e.g., ChannelType) referenced by ChannelCreate in
openrag/routers/admin.py, then build the dispatchers mapping in
openrag/components/notifications/__init__.py using ChannelType members (mapping
ChannelType.WEBHOOK -> WebhookDispatcher, ChannelType.EMAIL_SMTP ->
EmailDispatcher, ChannelType.TCHAP_BOT -> TchapDispatcher) and use that map to
resolve dispatcher_cls, so adding a new channel only requires updating the enum
used by ChannelCreate.

In `@openrag/components/notifications/tchap.py`:
- Around line 31-46: Escape interpolated HTML and href parts before building
formatted_body: call html.escape on title and body (and also on the text added
to plain_text if desired) and use urllib.parse.quote (or
urllib.parse.quote_plus) on url when embedding it into the <a href="...">
attribute so that formatted_body contains well-formed, safe HTML; update the
code that builds html_body/ formatted_body (the variables title, body, url and
the final formatted_body string) in tchap.py to use these escaped values.

In `@openrag/components/notifications/webhook.py`:
- Around line 28-39: Escape untrusted fields before interpolating into markup:
when building the HTML payload in the webhook code path (the branch that
constructs html_body and payload = {"html": html_body}), apply html.escape to
title and body and ensure the URL is validated or percent-encoded before
inserting into the href; similarly, in the Markdown branch (where md and payload
= {"text": md} are created) escape or sanitize characters used by Markdown
(e.g., ] and )) and validate the url before adding the [Open](url) link; update
the code that constructs payload in webhook.py to perform these
escapes/validations so title/body/url are never inserted raw into HTML or
Markdown.

In `@openrag/components/qa_override.py`:
- Around line 73-79: Move the invariant norm_q calculation outside the loop:
compute the norm of question_embedding once (using either sum(a*a)**0.5 or
numpy.linalg.norm(question_embedding) if numpy is available) before the for i,
override in enumerate(overrides) loop, then inside the loop keep computing
override_embedding, dot_product and norm_o and use the precomputed norm_q when
calculating similarity; update references to question_embedding,
override_embedding, similarity and response accordingly.

In `@openrag/routers/admin.py`:
- Around line 229-265: The endpoints get_partition_indexing and
set_partition_indexing currently use user=Depends(require_admin) which blocks
partition owners; change both dependencies to
user=Depends(require_partition_owner) (or the appropriate partition-scoped
dependency per the operation) so partition owners can manage their partition
while admins still pass via the admin bypass inside that dependency; also update
imports to bring require_partition_owner from openrag.routers.utils and keep the
rest of the handler logic unchanged.
- Line 207: Replace all naive datetime.now() usages with timezone-aware UTC
timestamps—e.g., set profile.updated_at = datetime.now(timezone.utc) or call a
small helper like utc_now()—and ensure you import timezone from datetime (or
centralize in a helper function) so every field mentioned (profile.updated_at
and other timestamp fields such as
created_at/updated_at/sent_at/closed_at/responded_at) consistently stores
UTC-aware datetimes.

In `@prompts_integration.md`:
- Around line 1-374: The file prompts_integration.md contains sensitive
deployment placeholders (EMBEDDER_API_KEY, API_KEY, VLM_API_KEY) and markdown
lint issues (MD040: missing language on fenced blocks around the Scaleway env
snippet and the script snippet); fix by: 1) replacing real-looking secrets with
clearly-named placeholders or referencing a .env.example and removing any actual
secret strings (search for EMBEDDER_API_KEY, API_KEY, VLM_API_KEY in the
document), 2) add explicit language tags to the fenced code blocks (e.g. ```bash
or ```env) where MD040 was flagged, and 3) move the document out of the product
tree into a private ops location (e.g. ops/ or a non-shipped directory) or add
it to .gitignore so it is not shipped with product code.

🪄 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: e6466e55-5c28-4fb0-b070-fd4a77b1d589

📥 Commits

Reviewing files that changed from the base of the PR and between d6fd8e7 and 336b5d0.

📒 Files selected for processing (21)

• .gitignore
• docs/content/docs/guides/openwebui-keycloak.mdx
• extern/indexer-ui
• openrag/auth/__init__.py
• openrag/auth/oidc.py
• openrag/auth/test_group_sync.py
• openrag/auth/test_oidc.py
• openrag/components/connectors/__init__.py
• openrag/components/connectors/drive.py
• openrag/components/eval.py
• openrag/components/indexer/loaders/pdf_loaders/marker.py
• openrag/components/notifications/__init__.py
• openrag/components/notifications/base.py
• openrag/components/notifications/email.py
• openrag/components/notifications/tchap.py
• openrag/components/notifications/webhook.py
• openrag/components/qa_override.py
• openrag/routers/admin.py
• openrag/scripts/migrations/alembic/versions/e1f2a3b4c5d6_add_oidc_support.py
• openrag/scripts/migrations/alembic/versions/f2a3b4c5d6e7_add_all_integration_tables.py
• prompts_integration.md

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🌐 Web query:

Open WebUI ENABLE_FORWARD_OAUTH_TOKEN forwards JWT as Authorization Bearer to OpenAI backend

💡 Result:

No, Open WebUI does not have an official environment variable named ENABLE_FORWARD_OAUTH_TOKEN that forwards a JWT as an Authorization Bearer token to an OpenAI backend. This variable was proposed in a GitHub pull request (#9673) and discussion (#8957) to pass the user's OAuth/JWT token instead of the API key to OpenAI-compatible services for user authentication and access control. However, the PR targeting the dev branch was closed without merging, and it is not listed in the official environment variable documentation. A related merged feature (PR #6589) adds ENABLE_FORWARD_USER_INFO_HEADERS to forward user info (name, email, id, role) as headers to OpenAI APIs, but not the full token in the Authorization header. Recent issues (e.g., #23074) discuss OAuth token forwarding to custom OpenAI-compatible backends via cookies or pipes, with server-side session management for automatic refresh. Users have implemented workarounds using pipes to read oauth_id_token cookies and set Authorization headers, or oauth2-proxy for token passing.

Citations:

• 1: feat: Add config to pass OAuth token to OpenAI service open-webui/open-webui#9673
• 2: I would like to add a `ENABLE_FORWARD_OAUTH_TOKEN` environment variable to pass the token to the LLM service open-webui/open-webui#8957
• 3: https://docs.openwebui.com/reference/env-configuration/
• 4: issue: v0.8.11 - OpenAI Responses API - oAuth Token sent to backend not refreshed open-webui/open-webui#23074
• 5: feat: Add option to forward user info as headers to OpenAI API open-webui/open-webui#6589
• 6: https://github.com/open-webui/open-webui/blob/main/backend/open_webui/utils/oauth.py

---

ENABLE_FORWARD_OAUTH_TOKEN does not exist in Open WebUI—remove or replace this guidance.

The guide recommends ENABLE_FORWARD_OAUTH_TOKEN=true to forward the user's Keycloak JWT to OpenRAG, but this environment variable is not an official Open WebUI feature. The PR proposing it (#9673) was closed without merging and is not present in the official environment variable documentation. The related merged feature (ENABLE_FORWARD_USER_INFO_HEADERS) forwards user info headers only, not the JWT itself. Users following this guide will find the setting has no effect. Either replace this with an officially supported approach (e.g., a custom pipe or oauth2-proxy) or remove the guidance entirely.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/content/docs/guides/openwebui-keycloak.mdx` around lines 75 - 82, The
docs currently instruct users to set ENABLE_FORWARD_OAUTH_TOKEN=true to forward
Keycloak JWTs, but that env var was never merged; remove the reference to
ENABLE_FORWARD_OAUTH_TOKEN from the guide and replace it with an accurate
recommendation: either mention the supported ENABLE_FORWARD_USER_INFO_HEADERS
feature (which only forwards user info headers, not JWTs) or describe a
validated alternative (e.g., configuring oauth2-proxy or a custom request
pipe/middleware to forward the JWT to OpenRAG), and update any example env block
and explanatory sentence to reflect the supported approach (referencing
ENABLE_FORWARD_OAUTH_TOKEN, ENABLE_FORWARD_USER_INFO_HEADERS, OpenRAG, Keycloak,
and “forward JWT” to locate and update the affected lines).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Non-string entries in the groups claim will crash parse_partition_roles.

claims.get(OIDC_GROUP_CLAIM, []) is trusted to be a list[str]. With some Keycloak mappers (e.g. when "Full group path" is off but additional attributes are added, or when a different claim is remapped), the value can contain dicts or None. parse_partition_roles then calls g.lstrip("/") / g.startswith(prefix) and raises AttributeError, which propagates out of the middleware as a 500.

-    groups_raw = claims.get(OIDC_GROUP_CLAIM, [])
-    if isinstance(groups_raw, str):
-        groups_raw = [groups_raw]
+    groups_raw = claims.get(OIDC_GROUP_CLAIM, [])
+    if isinstance(groups_raw, str):
+        groups_raw = [groups_raw]
+    elif not isinstance(groups_raw, list):
+        groups_raw = []
+    groups_raw = [g for g in groups_raw if isinstance(g, str)]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@openrag/auth/oidc.py` around lines 178 - 188, The groups claim can contain
non-string entries causing parse_partition_roles to call string methods and
crash; update the handling in openrag/auth/oidc.py where groups_raw is extracted
(the variable groups_raw and the OIDCIdentity construction) to normalize it to a
list[str] by converting or filtering entries—e.g., if groups_raw is a str wrap
it in a list, otherwise iterate the list and keep only items that are instances
of str (or safely cast entries to str as appropriate) before passing into
OIDCIdentity so parse_partition_roles receives only strings.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Resource leaks and non-atomic update flow.

Three related reliability problems in sync_source:

1. drive_client leak: created at line 155 but close() is only called on the success path (line 295). Any exception between 155 and 295 jumps to the outer except at 308 where the client is never closed → hanging connection pool.
2. Temp file leaks on failure: at lines 199–201 (new) and 262–264 (updated) you create NamedTemporaryFile(delete=False, …). Indexer.add_file only unlinks the input file after a successful run (see openrag/components/indexer/indexer.py:68-163 — cleanup is in finally but the path is shadowed by exceptions before the finally clause can read it in some flows; in any case, when add_file.remote itself raises here, nothing in this module unlinks tmp_path). Wrap in try/finally and unlink on failure.
3. Non-atomic update risks data loss: lines 252–293 delete the old vectors via vectordb.delete_file and only then re-download + re-index. If add_file fails, the file is gone from the index. Indexer.add_file already supports replace=True (see openrag/components/indexer/indexer.py:68-163) which performs an in-place replacement — use it instead of delete-then-add.

🛠️ Sketch of the fixes

         try:
             token = await self.get_access_token(source)
             drive_client = DriveClient(source.drive_base_url, token)
-
-            # List current files in Drive
-            drive_items = await drive_client.list_folder(source.drive_folder_id)
+            try:
+                drive_items = await drive_client.list_folder(source.drive_folder_id)
                 ...
-            await drive_client.close()
+            finally:
+                await drive_client.close()

-                    await indexer.add_file.remote(
-                        path=tmp_path,
-                        metadata=metadata,
-                        partition=source.partition_name,
-                    )
+                    try:
+                        await indexer.add_file.remote(
+                            path=tmp_path,
+                            metadata=metadata,
+                            partition=source.partition_name,
+                            replace=True,  # only for the "updated_ids" branch
+                        )
+                    finally:
+                        Path(tmp_path).unlink(missing_ok=True)

(For the updated branch, drop the separate vectordb.delete_file call and rely on replace=True.)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@openrag/components/connectors/drive.py` around lines 153 - 319, The sync
logic leaks DriveClient and temp files and risks data loss on updates: ensure
drive_client.close() is always called by moving the DriveClient lifecycle into a
try/finally (or closing it in the outer finally) around the block where
drive_client is used; for both new-file and updated-file paths (where
tempfile.NamedTemporaryFile(delete=False, ...) is created) wrap the
download/index steps in try/finally and unlink(tmp_path) in the finally on
failure so temp files are not left behind; and avoid delete-then-readd for
updates—remove the explicit vectordb.delete_file call in the updated-files loop
and call indexer.add_file.remote(..., replace=True) (using the existing
mapping.file_id in metadata) so replacement is atomic and prevents index data
loss.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check whether an async session is available in the codebase.
rg -nP --type=py -C2 'async(?:_)?session(?:maker)?|AsyncSession|create_async_engine'
rg -nP --type=py -C2 'def\s+Session\s*\('

Repository: linagora/openrag

Length of output: 42

---

🏁 Script executed:

# First, examine the drive.py file around the flagged lines to understand the context
head -n 200 openrag/components/connectors/drive.py | tail -n 50

Repository: linagora/openrag

Length of output: 2225

---

🏁 Script executed:

# Also look at the function containing lines 162-168 to see if it's async
sed -n '130,180p' openrag/components/connectors/drive.py

Repository: linagora/openrag

Length of output: 2021

---

🏁 Script executed:

# Check the file size and structure
wc -l openrag/components/connectors/drive.py

Repository: linagora/openrag

Length of output: 102

---

🏁 Script executed:

# Look for async functions and Ray actor definitions in the file
rg -n 'async def|@ray\.remote|def.*session_factory' openrag/components/connectors/drive.py

Repository: linagora/openrag

Length of output: 615

---

🏁 Script executed:

# Search more broadly for session creation and async patterns in the components directory
rg -n 'AsyncSession|create_async_engine|async_sessionmaker' openrag/components/

Repository: linagora/openrag

Length of output: 42

---

🏁 Script executed:

# Check the utils.logger and other imports to understand the codebase structure
fd -n 'session\|factory' openrag/ --type f

Repository: linagora/openrag

Length of output: 290

---

🏁 Script executed:

# Find session_factory definition
rg -n 'session_factory\s*=' openrag/

Repository: linagora/openrag

Length of output: 42

---

🏁 Script executed:

# Look at the class containing the async def sync_source and Ray actor setup
sed -n '100,350p' openrag/components/connectors/drive.py | head -n 150

Repository: linagora/openrag

Length of output: 6368

---

🏁 Script executed:

# Check if asyncio.to_thread is used anywhere in the codebase
rg -n 'asyncio\.to_thread|to_thread' openrag/

Repository: linagora/openrag

Length of output: 1799

---

🏁 Script executed:

# Look for DB/SQLAlchemy session patterns in other async contexts
rg -n 'async def.*session' openrag/ --type py

Repository: linagora/openrag

Length of output: 1386

---

🏁 Script executed:

# Check imports in drive.py to understand session_factory source
head -n 50 openrag/components/connectors/drive.py

Repository: linagora/openrag

Length of output: 1525

---

🏁 Script executed:

# Find where sync_source is called
rg -n 'sync_source' openrag/components/connectors/drive.py

Repository: linagora/openrag

Length of output: 204

---

🏁 Script executed:

# Look at the DriveSyncScheduler Ray actor
sed -n '320,385p' openrag/components/connectors/drive.py

Repository: linagora/openrag

Length of output: 2480

---

🏁 Script executed:

# Search for how session_factory is created/provided to this connector
rg -n 'session_factory' openrag/components/connectors/ -A 2 -B 2

Repository: linagora/openrag

Length of output: 2969

---

Wrap blocking SQLAlchemy calls with asyncio.to_thread() to avoid blocking the Ray actor's event loop.

This async function (part of a @ray.remote actor) contains multiple blocking ORM calls (session_factory() with s.query().all(), s.commit()) that will block the event loop during each database round-trip, starving other concurrent operations like the HTTP calls to Drive. Wrap these database operations with asyncio.to_thread() to run them in a thread pool instead. This pattern is already used elsewhere in the codebase (e.g., pdf_loaders/docling.py, loaders/audio/openai.py).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@openrag/components/connectors/drive.py` around lines 162 - 168, The ORM calls
inside the actor are blocking: wrap the database work that uses
session_factory(), the query for DriveFileMapping (the
s.query(...).filter_by(...).all() that produces existing_mappings and the
construction of existing_by_drive_id) and any subsequent s.commit() calls in
asyncio.to_thread() so they run off the event loop; e.g., move the with
session_factory() as s: ... block into a synchronous helper that returns
existing_mappings/existing_by_drive_id (and performs commits) and call that
helper via await asyncio.to_thread(helper) where session_factory,
DriveFileMapping, existing_mappings, existing_by_drive_id and any commit logic
are referenced.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Critical: offset-aware vs offset-naive datetime comparison will raise TypeError.

datetime.fromisoformat(item.updated_at.replace("Z", "+00:00")) returns an offset-aware datetime, but mapping.drive_item_updated_at is read back from a sa.DateTime column (no timezone=True — see f2a3b4c5d6e7_add_all_integration_tables.py line 179), which yields an offset-naive value on Postgres/SQLite. The item_dt > mapping.drive_item_updated_at comparison will raise TypeError: can't compare offset-naive and offset-aware datetimes, which is swallowed by the outer except at line 308 and marks every sync that has any common files as failed. Normalize both sides to the same tz-ness (e.g., drop tzinfo via .astimezone(timezone.utc).replace(tzinfo=None) before comparing, or migrate the columns to DateTime(timezone=True)).

🛠️ Proposed fix

-            # Check for updates (modified files)
-            updated_ids = set()
-            for item_id in common_ids:
-                item = drive_items_by_id[item_id]
-                mapping = existing_by_drive_id[item_id]
-                if item.updated_at and mapping.drive_item_updated_at:
-                    item_dt = datetime.fromisoformat(item.updated_at.replace("Z", "+00:00"))
-                    if item_dt > mapping.drive_item_updated_at:
-                        updated_ids.add(item_id)
+            # Check for updates (modified files)
+            updated_ids = set()
+            for item_id in common_ids:
+                item = drive_items_by_id[item_id]
+                mapping = existing_by_drive_id[item_id]
+                if item.updated_at and mapping.drive_item_updated_at:
+                    item_dt = datetime.fromisoformat(
+                        item.updated_at.replace("Z", "+00:00")
+                    ).astimezone(timezone.utc).replace(tzinfo=None)
+                    if item_dt > mapping.drive_item_updated_at:
+                        updated_ids.add(item_id)

(Also add from datetime import timezone and apply the same normalization at the insert sites, lines 223 and 284, so stored and compared values stay consistent.)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@openrag/components/connectors/drive.py` around lines 183 - 186, The code
compares an offset-aware datetime (item.updated_at parsed with fromisoformat) to
a DB-loaded offset-naive datetime (mapping.drive_item_updated_at), causing
TypeError; fix by normalizing both sides to the same tz-ness before comparing:
parse item.updated_at as you do, then call
.astimezone(timezone.utc).replace(tzinfo=None) on that parsed datetime and
likewise convert mapping.drive_item_updated_at (if not None) to UTC-naive before
the comparison that leads to updated_ids.add(item_id); also apply the same
normalization when writing/storing drive timestamps (the places that assign
mapping.drive_item_updated_at / insert the drive item timestamp) so stored
values remain UTC-naive and add "from datetime import timezone" to imports.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

GET /qa/export is unreachable — shadowed by GET /qa/{qa_id}.

FastAPI matches routes in declaration order. @router.get("/qa/{qa_id}") at line 324 is declared before @router.get("/qa/export") at line 386, and qa_id is typed int. A request to GET /qa/export matches the parametric route first, fails int validation on "export", and returns 422 Unprocessable Entity — the export handler is never invoked.

Fix by either moving the static route above the parametric one, or constraining the path parameter:

-@router.get("/qa/{qa_id}")
-async def get_qa_entry(qa_id: int, user=Depends(require_admin)):
+@router.get("/qa/export")
+async def export_qa_entries(partition: str | None = None, user=Depends(require_admin)):
+    Session = await get_session()
+    with Session() as s:
+        query = s.query(QAEntry)
+        if partition:
+            query = query.filter(QAEntry.partition_name == partition)
+        entries = query.all()
+        return [e.to_dict() for e in entries]
+
+
+@router.get("/qa/{qa_id}")
+async def get_qa_entry(qa_id: int, user=Depends(require_admin)):
     ...

(And delete the later export_qa_entries definition.) Same ordering discipline applies to any future static /qa/<name> routes.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@openrag/routers/admin.py` around lines 324 - 394, The static export route is
shadowed by the parametric route; move the `@router.get`("/qa/export") endpoint
(export_qa_entries) so it is declared before `@router.get`("/qa/{qa_id}")
(get_qa_entry), or alternatively constrain the parametric path to avoid matching
static names; update the source so router.get("/qa/export") appears above
router.get("/qa/{qa_id}") and ensure the single export_qa_entries definition is
kept (delete any duplicate) and apply the same ordering rule for any future
static /qa/<name> routes.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Multiple endpoints return success but perform no work (TODO stubs).

POST /qa/eval, POST /drive-sources/{id}/sync, POST /channels/{id}/test, and POST /announcements/{id}/send all respond with 200/201 and a success-shaped body without doing the corresponding work — the Ray actor call / dispatcher invocation is a TODO comment. Callers (admin UI, monitoring, cron) will treat these as completed:

• /qa/eval creates a QAEvalRun in status pending with total_questions set but never launches run_evaluation, so the run stays pending forever.
• /drive-sources/{id}/sync returns "Sync triggered" without triggering anything.
• /channels/{id}/test returns "Test message sent via ..." — literally false.
• /announcements/{id}/send flips status="sent" and sent_at=now() in the DB without dispatching to any channel; operators will believe the announcement went out.

Either return 501 Not Implemented until wired up, or wire the real dispatch (run_evaluation via Ray, DriveSyncScheduler.trigger_sync.remote, get_dispatcher(channel).send(...)) before merge. announcement.status = "sent" in particular should not be set until the dispatcher succeeds.

Want me to open a follow-up issue listing each stubbed endpoint and the target actor/dispatcher it needs to call?

Also applies to: 711-724, 784-797, 893-907

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@openrag/routers/admin.py` around lines 400 - 426, start_eval_run creates a
QAEvalRun record but never dispatches the worker; after s.refresh(run) call the
evaluation dispatcher (e.g., run_evaluation.remote(run.id) or the appropriate
Ray task/actor invocation) to actually start the job, update run.status to
"running" (and commit) if dispatch succeeds, and on dispatch failure set
run.status="failed" with error details; use the QAEvalRun and run_evaluation
symbols to locate where to add the dispatch and error handling so the run does
not remain perpetually "pending".

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -nP --type=py -C3 '\bclass\s+PartitionMembership\b'
rg -nP --type=py 'source\s*=\s*Column\('

Repository: linagora/openrag

Length of output: 609

---

🏁 Script executed:

sed -n '143,160p' openrag/components/indexer/vectordb/models.py

Repository: linagora/openrag

Length of output: 848

---

🏁 Script executed:

sed -n '143,170p' openrag/components/indexer/vectordb/models.py

Repository: linagora/openrag

Length of output: 1091

---

Add source column to the PartitionMembership ORM model.

The migration correctly adds the source column to the partition_memberships table with server_default="manual", but the corresponding field is missing from the PartitionMembership model in openrag/components/indexer/vectordb/models.py (lines 143–164). Without it, the ORM won't track this column, preventing queries from returning the value or Python code from setting it directly.

Add the following to the model:

source = Column(String, nullable=False, server_default="manual")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@openrag/scripts/migrations/alembic/versions/e1f2a3b4c5d6_add_oidc_support.py`
around lines 18 - 27, The PartitionMembership ORM model is missing the new
database column added by the migration; update the PartitionMembership class in
openrag/components/indexer/vectordb/models.py (around the existing class
definition) to declare the field so SQLAlchemy maps it: add a model attribute
named source using Column and String with nullable=False and
server_default="manual" (ensure Column and String are imported if not already)
so the ORM will read/write the new partition_memberships.source column.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

chunk_overlap_rate and similarity_threshold should be numeric, not String.

These are numeric settings (defaults "0.2" and "0.6"). Storing them as String means the DB can’t validate that they’re numbers, sorting/filtering uses string collation, and every read site has to float(...). Prefer sa.Numeric or sa.Float.

🛠️ Proposed fix

-        sa.Column("chunk_overlap_rate", sa.String, nullable=False, server_default="0.2"),
+        sa.Column("chunk_overlap_rate", sa.Numeric(3, 2), nullable=False, server_default="0.2"),
@@
-        sa.Column("similarity_threshold", sa.String, nullable=False, server_default="0.6"),
+        sa.Column("similarity_threshold", sa.Numeric(3, 2), nullable=False, server_default="0.6"),

📝 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.

Suggested change

	sa.Column("chunker_name", sa.String(50), nullable=False, server_default="recursive_splitter"),
	sa.Column("chunk_size", sa.Integer, nullable=False, server_default="512"),
	sa.Column("chunk_overlap_rate", sa.String, nullable=False, server_default="0.2"),
	sa.Column("contextual_retrieval", sa.Boolean, nullable=False, server_default=sa.text("true")),
	sa.Column("contextualization_timeout", sa.Integer, nullable=False, server_default="120"),
	sa.Column("max_concurrent_contextualization", sa.Integer, nullable=False, server_default="10"),
	sa.Column("retriever_type", sa.String(50), nullable=False, server_default="single"),
	sa.Column("retriever_top_k", sa.Integer, nullable=False, server_default="50"),
	sa.Column("similarity_threshold", sa.String, nullable=False, server_default="0.6"),
	sa.Column("extra_params", sa.JSON, nullable=False, server_default="{}"),
	sa.Column("created_at", sa.DateTime, nullable=False, server_default=sa.func.now()),
	sa.Column("updated_at", sa.DateTime, nullable=False, server_default=sa.func.now()),
	)
	sa.Column("chunker_name", sa.String(50), nullable=False, server_default="recursive_splitter"),
	sa.Column("chunk_size", sa.Integer, nullable=False, server_default="512"),
	sa.Column("chunk_overlap_rate", sa.Numeric(3, 2), nullable=False, server_default="0.2"),
	sa.Column("contextual_retrieval", sa.Boolean, nullable=False, server_default=sa.text("true")),
	sa.Column("contextualization_timeout", sa.Integer, nullable=False, server_default="120"),
	sa.Column("max_concurrent_contextualization", sa.Integer, nullable=False, server_default="10"),
	sa.Column("retriever_type", sa.String(50), nullable=False, server_default="single"),
	sa.Column("retriever_top_k", sa.Integer, nullable=False, server_default="50"),
	sa.Column("similarity_threshold", sa.Numeric(3, 2), nullable=False, server_default="0.6"),
	sa.Column("extra_params", sa.JSON, nullable=False, server_default="{}"),
	sa.Column("created_at", sa.DateTime, nullable=False, server_default=sa.func.now()),
	sa.Column("updated_at", sa.DateTime, nullable=False, server_default=sa.func.now()),
	)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@openrag/scripts/migrations/alembic/versions/f2a3b4c5d6e7_add_all_integration_tables.py`
around lines 24 - 36, The migration defines chunk_overlap_rate and
similarity_threshold as sa.String columns but they represent numeric settings;
update the columns in the migration (file
f2a3b4c5d6e7_add_all_integration_tables.py) to use a numeric type (e.g.,
sa.Numeric(precision=3, scale=2) or sa.Float) instead of sa.String for the
chunk_overlap_rate and similarity_threshold columns, and change their
server_default values to numeric defaults (use sa.text('0.2') and sa.text('0.6')
or literal numeric defaults compatible with your DB) so the DB enforces numeric
typing and avoids downstream float(...) conversions.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Use DateTime(timezone=True) for Drive timestamps.

drive_sources.last_synced_at and drive_file_mappings.drive_item_updated_at/last_synced_at are compared against values produced by datetime.fromisoformat("…Z") in drive.py (which are tz-aware). Declaring the columns as sa.DateTime(timezone=True) keeps the round-trip consistent and is what prevents the tz-naive/tz-aware TypeError flagged in drive.py from reappearing after the fix.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@openrag/scripts/migrations/alembic/versions/f2a3b4c5d6e7_add_all_integration_tables.py`
around lines 161 - 184, Update the DateTime columns that store Drive timestamps
to be timezone-aware: change the Column definitions for
drive_sources.last_synced_at and the drive_file_mappings columns
drive_item_updated_at and last_synced_at to use sa.DateTime(timezone=True).
Locate these in the op.create_table calls that build "drive_sources" (look for
Column("last_synced_at", sa.DateTime, ...)) and "drive_file_mappings" (look for
Column("drive_item_updated_at", sa.DateTime, ...) and Column("last_synced_at",
sa.DateTime, ...)) and adjust their types to sa.DateTime(timezone=True) so
values produced by datetime.fromisoformat("…Z") remain tz-aware and avoid
tz-naive/tz-aware TypeError.