Phase 1: Registry[T] + exception hierarchy + core utilities - #329
Merged
andyne13 merged 9 commits intoApr 21, 2026
Conversation
Type-safe registry for pluggable components. Each domain (embedder,
reranker, llm, vlm, chunking, parser) will have its own Registry
instance. Implementations register via @registry.register("name")
decorator and are instantiated via registry.create("name", **kwargs).
Includes RegistryError with helpful message listing available
implementations when a lookup fails.
Consolidates all exception classes into core/utils/exceptions.py. Preserves the existing OpenRAGError API (message, code, status_code, to_dict()) and all existing VDB/Embedding subclasses for backward compatibility. Adds new exception categories for the hexagonal architecture: - ConfigError, RegistryError, PipelineError - AuthError, AuthenticationError (401) - ValidationError (422), NotFoundError (404) with domain subtypes - QuotaExceededError (429) - ServiceUnavailableError (503), CircuitBreakerOpenError - InferenceError with LLMParsingError (502), timeout (504), connection (503) - StorageError with MilvusError, PostgresError Status codes preserved from existing codebase for backward compat. Will be moved to api/error_handlers.py mapping in Phase 10.
Updates utils/exceptions/{__init__,base,vectordb,embeddings}.py to
re-export from openrag.core.utils.exceptions. All existing consumer
imports continue to work unchanged.
New code should import from openrag.core.utils.exceptions directly.
These shims will be removed in Phase 12.
CatalogStore ABC will live in core/ports/catalog_store.py (alongside the repository ABCs it composes), not in a separate core/catalog/ folder. Decision logged with Phase 1 entries.
Pure text cleaning functions moved from components/indexer/utils/text_sanitizer.py. No infrastructure imports — only re and unicodedata. Includes sanitize_text(), clean_markdown_table_spacing(), and sanitize_extracted_text().
Pure filename functions extracted from components/indexer/utils/files.py. Only the infrastructure-free parts: sanitize_filename() and make_unique_filename(). The rest (save_file_to_disk, serialize_file) stays in the old location until Phase 5+.
Pure utility for detecting when errors originate from external HTTP resources (VLM image fetches, etc.) rather than internal failures. Moved from utils/external_resource_errors.py.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 44 minutes and 29 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR establishes a canonical exception hierarchy in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 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: 9
🤖 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/utils/exceptions.py`:
- Around line 188-195: CircuitBreakerOpenError (and other leaf subclasses like
AuthenticationError → AuthError) can raise TypeError when callers pass
code/status_code via **kwargs because the subclass also injects a hard-coded
code; to fix, remove any incoming code and status_code from kwargs before
calling the parent __init__ (e.g., pop 'code' and 'status_code' from kwargs in
CircuitBreakerOpenError.__init__) or alternatively call
super().__init__(**kwargs) first and then set self.code/self.status_code to the
intended hard-coded values (apply the same pattern to other subclasses such as
AuthenticationError/AuthError and anywhere raise_from/wrappers may forward
kwargs).
- Around line 48-50: The import block in openrag/core/utils/exceptions.py is not
sorted ( Ruff I001 )—ensure the imports (including the existing "from __future__
import annotations") are ordered per ruff/isort rules (future imports first,
then stdlib, third-party, local) and that spacing between groups is correct;
either run "ruff check --fix" (or "uv run ruff format") to auto-fix, or manually
reorder the import statements to match isort conventions and re-run ruff until
no I001 errors remain.
- Around line 55-72: OpenRAGError currently freezes Exception.args by passing
the formatted string to super().__init__, causing subclass changes to
self.code/self.status_code to be lost in str(exc); fix by changing
OpenRAGError.__init__ to call super().__init__(message) (or no formatted arg)
and add a __str__ method on OpenRAGError that returns f"{self.code}:
{self.message}" so stringification is computed lazily and respects subclasses
(affects OpenRAGError.__init__ and add OpenRAGError.__str__).
In `@openrag/core/utils/registry.py`:
- Around line 15-76: Replace typing generics with Python 3.12 PEP 585/695
syntax: make Registry a parametric class using class Registry[T] instead of
inheriting Generic[T], use the built-in type for class objects (e.g., annotate
self._registry as dict[str, type[T]] and have get_class return type[T]), and
keep return/param types for create/get_class as T and str/list[str]
respectively; also remove the redundant pass in RegistryError. Update
annotations in __init__, create, get_class, and list_registered to use built-in
generics and the type[...] form, and leave method names (register, create,
get_class, list_registered, __contains__) and the _registry attribute unchanged.
- Around line 23-26: Replace the local RegistryError class in Registry (the
RegistryError defined in registry.py) with the canonical exception from the
exceptions module: remove the duplicate class and add an import of the canonical
RegistryError (which inherits OpenRAGError and provides code, status_code,
to_dict()) from openrag.core.utils.exceptions (or the exceptions module where
RegistryError/OpenRAGError are defined) so all code using Registry.lookup,
Registry.register, etc. will raise and catch the unified RegistryError type;
ensure no other local references to the removed class remain and run tests to
confirm behavior unchanged.
- Around line 1-13: Update imports that incorrectly include the "openrag."
prefix to the repo-root relative form: remove the "openrag." prefix in the
docstring example in Registry (the example import line in the registry.py
docstring) and in the actual import statements in the exceptions modules and
test: change any "from openrag.core.utils.exceptions import ..." to "from
core.utils.exceptions import ..." and change "from
openrag.components.indexer.vectordb import MilvusDB" to "from
components.indexer.vectordb import MilvusDB"; locate these by searching for the
literal import text in registry.py (docstring example),
utils/exceptions/base.py, utils/exceptions/embeddings.py,
utils/exceptions/vectordb.py, utils/exceptions/__init__.py, and
tests/test_vectordb.py and update them accordingly.
In `@openrag/core/utils/text.py`:
- Around line 83-85: The current code unconditionally uses cleaned_cells[1:-1],
which drops content when the input row omits outer pipes (e.g., "a | b"); update
the logic around parts/cleaned_cells/new_line to detect whether the original
line has leading and trailing pipe characters (check
line.strip().startswith("|") and line.strip().endswith("|")) and only use
cleaned_cells[1:-1] when both are present; otherwise use the full cleaned_cells
(or cleaned_cells[0:]) when joining. Then build new_line consistently (e.g., "|
" + " | ".join(cells_to_join) + " |") so rows without outer pipes preserve their
cells while still producing the normalized pipe-wrapped output.
In `@openrag/utils/exceptions/__init__.py`:
- Around line 1-3: The shim in __init__.py imports using the prefixed path
"openrag.core.utils.exceptions" and uses a star import which can both break at
runtime and over-export; change the import to the package-root relative
canonical module (import from core.utils.exceptions) and replace the wildcard
re-export with an explicit export list: either import the specific exception
names you want to re-export (e.g., ExceptionName1, ExceptionName2) and re-export
them, or ensure core.utils.exceptions defines __all__ and then import those
names; update the module to import the explicit symbols from
core.utils.exceptions and expose only those names to callers.
In `@REFACTORING_DECISION_LOG.md`:
- Around line 58-77: The Phase 1 block is inserted inside and after the existing
"## Template for future entries" header, creating a duplicate template heading
and MD024; move the entire "## Phase 1 — Registry + Exceptions (2026-04-21)"
section (including its content and the trailing separator) so it appears above
the single "## Template for future entries" entry, remove the duplicate "##
Template for future entries" that was re-added, and ensure the template's
closing code fence and formatting remain intact; look for the headings "## Phase
1 — Registry + Exceptions (2026-04-21)" and "## Template for future entries" to
locate the blocks to reorder.
🪄 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: 0dc0704f-25a8-4a55-a047-41df78bc739d
📒 Files selected for processing (11)
REFACTORING_DECISION_LOG.mdopenrag/core/catalog/__init__.pyopenrag/core/utils/exceptions.pyopenrag/core/utils/external_errors.pyopenrag/core/utils/filename.pyopenrag/core/utils/registry.pyopenrag/core/utils/text.pyopenrag/utils/exceptions/__init__.pyopenrag/utils/exceptions/base.pyopenrag/utils/exceptions/embeddings.pyopenrag/utils/exceptions/vectordb.py
Comment on lines
+48
to
+50
| from __future__ import annotations | ||
|
|
||
|
|
There was a problem hiding this comment.
Ruff I001 import-sort failure is blocking the lint CI job.
Pipeline failure: ruff check failed (I001): Import block is un-sorted or un-formatted at line 48. Run ruff check --fix locally (or uv run ruff format) before pushing.
🧰 Tools
🪛 GitHub Actions: Linting
[error] 48-48: ruff check failed (I001): Import block is un-sorted or un-formatted
🪛 GitHub Check: lint (3.12)
[failure] 48-48: Ruff (I001)
openrag/core/utils/exceptions.py:48:1: I001 Import block is un-sorted or un-formatted
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/core/utils/exceptions.py` around lines 48 - 50, The import block in
openrag/core/utils/exceptions.py is not sorted ( Ruff I001 )—ensure the imports
(including the existing "from __future__ import annotations") are ordered per
ruff/isort rules (future imports first, then stdlib, third-party, local) and
that spacing between groups is correct; either run "ruff check --fix" (or "uv
run ruff format") to auto-fix, or manually reorder the import statements to
match isort conventions and re-run ruff until no I001 errors remain.
Comment on lines
+55
to
+72
| class OpenRAGError(Exception): | ||
| """Base class for all OpenRAG exceptions. | ||
|
|
||
| Preserves the existing API: message, code, status_code, to_dict(). | ||
| """ | ||
|
|
||
| def __init__( | ||
| self, | ||
| message: str, | ||
| code: str = "OPENRAG_ERROR", | ||
| status_code: int = 500, | ||
| **kwargs, | ||
| ): | ||
| self.message = message | ||
| self.code = code | ||
| self.status_code = status_code | ||
| self.extra = kwargs or {} | ||
| super().__init__(f"{self.code}: {self.message}") |
There was a problem hiding this comment.
Stale Exception.args after subclass mutation of code/status_code.
OpenRAGError.__init__ calls super().__init__(f"{self.code}: {self.message}"), which freezes the parent's args tuple at construction time. Several subclasses (AuthenticationError, CircuitBreakerOpenError, LLMParsingError, InferenceTimeoutError, InferenceConnectionError, MilvusError, PostgresError) call super().__init__(...) first and then overwrite self.code/self.status_code. The effect:
exc.code/exc.status_code/to_dict()→ new (correct) valuesstr(exc)/ logging%sof the exception → stale parent code, e.g."STORAGE_ERROR: ..."on aMilvusError,"AUTH_ERROR: ..."on anAuthenticationError
This silently degrades log/trace quality. Two safe fixes:
♻️ Option A — pass `code`/`status_code` through `super().__init__`
class AuthenticationError(AuthError):
def __init__(self, message: str, **kwargs):
- super().__init__(message, **kwargs)
- self.code = "AUTHENTICATION_ERROR"
- self.status_code = 401
+ super().__init__(message, **kwargs)
+ self.code = "AUTHENTICATION_ERROR"
+ self.status_code = 401
+ self.args = (f"{self.code}: {self.message}",)♻️ Option B (preferred) — make `OpenRAGError.__str__` compute lazily
class OpenRAGError(Exception):
def __init__(self, message, code="OPENRAG_ERROR", status_code=500, **kwargs):
self.message = message
self.code = code
self.status_code = status_code
self.extra = kwargs or {}
- super().__init__(f"{self.code}: {self.message}")
+ super().__init__(message)
+
+ def __str__(self) -> str:
+ return f"{self.code}: {self.message}"This keeps the current public string format while letting subclass mutations propagate consistently.
📝 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
| class OpenRAGError(Exception): | |
| """Base class for all OpenRAG exceptions. | |
| Preserves the existing API: message, code, status_code, to_dict(). | |
| """ | |
| def __init__( | |
| self, | |
| message: str, | |
| code: str = "OPENRAG_ERROR", | |
| status_code: int = 500, | |
| **kwargs, | |
| ): | |
| self.message = message | |
| self.code = code | |
| self.status_code = status_code | |
| self.extra = kwargs or {} | |
| super().__init__(f"{self.code}: {self.message}") | |
| class OpenRAGError(Exception): | |
| """Base class for all OpenRAG exceptions. | |
| Preserves the existing API: message, code, status_code, to_dict(). | |
| """ | |
| def __init__( | |
| self, | |
| message: str, | |
| code: str = "OPENRAG_ERROR", | |
| status_code: int = 500, | |
| **kwargs, | |
| ): | |
| self.message = message | |
| self.code = code | |
| self.status_code = status_code | |
| self.extra = kwargs or {} | |
| super().__init__(message) | |
| def __str__(self) -> str: | |
| return f"{self.code}: {self.message}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/core/utils/exceptions.py` around lines 55 - 72, OpenRAGError
currently freezes Exception.args by passing the formatted string to
super().__init__, causing subclass changes to self.code/self.status_code to be
lost in str(exc); fix by changing OpenRAGError.__init__ to call
super().__init__(message) (or no formatted arg) and add a __str__ method on
OpenRAGError that returns f"{self.code}: {self.message}" so stringification is
computed lazily and respects subclasses (affects OpenRAGError.__init__ and add
OpenRAGError.__str__).
Comment on lines
+188
to
+195
| def __init__(self, service_type: str, **kwargs): | ||
| self.service_type = service_type | ||
| super().__init__( | ||
| f"Circuit breaker open for {service_type} — service unavailable", | ||
| **kwargs, | ||
| ) | ||
| self.code = "CIRCUIT_BREAKER_OPEN" | ||
|
|
There was a problem hiding this comment.
CircuitBreakerOpenError will crash if a caller passes code= via **kwargs.
ServiceUnavailableError.__init__ already forwards a hard-coded code="SERVICE_UNAVAILABLE" to OpenRAGError.__init__. If any caller (or raise_from/wrapper) passes code= or status_code= into CircuitBreakerOpenError(**kwargs), you get TypeError: got multiple values for keyword argument 'code'. Same latent risk exists for every subclass that forwards **kwargs through a parent that also injects code/status_code (e.g. AuthenticationError → AuthError forwards kwargs then AuthError adds code="AUTH_ERROR").
Consider popping code/status_code from kwargs in the leaf class before forwarding, or setting them only after super().__init__ (combined with the __str__ fix above).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/core/utils/exceptions.py` around lines 188 - 195,
CircuitBreakerOpenError (and other leaf subclasses like AuthenticationError →
AuthError) can raise TypeError when callers pass code/status_code via **kwargs
because the subclass also injects a hard-coded code; to fix, remove any incoming
code and status_code from kwargs before calling the parent __init__ (e.g., pop
'code' and 'status_code' from kwargs in CircuitBreakerOpenError.__init__) or
alternatively call super().__init__(**kwargs) first and then set
self.code/self.status_code to the intended hard-coded values (apply the same
pattern to other subclasses such as AuthenticationError/AuthError and anywhere
raise_from/wrappers may forward kwargs).
Comment on lines
+1
to
+13
| """Generic registry pattern for pluggable components. | ||
|
|
||
| Usage: | ||
| from openrag.core.utils.registry import Registry | ||
|
|
||
| embedder_registry: Registry[Embedder] = Registry("embedder") | ||
|
|
||
| @embedder_registry.register("vllm") | ||
| class VLLMEmbedder(Embedder): | ||
| ... | ||
|
|
||
| instance = embedder_registry.create("vllm", endpoint="http://...") | ||
| """ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm sys.path setup and whether `import openrag` vs `import core` is expected.
fd -t f 'pyproject.toml|setup.cfg|setup.py|Dockerfile|conftest.py' -x echo '===' {} \; -x cat {}
echo '--- existing absolute import styles in repo ---'
rg -n --type=py '^\s*from\s+(openrag\.)?(core|utils|components|routers|models|config)\b' -g '!**/venv/**' -g '!**/.venv/**' | head -80
echo '--- any __init__.py at repo root declaring openrag package? ---'
fd -t f '__init__.py' -d 2Repository: linagora/openrag
Length of output: 32539
Fix import paths to remove openrag. prefix — these fail at runtime due to PYTHONPATH configuration.
Per repo convention, the Python path root is openrag/ (set in Dockerfile as ENV PYTHONPATH=/app/openrag/), so imports must use the format from core.utils.xxx import ... not from openrag.core.utils.xxx import ....
Locations needing fixes:
openrag/core/utils/registry.pyline 4: docstring exampleopenrag/utils/exceptions/base.pyline 3openrag/utils/exceptions/embeddings.pyline 3openrag/utils/exceptions/vectordb.pyline 3openrag/utils/exceptions/__init__.pyline 3tests/test_vectordb.pyline 1
Replace from openrag.core.utils.exceptions import ... with from core.utils.exceptions import ... and from openrag.components.indexer.vectordb import MilvusDB with from components.indexer.vectordb import MilvusDB.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/core/utils/registry.py` around lines 1 - 13, Update imports that
incorrectly include the "openrag." prefix to the repo-root relative form: remove
the "openrag." prefix in the docstring example in Registry (the example import
line in the registry.py docstring) and in the actual import statements in the
exceptions modules and test: change any "from openrag.core.utils.exceptions
import ..." to "from core.utils.exceptions import ..." and change "from
openrag.components.indexer.vectordb import MilvusDB" to "from
components.indexer.vectordb import MilvusDB"; locate these by searching for the
literal import text in registry.py (docstring example),
utils/exceptions/base.py, utils/exceptions/embeddings.py,
utils/exceptions/vectordb.py, utils/exceptions/__init__.py, and
tests/test_vectordb.py and update them accordingly.
Comment on lines
+23
to
+26
| class RegistryError(Exception): | ||
| """Raised when a registry lookup fails.""" | ||
|
|
||
| pass |
There was a problem hiding this comment.
Duplicate RegistryError that bypasses the unified hierarchy.
This RegistryError(Exception) shadows the canonical RegistryError(OpenRAGError) defined in openrag/core/utils/exceptions.py (line 92). Because they're distinct classes, except RegistryError in one module won't catch the other, and the local one lacks code, status_code, to_dict(), and OpenRAGError inheritance required by the coding guideline.
Import from the canonical module instead:
♻️ Proposed fix
-from collections.abc import Callable
-from typing import Any, Generic, Type, TypeVar
+from collections.abc import Callable
+from typing import Any, Generic, TypeVar
+
+from core.utils.exceptions import RegistryError
T = TypeVar("T")
-
-
-class RegistryError(Exception):
- """Raised when a registry lookup fails."""
-
- passAs per coding guidelines: "All custom exceptions must inherit from OpenRAGError defined in openrag/utils/exceptions/".
🧰 Tools
🪛 GitHub Check: lint (3.12)
[failure] 26-26: Ruff (PIE790)
openrag/core/utils/registry.py:26:5: PIE790 Unnecessary pass statement
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/core/utils/registry.py` around lines 23 - 26, Replace the local
RegistryError class in Registry (the RegistryError defined in registry.py) with
the canonical exception from the exceptions module: remove the duplicate class
and add an import of the canonical RegistryError (which inherits OpenRAGError
and provides code, status_code, to_dict()) from openrag.core.utils.exceptions
(or the exceptions module where RegistryError/OpenRAGError are defined) so all
code using Registry.lookup, Registry.register, etc. will raise and catch the
unified RegistryError type; ensure no other local references to the removed
class remain and run tests to confirm behavior unchanged.
Comment on lines
+83
to
+85
| parts = line.split("|") | ||
| cleaned_cells = [cell.strip() for cell in parts] | ||
| new_line = "| " + " | ".join(cleaned_cells[1:-1]) + " |" |
There was a problem hiding this comment.
Preserve cells when markdown rows omit outer pipes.
cleaned_cells[1:-1] drops all content for valid rows like a | b, returning | |. Handle optional leading/trailing pipes before slicing.
🐛 Proposed fix
- parts = line.split("|")
- cleaned_cells = [cell.strip() for cell in parts]
- new_line = "| " + " | ".join(cleaned_cells[1:-1]) + " |"
+ has_leading_pipe = line.lstrip().startswith("|")
+ has_trailing_pipe = line.rstrip().endswith("|")
+ cleaned_cells = [cell.strip() for cell in line.split("|")]
+
+ start = 1 if has_leading_pipe else 0
+ end = -1 if has_trailing_pipe else len(cleaned_cells)
+ inner = " | ".join(cleaned_cells[start:end])
+
+ if has_leading_pipe and has_trailing_pipe:
+ new_line = f"| {inner} |"
+ elif has_leading_pipe:
+ new_line = f"| {inner}"
+ elif has_trailing_pipe:
+ new_line = f"{inner} |"
+ else:
+ new_line = inner📝 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
| parts = line.split("|") | |
| cleaned_cells = [cell.strip() for cell in parts] | |
| new_line = "| " + " | ".join(cleaned_cells[1:-1]) + " |" | |
| has_leading_pipe = line.lstrip().startswith("|") | |
| has_trailing_pipe = line.rstrip().endswith("|") | |
| cleaned_cells = [cell.strip() for cell in line.split("|")] | |
| start = 1 if has_leading_pipe else 0 | |
| end = -1 if has_trailing_pipe else len(cleaned_cells) | |
| inner = " | ".join(cleaned_cells[start:end]) | |
| if has_leading_pipe and has_trailing_pipe: | |
| new_line = f"| {inner} |" | |
| elif has_leading_pipe: | |
| new_line = f"| {inner}" | |
| elif has_trailing_pipe: | |
| new_line = f"{inner} |" | |
| else: | |
| new_line = inner |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/core/utils/text.py` around lines 83 - 85, The current code
unconditionally uses cleaned_cells[1:-1], which drops content when the input row
omits outer pipes (e.g., "a | b"); update the logic around
parts/cleaned_cells/new_line to detect whether the original line has leading and
trailing pipe characters (check line.strip().startswith("|") and
line.strip().endswith("|")) and only use cleaned_cells[1:-1] when both are
present; otherwise use the full cleaned_cells (or cleaned_cells[0:]) when
joining. Then build new_line consistently (e.g., "| " + " |
".join(cells_to_join) + " |") so rows without outer pipes preserve their cells
while still producing the normalized pipe-wrapped output.
Comment on lines
+1
to
+3
| # Re-export from canonical location for backward compatibility. | ||
| # New code should import from openrag.core.utils.exceptions directly. | ||
| from openrag.core.utils.exceptions import * # noqa: F401,F403 |
There was a problem hiding this comment.
Verify openrag. prefix works at runtime, and consider explicit re-exports.
Two concerns:
- Per the repo's import convention (Python path root is
openrag/), absolute imports should not be prefixed withopenrag.. If that convention holds, this shim willImportErrorat runtime — which would defeat the whole backward-compat promise of Phase 1. The verification script in theregistry.pycomment covers this; same fix applies here (from core.utils.exceptions import *). from X import *without an__all__incore/utils/exceptions.pyre-exports everything that happens to be module-level (includingannotationsfrom__future__, and any future helper imports). For a stability-critical shim, prefer an explicit export list or define__all__in the canonical module.
Based on learnings: "Never suggest from openrag.components.xxx style imports — these will fail at runtime."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/utils/exceptions/__init__.py` around lines 1 - 3, The shim in
__init__.py imports using the prefixed path "openrag.core.utils.exceptions" and
uses a star import which can both break at runtime and over-export; change the
import to the package-root relative canonical module (import from
core.utils.exceptions) and replace the wildcard re-export with an explicit
export list: either import the specific exception names you want to re-export
(e.g., ExceptionName1, ExceptionName2) and re-export them, or ensure
core.utils.exceptions defines __all__ and then import those names; update the
module to import the explicit symbols from core.utils.exceptions and expose only
those names to callers.
Comment on lines
58
to
77
| ## Template for future entries | ||
|
|
||
| ## Phase 1 — Registry + Exceptions (2026-04-21) | ||
|
|
||
| **1. Exceptions keep HTTP status_code on the class (OpenRAG style), not in | ||
| a separate error handler mapping (mandragora style).** | ||
| - Why: Existing code reads `exc.status_code` in multiple places. Switching | ||
| to a pure domain exception + API-layer mapping dict would require changing | ||
| every consumer now, which is unnecessary churn in Phase 1. | ||
| - Alternative considered: mandragora's pattern (bare exceptions in core/, | ||
| status code mapping in api/error_handlers.py). Cleaner for hexagonal | ||
| purity but rejected for backward compatibility. | ||
| - Follow-up: strip status codes from core exceptions in Phase 10 when | ||
| api/error_handlers.py is built. The error handler will own the mapping. | ||
|
|
||
| --- | ||
|
|
||
| ## Template for future entries | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Phase 1 section is inserted in the wrong place, creating a duplicate Template heading.
The pre-existing ## Template for future entries is still at line 58, the new Phase 1 content is dropped between it and its code fence, and a second ## Template for future entries is re-added at line 75. This both misplaces Phase 1 under the template heading and triggers markdownlint MD024. The Phase 1 block should go above the single template entry:
📝 Proposed fix
---
-## Template for future entries
-
-## Phase 1 — Registry + Exceptions (2026-04-21)
+## Phase 1 — Registry + Exceptions (2026-04-21)
**1. Exceptions keep HTTP status_code on the class (OpenRAG style), not in
a separate error handler mapping (mandragora style).**
...
- Follow-up: strip status codes from core exceptions in Phase 10 when
api/error_handlers.py is built. The error handler will own the mapping.
---
## Template for future entries
Phase N — [short title] ([YYYY-MM-DD])
...
📝 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
| ## Template for future entries | |
| ## Phase 1 — Registry + Exceptions (2026-04-21) | |
| **1. Exceptions keep HTTP status_code on the class (OpenRAG style), not in | |
| a separate error handler mapping (mandragora style).** | |
| - Why: Existing code reads `exc.status_code` in multiple places. Switching | |
| to a pure domain exception + API-layer mapping dict would require changing | |
| every consumer now, which is unnecessary churn in Phase 1. | |
| - Alternative considered: mandragora's pattern (bare exceptions in core/, | |
| status code mapping in api/error_handlers.py). Cleaner for hexagonal | |
| purity but rejected for backward compatibility. | |
| - Follow-up: strip status codes from core exceptions in Phase 10 when | |
| api/error_handlers.py is built. The error handler will own the mapping. | |
| --- | |
| ## Template for future entries | |
| ``` | |
| --- | |
| ## Phase 1 — Registry + Exceptions (2026-04-21) | |
| **1. Exceptions keep HTTP status_code on the class (OpenRAG style), not in | |
| a separate error handler mapping (mandragora style).** | |
| - Why: Existing code reads `exc.status_code` in multiple places. Switching | |
| to a pure domain exception + API-layer mapping dict would require changing | |
| every consumer now, which is unnecessary churn in Phase 1. | |
| - Alternative considered: mandragora's pattern (bare exceptions in core/, | |
| status code mapping in api/error_handlers.py). Cleaner for hexagonal | |
| purity but rejected for backward compatibility. | |
| - Follow-up: strip status codes from core exceptions in Phase 10 when | |
| api/error_handlers.py is built. The error handler will own the mapping. | |
| --- | |
| ## Template for future entries | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 75-75: Multiple headings with the same content
(MD024, no-duplicate-heading)
[warning] 77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@REFACTORING_DECISION_LOG.md` around lines 58 - 77, The Phase 1 block is
inserted inside and after the existing "## Template for future entries" header,
creating a duplicate template heading and MD024; move the entire "## Phase 1 —
Registry + Exceptions (2026-04-21)" section (including its content and the
trailing separator) so it appears above the single "## Template for future
entries" entry, remove the duplicate "## Template for future entries" that was
re-added, and ensure the template's closing code fence and formatting remain
intact; look for the headings "## Phase 1 — Registry + Exceptions (2026-04-21)"
and "## Template for future entries" to locate the blocks to reorder.
This was referenced May 6, 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
Phase 1 of the hexagonal refactoring. Purely additive — no existing behavior changed.
core/utils/registry.py— GenericRegistry[T]pattern with decorator-based registrationcore/utils/exceptions.py— Unified exception hierarchy (preserves existing OpenRAGError API + adds new categories)core/utils/text.py— Text sanitization (moved from components/indexer/utils/text_sanitizer.py)core/utils/filename.py— Filename sanitization (extracted from components/indexer/utils/files.py)core/utils/external_errors.py— External resource error detection (moved from utils/external_resource_errors.py)utils/exceptions/files re-export from core for backward compatibilitycore/catalog/(CatalogStore will live incore/ports/)Verification
python scripts/check_layer_imports.py→ OKSummary by CodeRabbit
New Features
Refactor