Phase 4: ABCs + ports - #333
Merged
Merged
Conversation
Async-native embedder interface: embed(), embed_single(), dimension property. Registry for pluggable implementations via @embedder_registry.register().
Async reranker interface: rerank(query, documents, top_k) returning (original_index, score) pairs sorted by relevance.
Async LLM interface: generate(), chat(), stream_chat(), generate_json(), chat_with_tools(). Default implementations for optional methods (streaming falls back to non-streaming, tools raises NotImplementedError).
Async vision-language model interface: caption_image(), caption_images_batch().
Async vector database interface: upsert(), search(), delete(), ensure_collection(), drop_collection(), collection_exists(), query_ids_by_filter(), query_chunks_by_filter().
ChunkingStrategy ABC: chunk(ProcessedDocument) -> list[Chunk]. DocumentParser ABC: parse(Document) -> ProcessedDocument, supported_types(). Both with registries for pluggable implementations.
Composes all 13 repository ports with lifecycle (initialize/shutdown). Concrete implementations (e.g. PostgresStore) live in services/storage/.
📝 WalkthroughWalkthroughThe PR establishes foundational architecture by introducing abstract base classes and registries for core components: chunking strategies, embeddings, document parsing, LLMs, VLMs, rerankers, vector storage, and data persistence via repository interfaces. A Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (2)
openrag/core/ports/prompt_repo.py (1)
7-23: UsePromptTypein the prompt-type query contract.
PromptTypealready defines the valid values, so accepting rawstrhere weakens the port contract and lets invalid prompt categories flow to implementations.Suggested type tightening
-from openrag.core.models.prompt import Prompt +from openrag.core.models.prompt import Prompt, PromptType @@ - async def get_by_type(self, prompt_type: str) -> list[Prompt]: ... + async def get_by_type(self, prompt_type: PromptType) -> list[Prompt]: ... `@abstractmethod` - async def get_active(self, prompt_type: str) -> Prompt | None: ... + async def get_active(self, prompt_type: PromptType) -> Prompt | None: ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/core/ports/prompt_repo.py` around lines 7 - 23, The port methods get_by_type and get_active accept raw str for the prompt type; tighten the contract to use the existing PromptType enum by importing PromptType from openrag.core.models.prompt and changing the parameter types on PromptRepository.get_by_type(prompt_type: PromptType) -> list[Prompt] and PromptRepository.get_active(prompt_type: PromptType) -> Prompt | None; keep other signatures (e.g., create_prompt, get_prompt) unchanged so implementations receive a validated PromptType instead of arbitrary strings.openrag/core/ports/document_repo.py (1)
21-39: UseDocumentStatusfor status filters.
DocumentRecord.statusis typed asDocumentStatusinopenrag/core/models/catalog.py:13-44, but this port accepts arbitrary strings forstatus. Tightening the contract now will keep adapters aligned and catch invalid statuses earlier.♻️ Proposed type tightening
-from openrag.core.models.catalog import DocumentRecord +from openrag.core.models.catalog import DocumentRecord, DocumentStatus @@ - status: str | None = None, + status: DocumentStatus | None = None, @@ - async def count_documents(self, partition: str | list[str] | None = None, status: str | None = None) -> int: ... + async def count_documents( + self, + partition: str | list[str] | None = None, + status: DocumentStatus | None = None, + ) -> int: ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/core/ports/document_repo.py` around lines 21 - 39, The status parameters on the document repo port should be tightened to use the DocumentStatus enum rather than raw strings: update the type hints for list_documents and count_documents to accept DocumentStatus | None (and any other methods that filter by status), import DocumentStatus from openrag.core.models.catalog, and adjust any internal references/signatures (e.g., list_documents, count_documents) so adapters and implementations must pass/accept DocumentStatus values instead of arbitrary strings; ensure type imports and exported signatures are updated consistently across the interface and adapters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openrag/core/embeddings/registry.py`:
- Around line 3-4: Replace the incorrect imports in registry.py: change the
package-local import "from openrag.core.embeddings.embedder import Embedder" to
a relative import "from .embedder import Embedder", and change the cross-package
import "from openrag.core.utils.registry import Registry" to an absolute import
rooted at the package root without the leading prefix: "from core.utils.registry
import Registry"; update the lines that reference Embedder and Registry
accordingly.
In `@openrag/core/indexing/parsers/__init__.py`:
- Around line 3-4: The imports in __init__.py use the package-root prefix and
should be converted to relative imports: replace the top-level imports of
DocumentParser and parser_registry with package-relative imports so that
DocumentParser (from document_parser) and parser_registry (from registry) are
imported via relative paths (e.g., using dot-prefixed imports) to match the
package pattern and avoid runtime import errors.
In `@openrag/core/llm/__init__.py`:
- Around line 3-4: Replace the absolute imports with relative imports: change
the top-level imports that import LLM and llm_registry to use package-relative
form (i.e., import LLM from .llm and llm_registry from .registry) so that the
symbols LLM and llm_registry are imported via relative imports within the same
package.
In `@openrag/core/llm/llm.py`:
- Around line 25-38: generate_json currently parses JSON then returns whatever
json.loads yields which can be a list or scalar; update the generate_json method
to validate that the parsed result is a dict/mapping and raise LLMParsingError
if it's not: after json.loads(text) in generate_json, check isinstance(result,
dict) (or collections.abc.Mapping) and if false raise LLMParsingError with
raw_response=text and a parse_error message indicating "expected JSON object but
got <type>", otherwise return the dict; reference function generate_json and
exception LLMParsingError when implementing the change.
In `@openrag/core/ports/conversation_repo.py`:
- Around line 16-29: The port methods get_conversation, delete_conversation,
add_message, and list_messages must carry the same user/partition scope as
list_conversations so implementations can enforce tenant/partition predicates
atomically; update their signatures to accept user_id: int and partition: str |
None (or include a single scope token param) and adjust add_message to accept
both Message and scope (or validate message.user_id and message.partition_scope
against the passed scope), ensuring Conversation (with its user_id and
partition_scope) and list_conversations remain consistent; also document that
implementations should validate a token/role-based auth scope when enforcing
partition access.
In `@openrag/core/ports/entity_repo.py`:
- Around line 17-21: Update the repository contract so document-scoped methods
are partition-aware: change the abstract signatures of get_by_document and
delete_by_document to accept the same partition identifier used by upsert/search
(e.g., add a partition_id: str or tenant_id: str parameter) and, if your auth
flow requires it, an optional auth_token: str for RBAC checks; then update all
implementations to honor the partition_id and validate RBAC via the token before
performing the operation (refer to get_by_document, delete_by_document, upsert,
search to keep signatures consistent).
In `@openrag/core/ports/idempotency_repo.py`:
- Around line 11-15: Add an atomic reservation/claim API to the idempotency
repository interface so callers can atomically acquire uniqueness before
executing side effects: extend the abstract class by adding a method like async
def reserve(key_hash: str, http_method: str, ttl_seconds: int) -> bool that
attempts to create an exclusive claim and returns whether the claim was
acquired, and a method like async def complete_reservation(key_hash: str,
status_code: int, response_body: bytes) -> None to store the final response and
release the claim; keep get_by_hash() and store() for read/legacy use but ensure
implementations enforce uniqueness in reserve() (e.g., use DB
uniqueness/transaction or atomic upsert) and honor TTL to avoid permanent locks.
In `@openrag/core/ports/job_repo.py`:
- Around line 20-21: The list_jobs port currently accepts status: str and no
partition filter which loses the typed JobStatus and partition scoping from
IndexationJob; update the abstract method signature list_jobs to accept status:
JobStatus | None and add a partition: str | None (or PartitionId type if
available) parameter so callers must pass typed status and an explicit partition
scope, and ensure implementations of list_jobs (and any related classes) enforce
token-based authentication with role-based access control when filtering by
partition to prevent unscoped multi-tenant access.
In `@openrag/core/ports/partition_repo.py`:
- Around line 20-21: Clarify and enforce cascade delete semantics for
delete_partition by updating the abstract method docstring and implementations:
explicitly state whether delete_partition(name: str) must remove the partition
row plus all related data (call
DocumentRepository.delete_documents_by_partition,
ChunkRepository.delete_by_partition, vector store cleanup, and user-partition
assignment removals) or only the metadata; if cascade is chosen, ensure
implementations invoke those repository APIs and vector cleanup in a
transaction/ordered cleanup; additionally require that any public API or service
method that calls delete_partition validates tenant token and enforces RBAC
(token-based auth + role check) before performing the deletion.
In `@openrag/core/ports/topic_tag_repo.py`:
- Around line 14-18: The repository interface methods get_by_document and
delete_by_document lack a partition parameter which weakens multi-tenant safety;
update their signatures to include a partition: str argument (matching search's
partition usage) and propagate this change to all implementations so
reads/deletes are partition-scoped, and ensure these methods (get_by_document,
delete_by_document, and search) validate an incoming token and enforce RBAC
checks before performing operations (rejecting cross-partition access when the
token’s roles/claims do not authorize the requested partition).
In `@openrag/core/ports/user_repo.py`:
- Around line 8-50: The update_partition_role method currently accepts a plain
string for the role which allows invalid values; change the port to accept the
enum-backed PartitionRole instead: import PartitionRole from
openrag.core.models.user and update the abstract method signature in
UserRepository (update_partition_role) to take role: PartitionRole rather than
role: str, ensuring callers and implementations use the PartitionRole enum that
backs UserPartition.role to prevent invalid roles from being persisted.
In `@openrag/core/rerankers/__init__.py`:
- Around line 3-4: The current top-level imports in __init__.py use absolute
package paths and fail at runtime; change them to relative imports by importing
reranker_registry and Reranker from the local package using the dot-syntax
(i.e., replace references to openrag.core.rerankers.reranker_registry and
openrag.core.rerankers.reranker with relative imports that import
reranker_registry and Reranker from .registry and .reranker respectively) so the
symbols reranker_registry and Reranker are imported from the same package
correctly.
In `@openrag/core/vector_stores/vector_store.py`:
- Around line 20-27: The search method is currently dense-only; modify the
VectorStore.search signature to accept optional sparse/hybrid parameters (e.g.,
sparse_query: str | None = None and sparse_weight: float = 0.0) and update the
docstring to describe hybrid behavior and defaults so existing callers remain
compatible; ensure backend implementations (and any code invoking vector search)
can detect hybrid requests and, for Milvus-based implementations, use the
MilvusDB Ray actor via ray.get_actor("Vectordb", namespace="openrag") to perform
dense+BM25 hybrid searches when sparse_query is provided and sparse_weight >
0.0.
In `@openrag/core/vlm/__init__.py`:
- Around line 3-4: Replace the two absolute imports in the package initializer
with relative imports: change references that import vlm_registry and VLM via
"from openrag.core.vlm.registry import vlm_registry" and "from
openrag.core.vlm.vlm import VLM" to use relative module paths (import the same
symbols from .registry and .vlm respectively) so the package __init__.py exposes
vlm_registry and VLM using relative imports.
---
Nitpick comments:
In `@openrag/core/ports/document_repo.py`:
- Around line 21-39: The status parameters on the document repo port should be
tightened to use the DocumentStatus enum rather than raw strings: update the
type hints for list_documents and count_documents to accept DocumentStatus |
None (and any other methods that filter by status), import DocumentStatus from
openrag.core.models.catalog, and adjust any internal references/signatures
(e.g., list_documents, count_documents) so adapters and implementations must
pass/accept DocumentStatus values instead of arbitrary strings; ensure type
imports and exported signatures are updated consistently across the interface
and adapters.
In `@openrag/core/ports/prompt_repo.py`:
- Around line 7-23: The port methods get_by_type and get_active accept raw str
for the prompt type; tighten the contract to use the existing PromptType enum by
importing PromptType from openrag.core.models.prompt and changing the parameter
types on PromptRepository.get_by_type(prompt_type: PromptType) -> list[Prompt]
and PromptRepository.get_active(prompt_type: PromptType) -> Prompt | None; keep
other signatures (e.g., create_prompt, get_prompt) unchanged so implementations
receive a validated PromptType instead of arbitrary strings.
🪄 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: 19ddeb39-a79a-4009-9f15-c24f9e64b207
📒 Files selected for processing (35)
openrag/core/chunking/__init__.pyopenrag/core/chunking/chunking_strategy.pyopenrag/core/chunking/registry.pyopenrag/core/embeddings/__init__.pyopenrag/core/embeddings/embedder.pyopenrag/core/embeddings/registry.pyopenrag/core/indexing/parsers/__init__.pyopenrag/core/indexing/parsers/document_parser.pyopenrag/core/indexing/parsers/registry.pyopenrag/core/llm/__init__.pyopenrag/core/llm/llm.pyopenrag/core/llm/registry.pyopenrag/core/ports/__init__.pyopenrag/core/ports/audit_log_repo.pyopenrag/core/ports/catalog_store.pyopenrag/core/ports/chunk_repo.pyopenrag/core/ports/conversation_repo.pyopenrag/core/ports/document_repo.pyopenrag/core/ports/entity_repo.pyopenrag/core/ports/idempotency_repo.pyopenrag/core/ports/job_repo.pyopenrag/core/ports/model_endpoint_repo.pyopenrag/core/ports/partition_repo.pyopenrag/core/ports/preset_repo.pyopenrag/core/ports/prompt_repo.pyopenrag/core/ports/topic_tag_repo.pyopenrag/core/ports/user_repo.pyopenrag/core/rerankers/__init__.pyopenrag/core/rerankers/registry.pyopenrag/core/rerankers/reranker.pyopenrag/core/vector_stores/__init__.pyopenrag/core/vector_stores/vector_store.pyopenrag/core/vlm/__init__.pyopenrag/core/vlm/registry.pyopenrag/core/vlm/vlm.py
Comment on lines
+3
to
+4
| from openrag.core.embeddings.embedder import Embedder | ||
| from openrag.core.utils.registry import Registry |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and examine its imports
cat -n openrag/core/embeddings/registry.py | head -20Repository: linagora/openrag
Length of output: 292
🏁 Script executed:
# Verify the target modules exist
ls -la openrag/core/embeddings/embedder.py
ls -la openrag/core/utils/registry.pyRepository: linagora/openrag
Length of output: 223
🏁 Script executed:
# Check how imports are done in similar files in the core package
rg "^from (openrag\.|\.|\w+)" openrag/core/embeddings/ -A1 | head -30Repository: linagora/openrag
Length of output: 720
🏁 Script executed:
# Check import patterns in core/utils to see the standard
rg "^from (openrag\.|\.|\w+)" openrag/core/utils/ -A1 | head -30Repository: linagora/openrag
Length of output: 715
🏁 Script executed:
# Check if there are any other imports or issues in the file
wc -l openrag/core/embeddings/registry.py
cat -n openrag/core/embeddings/registry.pyRepository: linagora/openrag
Length of output: 330
Use absolute imports without the openrag. prefix.
Line 3 imports from the same package and should use a relative import. Line 4 imports from a cross-package module and should use an absolute import rooted at openrag/, without the openrag. prefix—this will fail at runtime with the current format.
Proposed fix
-from openrag.core.embeddings.embedder import Embedder
-from openrag.core.utils.registry import Registry
+from .embedder import Embedder
+from core.utils.registry import Registry📝 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
| from openrag.core.embeddings.embedder import Embedder | |
| from openrag.core.utils.registry import Registry | |
| from .embedder import Embedder | |
| from core.utils.registry import Registry |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/core/embeddings/registry.py` around lines 3 - 4, Replace the
incorrect imports in registry.py: change the package-local import "from
openrag.core.embeddings.embedder import Embedder" to a relative import "from
.embedder import Embedder", and change the cross-package import "from
openrag.core.utils.registry import Registry" to an absolute import rooted at the
package root without the leading prefix: "from core.utils.registry import
Registry"; update the lines that reference Embedder and Registry accordingly.
Comment on lines
+3
to
+4
| from openrag.core.indexing.parsers.document_parser import DocumentParser | ||
| from openrag.core.indexing.parsers.registry import parser_registry |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and examine its contents
cat -n openrag/core/indexing/parsers/__init__.py 2>/dev/null || echo "File not found at specified path"Repository: linagora/openrag
Length of output: 328
🏁 Script executed:
# Search for how imports are done elsewhere in openrag/ for similar cases
rg "^from (openrag\.|\.)" openrag/core/indexing/parsers/ -n --color=never | head -20Repository: linagora/openrag
Length of output: 617
🏁 Script executed:
# Check the actual structure of the parsers directory
ls -la openrag/core/indexing/parsers/ 2>/dev/null || echo "Directory not found"Repository: linagora/openrag
Length of output: 356
🏁 Script executed:
# Look for imports in __init__.py files to understand the pattern used in this codebase
rg "^from" openrag/core/indexing/__init__.py -n --color=never
rg "^from" openrag/components/reranker/__init__.py -n --color=neverRepository: linagora/openrag
Length of output: 92
🏁 Script executed:
# Search for imports of this __init__.py module
rg "from openrag.core.indexing.parsers import|from .parsers import" --type pythonRepository: linagora/openrag
Length of output: 90
🏁 Script executed:
# Check if there are any runtime issues or error logs mentioning these imports
rg "openrag.core.indexing.parsers" --type python -c | head -20Repository: linagora/openrag
Length of output: 90
🏁 Script executed:
# Search for imports of this __init__.py module
rg "from openrag.core.indexing.parsers import|from .parsers import"Repository: linagora/openrag
Length of output: 42
🏁 Script executed:
# Check all uses of openrag.core.indexing.parsers
rg "openrag\.core\.indexing\.parsers"Repository: linagora/openrag
Length of output: 626
🏁 Script executed:
# Verify the import behavior - check if there's a setup.py or pyproject.toml that affects path resolution
cat openrag/core/indexing/__init__.py 2>/dev/null | head -20Repository: linagora/openrag
Length of output: 42
Replace openrag. prefix imports with relative imports for same-package modules.
Both modules are in the same package, and the current openrag.core... style imports will fail at runtime. Use relative imports to match the established pattern in other packages like openrag/components/reranker/.
Proposed fix
-from openrag.core.indexing.parsers.document_parser import DocumentParser
-from openrag.core.indexing.parsers.registry import parser_registry
+from .document_parser import DocumentParser
+from .registry import parser_registryPer coding guidelines, absolute imports from the openrag/ root should not use the openrag. prefix (which fails at runtime); relative imports are acceptable within the same package.
📝 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
| from openrag.core.indexing.parsers.document_parser import DocumentParser | |
| from openrag.core.indexing.parsers.registry import parser_registry | |
| from .document_parser import DocumentParser | |
| from .registry import parser_registry |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/core/indexing/parsers/__init__.py` around lines 3 - 4, The imports in
__init__.py use the package-root prefix and should be converted to relative
imports: replace the top-level imports of DocumentParser and parser_registry
with package-relative imports so that DocumentParser (from document_parser) and
parser_registry (from registry) are imported via relative paths (e.g., using
dot-prefixed imports) to match the package pattern and avoid runtime import
errors.
Comment on lines
+3
to
+4
| from openrag.core.llm.llm import LLM | ||
| from openrag.core.llm.registry import llm_registry |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only check: inspect import-root conventions and package configuration without importing project modules.
echo "Existing root-style imports:"
rg -n --type=py -C2 '^\s*from (components|core|utils)\.' openrag || true
echo
echo "openrag-prefixed imports:"
rg -n --type=py -C2 '^\s*from openrag\.' openrag || true
echo
echo "Packaging / PYTHONPATH hints:"
rg -n -C2 'PYTHONPATH|pythonpath|package-dir|packages|uvicorn|module' pyproject.toml setup.cfg setup.py Dockerfile docker-compose.yml .github 2>/dev/null || trueRepository: linagora/openrag
Length of output: 50372
Replace these imports with relative imports.
Lines 3-4 use from openrag.core.llm.xxx style, which violates the convention used throughout the codebase. Since both symbols are in the same package, use relative imports instead:
Proposed fix
-from openrag.core.llm.llm import LLM
-from openrag.core.llm.registry import llm_registry
+from .llm import LLM
+from .registry import llm_registryPer coding guidelines, openrag/**/*.py must use absolute imports from the openrag/ root (e.g., from components.xxx, from utils.xxx) across packages, while relative imports are acceptable within the same package. These imports are within openrag/core/llm/, so from .llm and from .registry are the correct form.
📝 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
| from openrag.core.llm.llm import LLM | |
| from openrag.core.llm.registry import llm_registry | |
| from .llm import LLM | |
| from .registry import llm_registry |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/core/llm/__init__.py` around lines 3 - 4, Replace the absolute
imports with relative imports: change the top-level imports that import LLM and
llm_registry to use package-relative form (i.e., import LLM from .llm and
llm_registry from .registry) so that the symbols LLM and llm_registry are
imported via relative imports within the same package.
Comment on lines
+25
to
+38
| async def generate_json(self, prompt: str, **kwargs) -> dict: | ||
| """Generate a JSON response. Default: parse generate() output. | ||
|
|
||
| Raises LLMParsingError if the LLM output is not valid JSON. | ||
| """ | ||
| response = await self.generate(prompt, **kwargs) | ||
| text = response.strip() | ||
| try: | ||
| return json.loads(text) | ||
| except json.JSONDecodeError as exc: | ||
| raise LLMParsingError( | ||
| raw_response=text, | ||
| parse_error=str(exc), | ||
| ) from exc |
There was a problem hiding this comment.
Validate that generate_json() returns a JSON object.
Line 33 can return a list/scalar from json.loads, which violates the dict return contract and can break callers expecting mapping semantics.
Proposed fix
text = response.strip()
try:
- return json.loads(text)
+ parsed = json.loads(text)
except json.JSONDecodeError as exc:
raise LLMParsingError(
raw_response=text,
parse_error=str(exc),
) from exc
+ if not isinstance(parsed, dict):
+ raise LLMParsingError(
+ raw_response=text,
+ parse_error=f"Expected JSON object, got {type(parsed).__name__}",
+ )
+ return parsed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/core/llm/llm.py` around lines 25 - 38, generate_json currently parses
JSON then returns whatever json.loads yields which can be a list or scalar;
update the generate_json method to validate that the parsed result is a
dict/mapping and raise LLMParsingError if it's not: after json.loads(text) in
generate_json, check isinstance(result, dict) (or collections.abc.Mapping) and
if false raise LLMParsingError with raw_response=text and a parse_error message
indicating "expected JSON object but got <type>", otherwise return the dict;
reference function generate_json and exception LLMParsingError when implementing
the change.
Comment on lines
+16
to
+29
| @abstractmethod | ||
| async def get_conversation(self, conversation_id: str) -> Conversation | None: ... | ||
|
|
||
| @abstractmethod | ||
| async def list_conversations(self, user_id: int, partition: str | None = None) -> list[Conversation]: ... | ||
|
|
||
| @abstractmethod | ||
| async def delete_conversation(self, conversation_id: str) -> bool: ... | ||
|
|
||
| @abstractmethod | ||
| async def add_message(self, message: Message) -> Message: ... | ||
|
|
||
| @abstractmethod | ||
| async def list_messages(self, conversation_id: str) -> list[Message]: ... |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Carry user and partition scope through item-level conversation operations.
list_conversations is scoped, but get_conversation, delete_conversation, add_message, and list_messages are not. Since Conversation includes user_id and partition_scope, the port should let implementations enforce those predicates atomically instead of relying on every caller to remember a separate authorization check.
Suggested scoped contract shape
- async def get_conversation(self, conversation_id: str) -> Conversation | None: ...
+ async def get_conversation(
+ self,
+ conversation_id: str,
+ user_id: int,
+ partition: str | None = None,
+ ) -> Conversation | None: ...
@@
- async def delete_conversation(self, conversation_id: str) -> bool: ...
+ async def delete_conversation(
+ self,
+ conversation_id: str,
+ user_id: int,
+ partition: str | None = None,
+ ) -> bool: ...
@@
- async def add_message(self, message: Message) -> Message: ...
+ async def add_message(
+ self,
+ message: Message,
+ user_id: int,
+ partition: str | None = None,
+ ) -> Message: ...
@@
- async def list_messages(self, conversation_id: str) -> list[Message]: ...
+ async def list_messages(
+ self,
+ conversation_id: str,
+ user_id: int,
+ partition: str | None = None,
+ ) -> list[Message]: ...As per coding guidelines, use token-based authentication with role-based access control for multi-tenant partition access.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/core/ports/conversation_repo.py` around lines 16 - 29, The port
methods get_conversation, delete_conversation, add_message, and list_messages
must carry the same user/partition scope as list_conversations so
implementations can enforce tenant/partition predicates atomically; update their
signatures to accept user_id: int and partition: str | None (or include a single
scope token param) and adjust add_message to accept both Message and scope (or
validate message.user_id and message.partition_scope against the passed scope),
ensuring Conversation (with its user_id and partition_scope) and
list_conversations remain consistent; also document that implementations should
validate a token/role-based auth scope when enforcing partition access.
Comment on lines
+14
to
+18
| @abstractmethod | ||
| async def get_by_document(self, document_id: str) -> list[dict]: ... | ||
|
|
||
| @abstractmethod | ||
| async def delete_by_document(self, document_id: str) -> int: ... |
There was a problem hiding this comment.
Partition-scope document tag reads and deletes.
search takes partition, but get_by_document and delete_by_document do not. This weakens the repository contract for multi-tenant access and makes cross-partition tag reads/deletes easier to introduce accidentally.
Suggested contract adjustment
- async def get_by_document(self, document_id: str) -> list[dict]: ...
+ async def get_by_document(self, partition: str, document_id: str) -> list[dict]: ...
`@abstractmethod`
- async def delete_by_document(self, document_id: str) -> int: ...
+ async def delete_by_document(self, partition: str, document_id: str) -> int: ...As per coding guidelines, use token-based authentication with role-based access control (RBAC) for multi-tenant partition access.
📝 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
| @abstractmethod | |
| async def get_by_document(self, document_id: str) -> list[dict]: ... | |
| @abstractmethod | |
| async def delete_by_document(self, document_id: str) -> int: ... | |
| `@abstractmethod` | |
| async def get_by_document(self, partition: str, document_id: str) -> list[dict]: ... | |
| `@abstractmethod` | |
| async def delete_by_document(self, partition: str, document_id: str) -> int: ... |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/core/ports/topic_tag_repo.py` around lines 14 - 18, The repository
interface methods get_by_document and delete_by_document lack a partition
parameter which weakens multi-tenant safety; update their signatures to include
a partition: str argument (matching search's partition usage) and propagate this
change to all implementations so reads/deletes are partition-scoped, and ensure
these methods (get_by_document, delete_by_document, and search) validate an
incoming token and enforce RBAC checks before performing operations (rejecting
cross-partition access when the token’s roles/claims do not authorize the
requested partition).
Comment on lines
+8
to
+50
| from openrag.core.models.user import User, UserPartition | ||
|
|
||
|
|
||
| class UserRepository(ABC): | ||
| """CRUD operations for users and partition memberships.""" | ||
|
|
||
| @abstractmethod | ||
| async def create_user(self, user: User) -> User: ... | ||
|
|
||
| @abstractmethod | ||
| async def get_user(self, user_id: int) -> User | None: ... | ||
|
|
||
| @abstractmethod | ||
| async def get_user_by_token(self, token_hash: str) -> User | None: ... | ||
|
|
||
| @abstractmethod | ||
| async def get_user_by_external_id(self, external_id: str) -> User | None: ... | ||
|
|
||
| @abstractmethod | ||
| async def list_users(self, offset: int = 0, limit: int = 50) -> list[User]: ... | ||
|
|
||
| @abstractmethod | ||
| async def update_user(self, user_id: int, **fields: Any) -> User | None: ... | ||
|
|
||
| @abstractmethod | ||
| async def delete_user(self, user_id: int) -> bool: ... | ||
|
|
||
| # ── Partition memberships ───────────────────────────────────────── | ||
|
|
||
| @abstractmethod | ||
| async def assign_partition(self, assignment: UserPartition) -> UserPartition: ... | ||
|
|
||
| @abstractmethod | ||
| async def remove_partition(self, user_id: int, partition: str) -> bool: ... | ||
|
|
||
| @abstractmethod | ||
| async def list_user_partitions(self, user_id: int) -> list[UserPartition]: ... | ||
|
|
||
| @abstractmethod | ||
| async def list_partition_users(self, partition: str) -> list[UserPartition]: ... | ||
|
|
||
| @abstractmethod | ||
| async def update_partition_role(self, user_id: int, partition: str, role: str) -> bool: ... |
There was a problem hiding this comment.
Use PartitionRole for role updates.
Line 50 accepts any string even though UserPartition.role is enum-backed. Tightening the port keeps invalid roles out of partition-membership persistence.
Proposed fix
-from openrag.core.models.user import User, UserPartition
+from openrag.core.models.user import PartitionRole, User, UserPartition
@@
- async def update_partition_role(self, user_id: int, partition: str, role: str) -> bool: ...
+ async def update_partition_role(
+ self,
+ user_id: int,
+ partition: str,
+ role: PartitionRole,
+ ) -> bool: ...As per coding guidelines, “Use token-based authentication with role-based access control (RBAC) for multi-tenant partition access, with role hierarchy: viewer (1) < editor (2) < owner (3)”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/core/ports/user_repo.py` around lines 8 - 50, The
update_partition_role method currently accepts a plain string for the role which
allows invalid values; change the port to accept the enum-backed PartitionRole
instead: import PartitionRole from openrag.core.models.user and update the
abstract method signature in UserRepository (update_partition_role) to take
role: PartitionRole rather than role: str, ensuring callers and implementations
use the PartitionRole enum that backs UserPartition.role to prevent invalid
roles from being persisted.
Comment on lines
+3
to
+4
| from openrag.core.rerankers.registry import reranker_registry | ||
| from openrag.core.rerankers.reranker import Reranker |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and check its current content
cat -n openrag/core/rerankers/__init__.pyRepository: linagora/openrag
Length of output: 293
🏁 Script executed:
# Check the directory structure
find openrag/core/rerankers -type f -name "*.py" | head -20Repository: linagora/openrag
Length of output: 164
🏁 Script executed:
# Look at how other __init__.py files in the codebase handle imports
fd -t f '__init__.py' openrag/components openrag/core openrag/utils --exec head -10 {} +Repository: linagora/openrag
Length of output: 7911
🏁 Script executed:
# Check for any sys.path manipulation or pytest configuration
find . -maxdepth 2 -type f \( -name "conftest.py" -o -name "setup.py" -o -name "pyproject.toml" -o -name "setup.cfg" \) -exec grep -l "sys.path\|PYTHONPATH" {} \;Repository: linagora/openrag
Length of output: 42
🏁 Script executed:
# Check if the openrag.core.rerankers imports actually work by examining how this module is imported elsewhere
rg "from openrag.core.rerankers import|from core.rerankers import|from .rerankers import" -A 2 -B 2Repository: linagora/openrag
Length of output: 42
🏁 Script executed:
# Check if there's any evidence of runtime failures or comments about import issues
rg "openrag\." openrag/core/rerankers/ -B 2 -A 2Repository: linagora/openrag
Length of output: 864
🏁 Script executed:
# Verify the pattern in components/auth which uses from components. (no openrag prefix)
cat openrag/components/auth/__init__.pyRepository: linagora/openrag
Length of output: 681
Use relative imports within the same package.
Lines 3–4 should use relative imports instead of the openrag.core.rerankers prefix, which fails at runtime. Since these imports are from the same package, use the . syntax.
Proposed fix
-from openrag.core.rerankers.registry import reranker_registry
-from openrag.core.rerankers.reranker import Reranker
+from .registry import reranker_registry
+from .reranker import Reranker🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/core/rerankers/__init__.py` around lines 3 - 4, The current top-level
imports in __init__.py use absolute package paths and fail at runtime; change
them to relative imports by importing reranker_registry and Reranker from the
local package using the dot-syntax (i.e., replace references to
openrag.core.rerankers.reranker_registry and openrag.core.rerankers.reranker
with relative imports that import reranker_registry and Reranker from .registry
and .reranker respectively) so the symbols reranker_registry and Reranker are
imported from the same package correctly.
Comment on lines
+20
to
+27
| async def search( | ||
| self, | ||
| embedding: list[float], | ||
| top_k: int = 10, | ||
| collection: str = "default", | ||
| filters: dict[str, Any] | None = None, | ||
| ) -> list[dict[str, Any]]: | ||
| """Search by embedding vector. Returns raw results.""" |
There was a problem hiding this comment.
Expose hybrid-search inputs in the vector-store contract.
search() is dense-only today. That prevents compliant backends from implementing dense + BM25 sparse hybrid search without a breaking interface change later.
Possible contract adjustment
async def search(
self,
+ query: str,
embedding: list[float],
top_k: int = 10,
collection: str = "default",
filters: dict[str, Any] | None = None,
+ **kwargs: Any,
) -> list[dict[str, Any]]:
- """Search by embedding vector. Returns raw results."""
+ """Hybrid search by query text and embedding vector. Returns raw results."""
...As per coding guidelines, “Use the MilvusDB Ray actor for vector database operations with hybrid search (dense + BM25 sparse), accessed via ray.get_actor("Vectordb", namespace="openrag")”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/core/vector_stores/vector_store.py` around lines 20 - 27, The search
method is currently dense-only; modify the VectorStore.search signature to
accept optional sparse/hybrid parameters (e.g., sparse_query: str | None = None
and sparse_weight: float = 0.0) and update the docstring to describe hybrid
behavior and defaults so existing callers remain compatible; ensure backend
implementations (and any code invoking vector search) can detect hybrid requests
and, for Milvus-based implementations, use the MilvusDB Ray actor via
ray.get_actor("Vectordb", namespace="openrag") to perform dense+BM25 hybrid
searches when sparse_query is provided and sparse_weight > 0.0.
Comment on lines
+3
to
+4
| from openrag.core.vlm.registry import vlm_registry | ||
| from openrag.core.vlm.vlm import VLM |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the actual file contents
head -20 openrag/core/vlm/__init__.pyRepository: linagora/openrag
Length of output: 209
🏁 Script executed:
# Check other __init__.py files to understand project import conventions
find openrag -name "__init__.py" -type f | head -10 | xargs head -10Repository: linagora/openrag
Length of output: 479
🏁 Script executed:
# Search for patterns of imports in __init__.py files across the project
rg "^from (openrag\.|\.)" openrag/**/__init__.py -A 1 | head -40Repository: linagora/openrag
Length of output: 174
🏁 Script executed:
# Count usage of both import patterns in __init__.py files
echo "=== Using 'from openrag.' pattern ===" && \
rg "from openrag\." openrag/**/__init__.py | wc -l && \
echo "=== Using 'from \.' pattern ===" && \
rg "^from \." openrag/**/__init__.py | wc -lRepository: linagora/openrag
Length of output: 133
🏁 Script executed:
# Show all non-empty __init__.py files to see actual import patterns used
rg -l "^from" openrag/**/__init__.py -A 3Repository: linagora/openrag
Length of output: 86
🏁 Script executed:
# Check if the guideline applies strictly or if both patterns are acceptable
git log --oneline openrag/core/vlm/__init__.py | head -5Repository: linagora/openrag
Length of output: 123
Use relative imports within this package initializer.
Same-package imports should use relative syntax (from .registry and from .vlm) rather than the absolute from openrag.core.vlm... pattern. This aligns with the guideline that relative imports are acceptable within the same package and avoids the openrag. prefix pattern which is not the recommended approach for this codebase.
Proposed fix
-from openrag.core.vlm.registry import vlm_registry
-from openrag.core.vlm.vlm import VLM
+from .registry import vlm_registry
+from .vlm import VLM🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/core/vlm/__init__.py` around lines 3 - 4, Replace the two absolute
imports in the package initializer with relative imports: change references that
import vlm_registry and VLM via "from openrag.core.vlm.registry import
vlm_registry" and "from openrag.core.vlm.vlm import VLM" to use relative module
paths (import the same symbols from .registry and .vlm respectively) so the
package __init__.py exposes vlm_registry and VLM using relative imports.
This was referenced May 5, 2026
This was referenced May 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
All component ABCs and repository port ABCs for the hexagonal architecture.
Component ABCs (co-located with registries)
core/embeddings/— Embedder ABC (embed, embed_single, dimension)core/rerankers/— Reranker ABC (rerank)core/llm/— LLM ABC (generate, chat, stream_chat, generate_json, chat_with_tools)core/vlm/— VLM ABC (caption_image, caption_images_batch)core/vector_stores/— VectorStore ABC (upsert, search, delete, ensure_collection, etc.)core/chunking/— ChunkingStrategy ABC + registrycore/indexing/parsers/— DocumentParser ABC + registryRepository ports
13 ABCs in
core/ports/: DocumentRepository, ChunkRepository, UserRepository,JobRepository, PartitionRepository, ConversationRepository, PromptRepository,
EntityRepository, TopicTagRepository, AuditLogRepository, IdempotencyRepository,
ModelEndpointRepository, PresetRepository.
CatalogStore
Aggregate root in
core/ports/catalog_store.pycomposing all 13 reposwith lifecycle (initialize/shutdown).
Verification
from openrag.core.embeddings import Embedder, embedder_registrySummary by CodeRabbit