refactor(phase-5): core domain logic for retrieval, chunking, prompts - #352
Closed
EnjoyBacon7 wants to merge 13 commits into
Closed
refactor(phase-5): core domain logic for retrieval, chunking, prompts#352EnjoyBacon7 wants to merge 13 commits into
EnjoyBacon7 wants to merge 13 commits into
Conversation
📝 WalkthroughWalkthroughPhase 5 introduces core retrieval ports and pipeline, markdown-aware chunking and utilities, prompt-building modules and a template loader, storage shim to Ray/Milvus, migration of query domain models, many backwards-compatibility shims in components, configuration additions, extensive tests, and Phase‑5 documentation logs. ChangesPhase 5 — Core retrieval, chunking, prompts, adapters, and tests
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Pipeline as RetrieverPipeline
participant Retriever as Retriever
participant Searcher as RetrievalSearcher
participant Reranker as Reranker
participant Merge as RRF_Fusion
Client->>Pipeline: get_relevant_docs(search_queries, top_k)
Pipeline->>Retriever: retrieve(partition, query[i])
Retriever->>Searcher: search(query_str, filter)
Searcher-->>Retriever: chunks[]
alt No results + allow_filterless_fallback
Retriever->>Searcher: search(query_str, filter=None)
Searcher-->>Retriever: chunks[]
end
alt Reranking enabled
Retriever->>Reranker: rerank(query, chunk_texts)
Reranker-->>Retriever: ranked_chunks[]
end
alt Expansion enabled
Retriever->>Searcher: get_related_chunks(...)
Searcher-->>Retriever: related_chunks[]
Retriever->>Reranker: rerank(query, expanded_texts)
Reranker-->>Retriever: ranked_chunks[]
end
Retriever-->>Pipeline: ranked_chunks[]
Pipeline->>Merge: rrf_reranking([ranked_chunks...])
Merge-->>Pipeline: fused_chunks[]
Pipeline-->>Client: final_chunks[]
sequenceDiagram
participant Client as Client
participant Pipeline as RetrieverPipeline
participant MultiQuery as MultiQueryRetriever
participant LLM as LLM
participant Searcher as RetrievalSearcher
Client->>Pipeline: retrieve_docs(query, top_k)
Pipeline->>MultiQuery: retrieve(partition, query_text)
MultiQuery->>LLM: generate(multi_query_prompt)
LLM-->>MultiQuery: "q1 [SEP] q2 [SEP] q3"
MultiQuery->>MultiQuery: split_multi_query_response()
MultiQuery->>Searcher: multi_query_search([q1,q2,q3], top_k_per_query)
Searcher-->>MultiQuery: chunks[]
MultiQuery-->>Pipeline: chunks[]
Pipeline-->>Client: final_chunks[]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (3)
openrag/core/retrieval/searcher.py (2)
34-46: Parameterfiltershadows the built-in.Using
filteras a parameter name shadows Python's built-infilterfunction. Consider renaming tofilter_exprormilvus_filterfor clarity.♻️ Suggested rename
`@abstractmethod` async def search( self, query: str, partition: list[str], top_k: int, - filter: str | None = None, + filter_expr: str | None = None, filter_params: dict | None = None, similarity_threshold: float = 0.0, with_surrounding_chunks: bool = True, ) -> list[Chunk]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/core/retrieval/searcher.py` around lines 34 - 46, The search method in openrag.core.retrieval.searcher.Searcher uses a parameter named filter which shadows Python's built-in; rename that parameter to something like filter_expr (or milvus_filter) across the abstract method signature and all implementations/call sites (the async def search(..., filter: str | None = None, filter_params: dict | None = None, ... ) and any overrides) and update references inside the function bodies and callers to use the new name to avoid the built-in shadowing.
62-81: Document the intentional partition parameter difference across search methods.
searchandmulti_query_searchacceptpartition: list[str]for multi-partition queries, whileget_related_chunksandget_ancestor_chunksacceptpartition: strfor single-partition lookups. Add a brief docstring note to the relationship/ancestor methods explaining this difference to clarify the design for implementers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/core/retrieval/searcher.py` around lines 62 - 81, Add a one-line docstring note to get_related_chunks and get_ancestor_chunks clarifying that their partition parameter is a single partition string (partition: str) used for single-partition lookups, and that this differs from search and multi_query_search which accept partition: list[str] for multi-partition queries; reference the methods get_related_chunks, get_ancestor_chunks, search, and multi_query_search so implementers understand the intentional API difference.openrag/core/prompts/query_rewriter.py (1)
20-27: Validate required placeholders before formatting templates.Missing
{question}/{query}/{k_queries}currently fails silently and can produce broken prompts.Suggested patch
def build_hyde_prompt(template: str, query: str) -> str: """Format a HyDe prompt. ``template`` must contain ``{question}``.""" + if "{question}" not in template: + raise ValueError("HyDe template must contain '{question}'") return template.format(question=query) @@ def build_multi_query_prompt(template: str, query: str, k_queries: int) -> str: """Format a multi-query prompt. ``template`` must contain ``{query}`` and ``{k_queries}``.""" + if "{query}" not in template or "{k_queries}" not in template: + raise ValueError("Multi-query template must contain '{query}' and '{k_queries}'") return template.format(query=query, k_queries=k_queries)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/core/prompts/query_rewriter.py` around lines 20 - 27, The template formatting functions build_hyde_prompt and build_multi_query_prompt should validate that required placeholders exist before calling str.format to avoid silent failures; update build_hyde_prompt to assert or raise a ValueError if "{question}" is not in template, and update build_multi_query_prompt to similarly check for both "{query}" and "{k_queries}" in the provided template (and coerce k_queries to a string if you plan to format non-string values), providing clear error messages that mention the missing placeholder and the function name.
🤖 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/chunking/markdown_utils.py`:
- Around line 198-219: The loop currently flushes header-only subtables and
carries the entire group_text into the next chunk; fix by (1) only appending a
subtable when current_rows contains more than just header_text (i.e., skip
appending if current_rows == [header_text]) to avoid header-only chunks, and (2)
ensure prev_last_row stores/represents only the final row string of a group
(e.g., last_line = group_txt.splitlines()[-1]) and when starting a new chunk
append that last_row (prev_last_row) instead of the full group_txt and compute
current_size using length_function(prev_last_row); update assignments around
prev_last_row, current_rows and current_size accordingly in the loop that
iterates over group_texts/groups_ntoks.
In `@openrag/core/chunking/recursive.py`:
- Around line 115-120: _chunk_metadata_base currently allows document.metadata
to overwrite reserved keys "file_id" and "partition", which causes chunk()
(which reads metadata["file_id"] to set Chunk.document_id) to be reassigned
incorrectly; fix by making the reserved fields authoritative in
_chunk_metadata_base (either merge metadata first and then set "file_id" and
"partition" last, or explicitly filter out "file_id" and "partition" from
document.metadata before merging) so that the returned dict always uses
document.document_id and the provided partition for those keys.
- Around line 163-220: Currently text is concatenated then tables/images are
emitted separately which breaks original order; change the logic to iterate over
the ordered markdown elements (use split_md_elements or the output of
_prepare_md_elements in sequence) and build chunks in a single pass: buffer
inline text pieces and call split_text/sanitize when you need to flush (before
emitting a standalone table/image), emit oversized tables via chunk_table
immediately when encountered, use get_chunk_page_number and prev_page the same
way when emitting flushed text chunks, and finally remove the final chunks.sort
call so original source order is preserved; update usage of chunk_size and
length_function where needed.
- Around line 100-112: The page marker is being generated from last_page instead
of the current block's page, causing incorrect markers when pages jump or the
first block starts on a later page; in the loop that iterates over
document.text_blocks (using variables parts, last_page, block.page_number)
replace the marker append parts.append(f"[PAGE_{last_page}]") with one that uses
the incoming block's page (e.g., parts.append(f"[PAGE_{block.page_number}]")) so
markers reflect the actual block.page_number; keep the existing condition
(block.page_number is not None and last_page is not None and block.page_number
!= last_page) and then update last_page = block.page_number as before.
In `@openrag/core/models/query.py`:
- Around line 72-75: The code currently swallows parse failures for temporal
predicates by catching TypeError/ValueError around
datetime.fromisoformat(p.value) and doing continue; instead, validate and
propagate the error: replace the silent continue with a structured failure
(e.g., raise a ValueError or a domain-specific ValidationError containing the
offending predicate p and its value) or return an explicit validation result
upstream so the query is rejected rather than silently unconstrained; update the
function that iterates predicates (the block using p.value and
datetime.fromisoformat) to raise/return the validation error and ensure callers
handle that exception/result accordingly.
In `@openrag/core/prompts/contextualization_builder.py`:
- Around line 75-77: The current conditional uses truthiness of chunk_context so
whitespace-only strings still produce a [CONTEXT] block; change the check to
treat whitespace-only context as empty by using a stripped check (e.g., if
chunk_context and chunk_context.strip():) before choosing CHUNK_FORMAT versus
BASE_CHUNK_FORMAT so CHUNK_FORMAT is only used when chunk_context contains
non-whitespace content; update the branch that returns
CHUNK_FORMAT.format(content=..., chunk_context=..., filename=...) to only run
when the stripped check passes.
In `@openrag/core/prompts/map_reduce_builder.py`:
- Line 21: The system prompt in openrag/core/prompts/map_reduce_builder.py
contains a contradictory sentence ("If a document does have any relevant
content... classify it irrelevant"), so update that prompt string to invert the
condition: change "does have" to "doesn't have" (or rephrase to "If a document
does not have any relevant content with respect to a query, classify it as
irrelevant and do not provide a synthesis") so the logic matches intended
behavior; locate the prompt string in map_reduce_builder.py (the system prompt /
prompt builder used by the map-reduce flow) and make this single-line correction
and run any lint/tests to ensure no formatting issues.
In `@openrag/core/retrieval/pipeline.py`:
- Around line 118-127: The retrieve_docs flow currently uses top_k in a truthy
check and only to size the expansion head, so callers can still receive more
items; change the logic in the method (the block using self.expansion_enabled,
self.reranker_top_k, retriever.expand_search_results, and _rerank_chunks) to (1)
compute limit with an explicit None check (e.g., limit =
max(self.reranker_top_k, top_k) if top_k is not None else self.reranker_top_k)
so top_k=0 is honored, (2) after expansion and optional reranking always enforce
the requested top_k by trimming chunks = chunks[:top_k] when top_k is not None
(return [] if top_k == 0), and (3) keep reranking behavior unchanged but operate
on the trimmed/expanded head as input to _rerank_chunks.
In `@openrag/core/retrieval/retriever.py`:
- Around line 130-145: In _generate_queries/retrieve paths (e.g.,
MultiQueryRetriever._generate_queries, retrieve and HyDeRetriever equivalents),
validate and sanitize the model-generated queries before calling
searcher.multi_query_search: strip whitespace, drop empty or purely whitespace
items, deduplicate results, and truncate the list to at most self.k_queries; if
the final list is empty use the original seed query as a single fallback. Ensure
split_multi_query_response output is processed accordingly and that
multi_query_search is only invoked with the cleaned list to prevent fan-out
beyond k_queries.
- Around line 221-253: file_infos currently accumulates duplicate (partition,
document_id) tuples causing repeated get_ancestor_chunks() calls; change
file_infos to a set (or dedupe it before scheduling) so each (partition,
file_id) is fetched only once, then use that deduped collection when creating
tasks for _safe_ancestors; update any type hints from list[tuple[str,str]] to
set[tuple[str,str]] (or cast via set(file_infos)) and ensure tasks.extend uses
the deduped iterable when include_ancestors is true, leaving _safe_ancestors,
related_limit, and max_ancestor_depth unchanged.
In `@openrag/core/retrieval/rrf.py`:
- Around line 26-59: The function rrf_reranking currently allows negative k
which can make rank + k == 0 and cause a ZeroDivisionError; add an upfront
validation at the start of rrf_reranking that checks if k is negative and raises
a ValueError (e.g., "k must be non-negative") so callers get a clear error
instead of a runtime crash.
In `@openrag/core/retrieval/test_pipeline.py`:
- Around line 129-137: The test
test_get_relevant_docs_runs_one_call_per_subquery_and_fuses should explicitly
assert the retriever was invoked once per subquery: add a call-count check after
calling RetrieverPipeline.get_relevant_docs by using FakeRetriever's call
counter (e.g., ensure FakeRetriever increments a call_count field on each
invoke) or replace r with a simple spy/mock and assert r.call_count ==
len(sq.query_list) (== 2); keep existing result assertions unchanged and add the
new assert right after computing out.
In `@openrag/services/storage/milvus_ray_shim.py`:
- Around line 49-57: The four direct Ray actor awaits
(self._actor.async_search.remote, async_multi_query_search.remote,
get_related_chunks.remote, get_ancestor_chunks.remote) must be wrapped with
call_ray_actor_with_timeout(future, timeout, task_description) from
openrag.components.ray_utils to ensure proper timeout/cancellation; import
call_ray_actor_with_timeout at the top, replace each direct await of .remote()
by first creating the future (e.g., fut = self._actor.async_search.remote(...))
and then awaiting call_ray_actor_with_timeout(fut, self.actor_call_timeout,
"async_search") (use an appropriate timeout property or constant and a clear
task_description like "async_multi_query_search", "get_related_chunks",
"get_ancestor_chunks").
---
Nitpick comments:
In `@openrag/core/prompts/query_rewriter.py`:
- Around line 20-27: The template formatting functions build_hyde_prompt and
build_multi_query_prompt should validate that required placeholders exist before
calling str.format to avoid silent failures; update build_hyde_prompt to assert
or raise a ValueError if "{question}" is not in template, and update
build_multi_query_prompt to similarly check for both "{query}" and "{k_queries}"
in the provided template (and coerce k_queries to a string if you plan to format
non-string values), providing clear error messages that mention the missing
placeholder and the function name.
In `@openrag/core/retrieval/searcher.py`:
- Around line 34-46: The search method in
openrag.core.retrieval.searcher.Searcher uses a parameter named filter which
shadows Python's built-in; rename that parameter to something like filter_expr
(or milvus_filter) across the abstract method signature and all
implementations/call sites (the async def search(..., filter: str | None = None,
filter_params: dict | None = None, ... ) and any overrides) and update
references inside the function bodies and callers to use the new name to avoid
the built-in shadowing.
- Around line 62-81: Add a one-line docstring note to get_related_chunks and
get_ancestor_chunks clarifying that their partition parameter is a single
partition string (partition: str) used for single-partition lookups, and that
this differs from search and multi_query_search which accept partition:
list[str] for multi-partition queries; reference the methods get_related_chunks,
get_ancestor_chunks, search, and multi_query_search so implementers understand
the intentional API difference.
🪄 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: f21a0284-ded4-42ba-9b1b-fcecd33ce38b
📒 Files selected for processing (26)
FORWARD_PORT_LOG.mdREFACTORING_DECISION_LOG.mdopenrag/core/chunking/__init__.pyopenrag/core/chunking/markdown_utils.pyopenrag/core/chunking/recursive.pyopenrag/core/chunking/test_markdown_utils.pyopenrag/core/chunking/test_recursive.pyopenrag/core/models/__init__.pyopenrag/core/models/query.pyopenrag/core/prompts/__init__.pyopenrag/core/prompts/chat_prompt_builder.pyopenrag/core/prompts/contextualization_builder.pyopenrag/core/prompts/map_reduce_builder.pyopenrag/core/prompts/query_rewriter.pyopenrag/core/prompts/template_loader.pyopenrag/core/prompts/test_chat_prompt_builder.pyopenrag/core/prompts/vlm_prompt_builder.pyopenrag/core/retrieval/__init__.pyopenrag/core/retrieval/pipeline.pyopenrag/core/retrieval/retriever.pyopenrag/core/retrieval/rrf.pyopenrag/core/retrieval/searcher.pyopenrag/core/retrieval/test_pipeline.pyopenrag/core/retrieval/test_retriever.pyopenrag/core/retrieval/test_rrf.pyopenrag/services/storage/milvus_ray_shim.py
EnjoyBacon7
force-pushed
the
refactor/hexagonal
branch
from
April 29, 2026 14:48
5bb6bbc to
7ac00f6
Compare
Phase 5C of the hexagonal refactor — pure string-formatting helpers
extracted from components/utils.py, components/pipeline.py,
components/prompts/prompts.py, components/retriever.py,
components/map_reduce.py, and components/indexer/loaders/base.py.
* template_loader — disk reader, no config dep
* chat_prompt_builder — format_context, format_web_context,
prepend_system_prompt, SOURCE_SEPARATOR
* query_rewriter — HyDe + multi-query templating + [SEP] split
* contextualization_builder — chunk-context system+user message pair,
wrap_chunk_with_context envelope
* map_reduce_builder — map-step system + user templates
* vlm_prompt_builder — multimodal chat-message + caption wrapper
Tokenizers are injected as Callable[[str], int] so this layer stays pure.
Web-search results are typed by Protocol so core does not depend on
components/websearch. Test coverage on the chat builder locks in the
[Source N] wire format.
Phase 5B of the hexagonal refactor.
* markdown_utils — split_md_elements, get_chunk_page_number,
parse_markdown_table, chunk_table; MDElement is now a
dataclass.
* recursive — BaseChunker + RecursiveSplitter rewritten against the
ChunkingStrategy ABC. Tokenizer is injected as
length_function instead of a ChatOpenAI handle, so
the chunker no longer pulls in an LLM client.
Contextualization (the LLM-driven [CONTEXT] block) is
extracted out — that becomes core/indexing/contextualize
in 5D, applied as a separate stage by the orchestrator.
Output is list[Chunk] (domain model), not LangChain
Document. Registered as 'recursive_splitter' in
chunking_registry.
Tests mirror the legacy chunker tests so behavior is locked in.
…t (5A)
Phase 5A of the hexagonal refactor.
* core/retrieval/searcher.py — RetrievalSearcher port: a transitional
ABC for chunk-level retrieval ops the
new retriever needs but the narrow
VectorStore ABC does not cover (search
by query string, multi-query search,
related/ancestor lookup). Phase 7 will
decompose these onto VectorStore +
ChunkRepository.
* core/retrieval/retriever.py — BaseRetriever, SingleRetriever,
MultiQueryRetriever, HyDeRetriever
rewritten against the new ABCs. No
Ray, no LangChain chains, no global
config. Templates are passed in as
strings; LLM calls go through the LLM
ABC. Registered via retriever_registry.
* core/retrieval/pipeline.py — RetrieverPipeline extracted from
components/pipeline.py. Operates on
Chunks, fuses sub-queries via RRF.
Reranker is the new ABC (returns
indexed scores).
* core/retrieval/rrf.py — Reciprocal Rank Fusion as a generic
free function with a key_fn.
* core/models/query.py — Lift Query, SearchQueries,
TemporalPredicate from
components/pipeline.py.
* services/storage/milvus_ray_shim.py — RetrievalSearcher impl wrapping
the legacy Vectordb Ray actor.
LangChain Documents are converted to
domain Chunks at this boundary.
50 unit tests in core/ pass with no Ray, no Milvus, no real LLM. The
legacy 167-test suite is unaffected.
* REFACTORING_DECISION_LOG.md — Phase 5 entry: why RetrievalSearcher
port instead of forcing legacy
methods onto VectorStore; why
Query/SearchQueries went into
core/models; why integration tests
wait until Phase 8 wires the pipe.
* FORWARD_PORT_LOG.md — created (Mode 2 requirement). Empty
since this branch is freshly
branched from refactor/hexagonal.
* ruff fix + format — applied to the Phase 5 files.
… loader, map-reduce, VLM builders Adds unit-test files for the prompt builders that shipped without their own tests in the initial 5C extraction. Brings core/prompts/ from 85% to 100% line coverage on production modules.
Addresses CodeRabbit feedback on PR #352 plus a chunk-type bug surfaced during the test pass. All changes preserve legacy semantics; behavior changes deferred to phase 8. Bugs: - chunking/recursive: image elements above the inline threshold were emitted with chunk_type="image" and crashed ChunkType("image"); map to "image_caption". - chunking/recursive: document.metadata could overwrite reserved file_id and partition keys, silently reassigning chunks to the wrong document. - chunking/recursive: synthetic [PAGE_N] markers used last_page+1, so blocks starting on page>1 or skipping pages were mislabeled. Use block.page_number-1 instead. - chunking/markdown_utils.chunk_table: oversized first group emitted a header-only chunk; overlap replayed the whole previous group instead of its last row. Gate the flush on body content; replay only the trailing line. - retrieval/pipeline: top_k was not enforced on returned chunks (only used for expansion head sizing) and top_k=0 was treated as None. - retrieval/retriever: MultiQueryRetriever did not cap the LLM response at k_queries; HyDeRetriever forwarded blank/whitespace generations. - retrieval/retriever._expand_with_related_chunks: ancestor fetches were not deduped by (partition, document_id), fanning out duplicate calls. - retrieval/rrf: negative k could produce ZeroDivisionError; raise ValueError up front. - services/storage/milvus_ray_shim: Ray actor calls bypassed call_ray_actor_with_timeout, losing timeout/cancellation semantics the rest of the app relies on. Route every .remote() through the helper with a configurable timeout. - models/query.to_milvus_filter: legacy logged a warning before dropping invalid temporal predicates; the warning was lost in the move. Restored. Tests: - core/retrieval, core/chunking, core/prompts now at 100% line coverage on production modules. - New regressions for every fix above plus the BaseChunker lazy text_splitter init, multi-block document page-marker injection, and the markdown table parser's blank-row tolerance. prompts/contextualization_builder: treat whitespace-only chunk_context as empty so the [CONTEXT] envelope isn't emitted with no usable content. prompts/map_reduce_builder: fix contradictory "does have ... irrelevant" sentence in the system prompt.
EnjoyBacon7
force-pushed
the
refactor/phase-5-core-domain
branch
from
May 5, 2026 09:10
66a974e to
f36aa02
Compare
4 tasks
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openrag/core/prompts/test_vlm_prompt_builder.py (1)
18-19: ⚡ Quick winAdd
len(parts) == 2to pin the expected number of content parts.The set-equality check on line 19 only verifies that both
"image_url"and"text"are present, not that there are exactly two parts. An implementation that returns three parts — e.g., two"text"entries — still passes this assertion despite being malformed.✅ Proposed fix
parts = msg["content"] +assert len(parts) == 2 assert {p["type"] for p in parts} == {"image_url", "text"}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openrag/core/prompts/test_vlm_prompt_builder.py` around lines 18 - 19, The test currently checks only that the set of content part types equals {"image_url","text"} but not the count; modify the assertion to also enforce the exact number of parts by adding a check that len(parts) == 2 (i.e., after extracting parts = msg["content"] assert len(parts) == 2 and then keep the existing set-equality check) so the test ensures there are exactly two content entries rather than just those types appearing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openrag/services/storage/milvus_ray_shim.py`:
- Line 53: The import in openrag/services/storage/milvus_ray_shim.py currently
uses a non-root module path; change the import that brings in
call_ray_actor_with_timeout from "components.ray_utils" to the package-root
absolute path "openrag.components.ray_utils" so the module resolves correctly in
packaged/runtime contexts; locate the import line referencing
call_ray_actor_with_timeout and replace its module path accordingly.
---
Nitpick comments:
In `@openrag/core/prompts/test_vlm_prompt_builder.py`:
- Around line 18-19: The test currently checks only that the set of content part
types equals {"image_url","text"} but not the count; modify the assertion to
also enforce the exact number of parts by adding a check that len(parts) == 2
(i.e., after extracting parts = msg["content"] assert len(parts) == 2 and then
keep the existing set-equality check) so the test ensures there are exactly two
content entries rather than just those types appearing.
🪄 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: 280a209b-12fd-477c-8736-023172960664
📒 Files selected for processing (31)
FORWARD_PORT_LOG.mdREFACTORING_DECISION_LOG.mdopenrag/core/chunking/__init__.pyopenrag/core/chunking/markdown_utils.pyopenrag/core/chunking/recursive.pyopenrag/core/chunking/test_markdown_utils.pyopenrag/core/chunking/test_recursive.pyopenrag/core/models/__init__.pyopenrag/core/models/query.pyopenrag/core/prompts/__init__.pyopenrag/core/prompts/chat_prompt_builder.pyopenrag/core/prompts/contextualization_builder.pyopenrag/core/prompts/map_reduce_builder.pyopenrag/core/prompts/query_rewriter.pyopenrag/core/prompts/template_loader.pyopenrag/core/prompts/test_chat_prompt_builder.pyopenrag/core/prompts/test_contextualization_builder.pyopenrag/core/prompts/test_map_reduce_builder.pyopenrag/core/prompts/test_query_rewriter.pyopenrag/core/prompts/test_template_loader.pyopenrag/core/prompts/test_vlm_prompt_builder.pyopenrag/core/prompts/vlm_prompt_builder.pyopenrag/core/retrieval/__init__.pyopenrag/core/retrieval/pipeline.pyopenrag/core/retrieval/retriever.pyopenrag/core/retrieval/rrf.pyopenrag/core/retrieval/searcher.pyopenrag/core/retrieval/test_pipeline.pyopenrag/core/retrieval/test_retriever.pyopenrag/core/retrieval/test_rrf.pyopenrag/services/storage/milvus_ray_shim.py
✅ Files skipped from review due to trivial changes (7)
- FORWARD_PORT_LOG.md
- openrag/core/retrieval/init.py
- openrag/core/prompts/template_loader.py
- openrag/core/prompts/map_reduce_builder.py
- openrag/core/prompts/test_chat_prompt_builder.py
- openrag/core/chunking/test_markdown_utils.py
- REFACTORING_DECISION_LOG.md
🚧 Files skipped from review as they are similar to previous changes (15)
- openrag/core/retrieval/rrf.py
- openrag/core/retrieval/test_rrf.py
- openrag/core/models/init.py
- openrag/core/retrieval/test_pipeline.py
- openrag/core/prompts/vlm_prompt_builder.py
- openrag/core/retrieval/searcher.py
- openrag/core/chunking/init.py
- openrag/core/prompts/contextualization_builder.py
- openrag/core/retrieval/test_retriever.py
- openrag/core/retrieval/retriever.py
- openrag/core/prompts/chat_prompt_builder.py
- openrag/core/prompts/query_rewriter.py
- openrag/core/chunking/markdown_utils.py
- openrag/core/models/query.py
- openrag/core/retrieval/pipeline.py
…ms over core
STRATEGY \xc2\xa74.1 mandates a three-step move: create new file, update old file
to re-export from new, update consumers. Phase 5A/5B/5C only did step one,
leaving ~2000 lines of duplicate code in components/. This commit completes
step two — six legacy files now delegate to core/.
Files shimmed:
- components/indexer/chunker/utils.py plain re-export of core.chunking.markdown_utils
- components/prompts/prompts.py load_prompt -> core.prompts.template_loader.load_template_by_key
- components/utils.py format_context / format_web_context route through core.prompts.chat_prompt_builder
- components/indexer/chunker/chunker.py BaseChunker / RecursiveSplitter delegate to core.chunking.RecursiveSplitter
via Document<->ProcessedDocument<->Chunk conversion. ChunkContextualizer
and ChunkerFactory retained (5D + Phase 8).
- components/retriever.py Single/MultiQuery/HyDe retrievers wrap core strategies. Ray actor ->
MilvusRayShim, ChatOpenAI -> _LangChainLLMAdapter. RetrieverFactory retained.
- components/pipeline.py Query/SearchQueries/TemporalPredicate re-exported from core.models.query.
RetrieverPipeline delegates to core.retrieval.pipeline.RetrieverPipeline
via _LegacyRerankerAdapter (Document-in/out -> str-in / (idx, score)-out).
RagPipeline + RAGMODE retained (Phase 8).
Also benefits:
- The CodeRabbit fixes from PR #352 (image_caption ChunkType, page-marker
synthesis, chunk_table header-only flush + last-row overlap) now apply to
the legacy code path too — that path was previously running the buggy
versions even though core was patched.
- milvus_ray_shim's call_ray_actor_with_timeout now wraps every Ray retrieval
call from the legacy retrievers as well.
Side effect: a noqa side-effect import in chunker.py keeps the legacy
components.utils <-> components.indexer.utils.files circular import resolving
in the right order. Removed once components.utils is split in Phase 6+.
Decision log updated with the shim strategy and rationale.
Three normal-severity bugs flagged by the multi-agent review. 1. core/config: TranscriberConfig was missing direct_upload_suffixes field and its pipe-string validator, and core/config/loader.py was missing the matching TRANSCRIBER_DIRECT_UPLOAD_SUFFIXES env override mapping. With AUDIOLOADER=OpenAIAudioLoader, AudioTranscriber.__init__ would AttributeError on config.loader.transcriber.direct_upload_suffixes, re-introducing the failures the legacy 71f8aee/490a31b commits fixed. Ported the field, validator, and constants from the legacy schema. 2. core/chunking/recursive: _get_chunks built per-chunk dicts with {page_content, page, chunk_type, **metadata} — the spread came LAST, so a stray chunk_type/page/page_content key in document.metadata silently clobbered the resolved value. A bad chunk_type would crash ChunkType(...) downstream. Reversed the order in all three dict literals so reserved keys win, matching the pattern already used by _chunk_metadata_base. 3. core/models/chunk: Chunk.from_langchain called ChunkType(...) on the raw metadata value, which crashes on legacy data where chunk_type is the pre-Phase-5 raw MDElement literal 'image'. Every retrieval flow now goes through MilvusRayShim -> Chunk.from_langchain after 5.15, so any deployment indexed pre-PR would fail on the first image chunk. Added _coerce_chunk_type with a legacy alias map ('image' -> IMAGE_CAPTION) plus an unknown-value fallback to TEXT. Tests: +9 regression tests (TranscriberConfig pipe parsing, _get_chunks metadata-poison rejection, from_langchain legacy aliases). Full suite 448 passed / 4 skipped. The fourth review finding (AudioTranscriber UnboundLocalError on ffmpeg/pydub failure) was classified pre-existing by the review and deferred — out of phase-5 scope.
The Milvus collection schema declares the primary key `_id` as INT64 with auto_id=True, so the value comes back from the Ray actor as a Python int. Chunk.id is typed `str`, which made every retrieval call fail with a Pydantic ValidationError once the new core retriever shim landed (8 api-tests failed: chat completions, search with include_related, source filtering). Coerce at the conversion boundary rather than loosen the domain model so the store-specific shape stays contained.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
openrag/core/config/test_indexation.py (1)
1-31: ⚡ Quick winAdd a test for list input (YAML sequence scenario).
The four existing tests cover defaults, pipe-string parsing, empty-component dropping, and set pass-through, but there is no test for a list input (the path taken when YAML deserializes
direct_upload_suffixes: [wav, flac]). Once the validator is fixed to normalize collection inputs, the following test locks in that behavior:✅ Suggested additional test
def test_transcriber_config_normalizes_list_input(): """YAML sequences arrive as lists; items must be dot-prefixed and lowercased.""" cfg = TranscriberConfig(direct_upload_suffixes=["wav", "FLAC", ".mp3"]) assert cfg.direct_upload_suffixes == {".wav", ".flac", ".mp3"}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openrag/core/config/test_indexation.py` around lines 1 - 31, Add a test that verifies TranscriberConfig normalizes YAML-sequence (list) inputs: create a new test function named test_transcriber_config_normalizes_list_input in openrag/core/config/test_indexation.py that constructs TranscriberConfig(direct_upload_suffixes=["wav", "FLAC", ".mp3"]) and asserts cfg.direct_upload_suffixes == {".wav", ".flac", ".mp3"}; if the validator in TranscriberConfig currently only handles pipe-strings, update its validation logic for the direct_upload_suffixes field to accept iterable inputs (list/tuple), iterate items, drop empty components, lower-case them and ensure each item is dot-prefixed before building the resulting set.openrag/core/config/indexation.py (1)
39-44: ⚡ Quick win
_split_suffixessilently drops normalization for list/frozenset inputs (YAML scenario).When a YAML config supplies
direct_upload_suffixesas a sequence (e.g.,[wav, flac]), the validator returns it unchanged. Pydantic then coerces it to{"wav", "flac"}— no leading dot. BecauseAudioTranscriber.transcribetestsfile_path.suffix.lower()(which always has a leading dot, e.g.,.mp3) against this set, those entries never match, silently falling back to WAV conversion regardless of the user's intent.🛠️ Proposed fix — normalize collection inputs the same way strings are
`@field_validator`("direct_upload_suffixes", mode="before") `@classmethod` def _split_suffixes(cls, v: Any) -> Any: if isinstance(v, str): return {n for raw in v.split("|") if (n := _normalize_suffix(raw))} + if isinstance(v, (list, tuple, frozenset, set)): + return {n for raw in v if isinstance(raw, str) and (n := _normalize_suffix(raw))} return v🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openrag/core/config/indexation.py` around lines 39 - 44, The _split_suffixes validator currently only normalizes when the input v is a string, so sequence inputs (list/tuple/set/frozenset) from YAML are returned unchanged and miss the leading-dot normalization used by _normalize_suffix; update _split_suffixes (the field validator for direct_upload_suffixes) to detect iterable/sequence types and return a set comprehension applying _normalize_suffix to each element (filtering falsy results), so both string and collection inputs are normalized consistently and will match AudioTranscriber.transcribe's file_path.suffix checks.openrag/components/retriever.py (2)
154-179: 💤 Low valueOptional: cache the core retriever instance instead of rebuilding per call.
_build_core_retriever()and_searcher()are invoked on everyretrieve()andexpand_search_results()call, each constructing a newMilvusRayShimand a new core*Retriever. The Ray actor handle lookup behindget_vectordb()is cheap but not free, and the shim/retriever are otherwise stateless. If the lazy-build motivation is just "actor must exist by first request," caching after the first build keeps that property without per-call churn. Skip if there's a deliberate reason to keep them stateless across calls.♻️ Suggested change
self._core_kwargs = self._build_core_kwargs(kwargs) + self._core_retriever = None def _build_core_kwargs(self, extra: dict[str, Any]) -> dict[str, Any]: ... def _build_core_retriever(self): """Late-bind the core retriever so the Ray actor is only resolved on use.""" - return self._CORE_CLS(searcher=_searcher(), **self._core_kwargs) + if self._core_retriever is None: + self._core_retriever = self._CORE_CLS(searcher=_searcher(), **self._core_kwargs) + return self._core_retriever🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openrag/components/retriever.py` around lines 154 - 179, The code currently calls _build_core_retriever() and _searcher() on every retrieve() and expand_search_results() call, rebuilding a MilvusRayShim and core *Retriever each time; change _build_core_retriever to cache the created instance (e.g., store it on self like self._core_retriever) and return the cached object on subsequent calls, and update any places that call _searcher() if you also want to cache the shim (e.g., create and reuse a single MilvusRayShim instance tied to the cached core retriever), ensuring the lazy/binding behavior remains (build on first request) but avoids per-call reconstruction in retrieve() and expand_search_results().
56-68: 💤 Low valueAdd defensive type check in
chat()to matchgenerate()'s approach.
out.contentinchat()(line 68) is typed asstr | list[str | dict]onlangchain-coreBaseMessage, but the method declares-> str. WhileChatOpenAIwith standard configuration returns string content today, upgrading to vision or multimodal models would expose a type contract violation.generate()already guards withstr(out)fallback (line 63);chat()should mirror this defensively:async def chat(self, messages: list[dict[str, str]], **kwargs) -> str: lc_msgs = [self._ROLE_MAP[m["role"]](content=m["content"]) for m in messages] out = await self._llm.ainvoke(lc_msgs) - return out.content + return out.content if isinstance(out.content, str) else str(out.content)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openrag/components/retriever.py` around lines 56 - 68, The chat() method should defensively handle non-string content like generate() does: in retriever.py update the chat(self, ...) implementation (which builds lc_msgs using _ROLE_MAP and calls self._llm.ainvoke) to check if the returned out has a "content" attribute and return out.content when it's a string, otherwise return str(out) as a fallback; mirror the same hasattr(out, "content") -> out.content else str(out) pattern used in generate() to avoid type-contract violations with multimodal/vision models.openrag/core/chunking/recursive.py (1)
75-86: 💤 Low valueOptional: drop
file_id/partitionfrom per-chunk metadata to avoid duplication with first-class fields.
metadatahere still carriesfile_idandpartitionfrom_chunk_metadata_base, while those values are also stored onChunk.document_id/Chunk.partition. The duplication is harmless today but invites drift if someone updates one but not the other in a downstream stage.♻️ Suggested cleanup
- metadata={k: v for k, v in md_chunks_meta.items() if k not in ("page_content", "chunk_type", "page")}, + metadata={ + k: v + for k, v in md_chunks_meta.items() + if k not in ("page_content", "chunk_type", "page", "file_id", "partition") + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openrag/core/chunking/recursive.py` around lines 75 - 86, The per-chunk metadata currently includes keys that are already stored as first-class fields (file_id -> Chunk.document_id and partition -> Chunk.partition); update the list comprehension that builds Chunk(...) in recursive.py so the metadata dict comprehension excludes "file_id" and "partition" (i.e., filter out those keys from md_chunks_meta when constructing metadata for each Chunk) while keeping document_id and partition populated as before using metadata.get("file_id", "") and the partition variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openrag/core/models/chunk.py`:
- Around line 33-37: The comment in _coerce_chunk_type is misleading because no
logging happens; update the except ValueError block to log the offending value
before falling back to ChunkType.TEXT. Specifically, in the _coerce_chunk_type
function add a logger.warning (or logger.debug) call that includes repr(value)
and a short context message, then return ChunkType.TEXT as currently done;
ensure you reference the module-level logger (or import one) so the warning is
actually emitted instead of just leaving the comment.
---
Nitpick comments:
In `@openrag/components/retriever.py`:
- Around line 154-179: The code currently calls _build_core_retriever() and
_searcher() on every retrieve() and expand_search_results() call, rebuilding a
MilvusRayShim and core *Retriever each time; change _build_core_retriever to
cache the created instance (e.g., store it on self like self._core_retriever)
and return the cached object on subsequent calls, and update any places that
call _searcher() if you also want to cache the shim (e.g., create and reuse a
single MilvusRayShim instance tied to the cached core retriever), ensuring the
lazy/binding behavior remains (build on first request) but avoids per-call
reconstruction in retrieve() and expand_search_results().
- Around line 56-68: The chat() method should defensively handle non-string
content like generate() does: in retriever.py update the chat(self, ...)
implementation (which builds lc_msgs using _ROLE_MAP and calls
self._llm.ainvoke) to check if the returned out has a "content" attribute and
return out.content when it's a string, otherwise return str(out) as a fallback;
mirror the same hasattr(out, "content") -> out.content else str(out) pattern
used in generate() to avoid type-contract violations with multimodal/vision
models.
In `@openrag/core/chunking/recursive.py`:
- Around line 75-86: The per-chunk metadata currently includes keys that are
already stored as first-class fields (file_id -> Chunk.document_id and partition
-> Chunk.partition); update the list comprehension that builds Chunk(...) in
recursive.py so the metadata dict comprehension excludes "file_id" and
"partition" (i.e., filter out those keys from md_chunks_meta when constructing
metadata for each Chunk) while keeping document_id and partition populated as
before using metadata.get("file_id", "") and the partition variable.
In `@openrag/core/config/indexation.py`:
- Around line 39-44: The _split_suffixes validator currently only normalizes
when the input v is a string, so sequence inputs (list/tuple/set/frozenset) from
YAML are returned unchanged and miss the leading-dot normalization used by
_normalize_suffix; update _split_suffixes (the field validator for
direct_upload_suffixes) to detect iterable/sequence types and return a set
comprehension applying _normalize_suffix to each element (filtering falsy
results), so both string and collection inputs are normalized consistently and
will match AudioTranscriber.transcribe's file_path.suffix checks.
In `@openrag/core/config/test_indexation.py`:
- Around line 1-31: Add a test that verifies TranscriberConfig normalizes
YAML-sequence (list) inputs: create a new test function named
test_transcriber_config_normalizes_list_input in
openrag/core/config/test_indexation.py that constructs
TranscriberConfig(direct_upload_suffixes=["wav", "FLAC", ".mp3"]) and asserts
cfg.direct_upload_suffixes == {".wav", ".flac", ".mp3"}; if the validator in
TranscriberConfig currently only handles pipe-strings, update its validation
logic for the direct_upload_suffixes field to accept iterable inputs
(list/tuple), iterate items, drop empty components, lower-case them and ensure
each item is dot-prefixed before building the resulting set.
🪄 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: e713814e-9ad8-4fef-919f-b231df5867ce
📒 Files selected for processing (14)
REFACTORING_DECISION_LOG.mdopenrag/components/indexer/chunker/chunker.pyopenrag/components/indexer/chunker/utils.pyopenrag/components/pipeline.pyopenrag/components/prompts/prompts.pyopenrag/components/retriever.pyopenrag/components/utils.pyopenrag/core/chunking/recursive.pyopenrag/core/chunking/test_recursive.pyopenrag/core/config/indexation.pyopenrag/core/config/loader.pyopenrag/core/config/test_indexation.pyopenrag/core/models/chunk.pyopenrag/core/models/test_chunk.py
✅ Files skipped from review due to trivial changes (2)
- openrag/core/models/test_chunk.py
- REFACTORING_DECISION_LOG.md
Collaborator
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Ahmath-Gadji
reviewed
May 6, 2026
|
|
||
| from typing import Any | ||
|
|
||
| IMAGE_DESCRIPTION_OPEN = "<image_description>" |
Collaborator
There was a problem hiding this comment.
I added a new file at openrag/core/utils/conts.py that defines constants like these. Should be used once PR #354 integrates these changes
Collaborator
There was a problem hiding this comment.
Do not take this into account this comment. In concerns post-rebased #354 with this branch
Ahmath-Gadji
reviewed
May 6, 2026
Collaborator
There was a problem hiding this comment.
Utils to be used in core/indexing/contextualize.py
Collaborator
There was a problem hiding this comment.
Do not take this into account this comment. In concerns post-rebased #354 with this branch
Ahmath-Gadji
reviewed
May 6, 2026
Comment on lines
+272
to
+278
| # --------------------------------------------------------------------------- | ||
| # Registry — config-driven factory replacement | ||
| # --------------------------------------------------------------------------- | ||
| retriever_registry: Registry[Retriever] = Registry("retriever") | ||
| retriever_registry.register("single")(SingleRetriever) | ||
| retriever_registry.register("multiQuery")(MultiQueryRetriever) | ||
| retriever_registry.register("hyde")(HyDeRetriever) |
Collaborator
There was a problem hiding this comment.
Just a one thing. For registering, the doc suggests this following format
- create core/retrieval/registry.py where retriever_registry is initialized
- Then import it and register via decorator style.
from ..registry import retriever_registry
@retriever_registry.register("single")
class SingleRetriever(BaseRetriever):
Ahmath-Gadji
reviewed
May 6, 2026
Mirrors the chunking pattern: registry instance lives in core/retrieval/registry.py and concrete strategies register themselves via @retriever_registry.register(...) decorator at class definition, instead of the imperative block at the bottom of retriever.py. Addresses Ahmath's review on PR #352.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openrag/core/models/chunk.py`:
- Around line 26-36: The membership check against _CHUNK_TYPE_LEGACY_ALIASES in
_coerce_chunk_type can raise TypeError for unhashable values (lists/dicts);
guard that lookup by only checking aliases when value is a str (e.g., add an
isinstance(value, str) condition before "if value in
_CHUNK_TYPE_LEGACY_ALIASES") or move the alias lookup inside the try/except so
non-hashable inputs fall through to the except and return ChunkType.TEXT.
In `@openrag/core/retrieval/retriever.py`:
- Around line 242-257: Both helpers (_safe_related and _safe_ancestors)
currently swallow exceptions and return [] without logging; update them to
import get_logger from utils.logger, create a bound logger (including partition
and rel_id or file_id as context), and emit a single structured warning on
exception that includes the bound context and the exception details (e.g.,
logger.bind(partition=part, file_id=file_id).warning(..., exc=exc) or similar)
before returning []; keep the existing exception swallowing behavior after
logging.
🪄 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: 25f995d9-30d8-4685-8ac5-3de7baf0a8ee
📒 Files selected for processing (7)
REFACTORING_DECISION_LOG.mdopenrag/core/models/chunk.pyopenrag/core/retrieval/__init__.pyopenrag/core/retrieval/registry.pyopenrag/core/retrieval/retriever.pyopenrag/core/retrieval/test_retriever.pyopenrag/services/storage/milvus_ray_shim.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openrag/core/retrieval/test_retriever.py
Comment on lines
+26
to
+36
| def _coerce_chunk_type(value: Any) -> ChunkType: | ||
| if isinstance(value, ChunkType): | ||
| return value | ||
| if value in _CHUNK_TYPE_LEGACY_ALIASES: | ||
| return _CHUNK_TYPE_LEGACY_ALIASES[value] | ||
| try: | ||
| return ChunkType(value) | ||
| except (ValueError, TypeError): | ||
| # Unknown value from upstream/legacy data — fall back to TEXT rather | ||
| # than crash the retrieval call. | ||
| return ChunkType.TEXT |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Current helper:"
sed -n '26,36p' openrag/core/models/chunk.py
echo
echo "Existing tests touching chunk-type coercion:"
rg -n "_coerce_chunk_type|chunk_type" openrag/core/models/test_chunk.py || true
echo
python - <<'PY'
aliases = {"image": "image_caption"}
for value in (["image"], {"kind": "image"}):
try:
_ = value in aliases
print("membership unexpectedly succeeded")
except Exception as exc:
print(f"{type(exc).__name__}: {exc}")
PYRepository: linagora/openrag
Length of output: 1472
Guard the legacy alias lookup against unhashable metadata values.
The if value in _CHUNK_TYPE_LEGACY_ALIASES check at line 29 executes before the exception handler, so a corrupted stored chunk_type like a list or dict raises TypeError and crashes retrieval instead of degrading to ChunkType.TEXT. Add isinstance(value, str) guard before the alias check, or move it inside the try/except block.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openrag/core/models/chunk.py` around lines 26 - 36, The membership check
against _CHUNK_TYPE_LEGACY_ALIASES in _coerce_chunk_type can raise TypeError for
unhashable values (lists/dicts); guard that lookup by only checking aliases when
value is a str (e.g., add an isinstance(value, str) condition before "if value
in _CHUNK_TYPE_LEGACY_ALIASES") or move the alias lookup inside the try/except
so non-hashable inputs fall through to the except and return ChunkType.TEXT.
Comment on lines
+242
to
+257
| async def _safe_related(part: str, rel_id: str) -> list[Chunk]: | ||
| try: | ||
| return await searcher.get_related_chunks(partition=part, relationship_id=rel_id, limit=related_limit) | ||
| except Exception: | ||
| return [] | ||
|
|
||
| async def _safe_ancestors(part: str, file_id: str) -> list[Chunk]: | ||
| try: | ||
| return await searcher.get_ancestor_chunks( | ||
| partition=part, | ||
| file_id=file_id, | ||
| limit=related_limit, | ||
| max_ancestor_depth=max_ancestor_depth, | ||
| ) | ||
| except Exception: | ||
| return [] |
There was a problem hiding this comment.
Log expansion failures before falling back.
Both helpers silently return [] on every exception, so related/ancestor enrichment can disappear with no signal even though the docstring says these failures are logged. Please emit one structured warning per failed lookup before swallowing it.
As per coding guidelines **/*.py: "Use Loguru structured logging via from utils.logger import get_logger with binding for context variables like file_id and partition".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openrag/core/retrieval/retriever.py` around lines 242 - 257, Both helpers
(_safe_related and _safe_ancestors) currently swallow exceptions and return []
without logging; update them to import get_logger from utils.logger, create a
bound logger (including partition and rel_id or file_id as context), and emit a
single structured warning on exception that includes the bound context and the
exception details (e.g., logger.bind(partition=part,
file_id=file_id).warning(..., exc=exc) or similar) before returning []; keep the
existing exception swallowing behavior after logging.
4 tasks
EnjoyBacon7
added a commit
that referenced
this pull request
May 7, 2026
…ms over core
STRATEGY \xc2\xa74.1 mandates a three-step move: create new file, update old file
to re-export from new, update consumers. Phase 5A/5B/5C only did step one,
leaving ~2000 lines of duplicate code in components/. This commit completes
step two — six legacy files now delegate to core/.
Files shimmed:
- components/indexer/chunker/utils.py plain re-export of core.chunking.markdown_utils
- components/prompts/prompts.py load_prompt -> core.prompts.template_loader.load_template_by_key
- components/utils.py format_context / format_web_context route through core.prompts.chat_prompt_builder
- components/indexer/chunker/chunker.py BaseChunker / RecursiveSplitter delegate to core.chunking.RecursiveSplitter
via Document<->ProcessedDocument<->Chunk conversion. ChunkContextualizer
and ChunkerFactory retained (5D + Phase 8).
- components/retriever.py Single/MultiQuery/HyDe retrievers wrap core strategies. Ray actor ->
MilvusRayShim, ChatOpenAI -> _LangChainLLMAdapter. RetrieverFactory retained.
- components/pipeline.py Query/SearchQueries/TemporalPredicate re-exported from core.models.query.
RetrieverPipeline delegates to core.retrieval.pipeline.RetrieverPipeline
via _LegacyRerankerAdapter (Document-in/out -> str-in / (idx, score)-out).
RagPipeline + RAGMODE retained (Phase 8).
Also benefits:
- The CodeRabbit fixes from PR #352 (image_caption ChunkType, page-marker
synthesis, chunk_table header-only flush + last-row overlap) now apply to
the legacy code path too — that path was previously running the buggy
versions even though core was patched.
- milvus_ray_shim's call_ray_actor_with_timeout now wraps every Ray retrieval
call from the legacy retrievers as well.
Side effect: a noqa side-effect import in chunker.py keeps the legacy
components.utils <-> components.indexer.utils.files circular import resolving
in the right order. Removed once components.utils is split in Phase 6+.
Decision log updated with the shim strategy and rationale.
EnjoyBacon7
added a commit
that referenced
this pull request
May 7, 2026
…ms over core
STRATEGY \xc2\xa74.1 mandates a three-step move: create new file, update old file
to re-export from new, update consumers. Phase 5A/5B/5C only did step one,
leaving ~2000 lines of duplicate code in components/. This commit completes
step two — six legacy files now delegate to core/.
Files shimmed:
- components/indexer/chunker/utils.py plain re-export of core.chunking.markdown_utils
- components/prompts/prompts.py load_prompt -> core.prompts.template_loader.load_template_by_key
- components/utils.py format_context / format_web_context route through core.prompts.chat_prompt_builder
- components/indexer/chunker/chunker.py BaseChunker / RecursiveSplitter delegate to core.chunking.RecursiveSplitter
via Document<->ProcessedDocument<->Chunk conversion. ChunkContextualizer
and ChunkerFactory retained (5D + Phase 8).
- components/retriever.py Single/MultiQuery/HyDe retrievers wrap core strategies. Ray actor ->
MilvusRayShim, ChatOpenAI -> _LangChainLLMAdapter. RetrieverFactory retained.
- components/pipeline.py Query/SearchQueries/TemporalPredicate re-exported from core.models.query.
RetrieverPipeline delegates to core.retrieval.pipeline.RetrieverPipeline
via _LegacyRerankerAdapter (Document-in/out -> str-in / (idx, score)-out).
RagPipeline + RAGMODE retained (Phase 8).
Also benefits:
- The CodeRabbit fixes from PR #352 (image_caption ChunkType, page-marker
synthesis, chunk_table header-only flush + last-row overlap) now apply to
the legacy code path too — that path was previously running the buggy
versions even though core was patched.
- milvus_ray_shim's call_ray_actor_with_timeout now wraps every Ray retrieval
call from the legacy retrievers as well.
Side effect: a noqa side-effect import in chunker.py keeps the legacy
components.utils <-> components.indexer.utils.files circular import resolving
in the right order. Removed once components.utils is split in Phase 6+.
Decision log updated with the shim strategy and rationale.
EnjoyBacon7
added a commit
that referenced
this pull request
May 26, 2026
…ms over core
STRATEGY \xc2\xa74.1 mandates a three-step move: create new file, update old file
to re-export from new, update consumers. Phase 5A/5B/5C only did step one,
leaving ~2000 lines of duplicate code in components/. This commit completes
step two — six legacy files now delegate to core/.
Files shimmed:
- components/indexer/chunker/utils.py plain re-export of core.chunking.markdown_utils
- components/prompts/prompts.py load_prompt -> core.prompts.template_loader.load_template_by_key
- components/utils.py format_context / format_web_context route through core.prompts.chat_prompt_builder
- components/indexer/chunker/chunker.py BaseChunker / RecursiveSplitter delegate to core.chunking.RecursiveSplitter
via Document<->ProcessedDocument<->Chunk conversion. ChunkContextualizer
and ChunkerFactory retained (5D + Phase 8).
- components/retriever.py Single/MultiQuery/HyDe retrievers wrap core strategies. Ray actor ->
MilvusRayShim, ChatOpenAI -> _LangChainLLMAdapter. RetrieverFactory retained.
- components/pipeline.py Query/SearchQueries/TemporalPredicate re-exported from core.models.query.
RetrieverPipeline delegates to core.retrieval.pipeline.RetrieverPipeline
via _LegacyRerankerAdapter (Document-in/out -> str-in / (idx, score)-out).
RagPipeline + RAGMODE retained (Phase 8).
Also benefits:
- The CodeRabbit fixes from PR #352 (image_caption ChunkType, page-marker
synthesis, chunk_table header-only flush + last-row overlap) now apply to
the legacy code path too — that path was previously running the buggy
versions even though core was patched.
- milvus_ray_shim's call_ray_actor_with_timeout now wraps every Ray retrieval
call from the legacy retrievers as well.
Side effect: a noqa side-effect import in chunker.py keeps the legacy
components.utils <-> components.indexer.utils.files circular import resolving
in the right order. Removed once components.utils is split in Phase 6+.
Decision log updated with the shim strategy and rationale.
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
2 participants
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 5A + 5B + 5C of the hexagonal refactor — moves pure business logic into
core/. First phase of MODE 2 (ISOLATE).core/prompts/(6 builders + disk template loader)template_loader(no config dep),chat_prompt_builder(format_context,format_web_context,prepend_system_prompt,SOURCE_SEPARATOR),query_rewriter(HyDe + multi-query +[SEP]split),contextualization_builder(chunk-context messages +[CONTEXT]envelope),map_reduce_builder,vlm_prompt_builder.Callable[[str], int]. Web sources typed byProtocol. No LLM client / LangChain in core.core/chunking/markdown_utils—MDElement(now a dataclass),split_md_elements,get_chunk_page_number,chunk_table,parse_markdown_table.recursive—BaseChunker+RecursiveSplitterregistered asrecursive_splitterinchunking_registry. Produceslist[Chunk]against theChunkingStrategyABC. Tokenizer is injected (noChatOpenAI). Contextualization is excluded — moves tocore/indexing/contextualize.pyin 5D.core/retrieval/searcher—RetrievalSearcherABC (transitional port). The narrow Phase-4VectorStoreABC doesn't fit the legacy methods (search by query string, multi-query, related/ancestor lookup); decision logged inREFACTORING_DECISION_LOG.md.retriever—BaseRetriever,SingleRetriever,MultiQueryRetriever,HyDeRetrieverrewritten against the new ABCs. Noget_vectordb(), noChatOpenAI, no LangChain chains. Templates passed in as strings; LLM calls go through theLLMABC. Registered viaretriever_registry.pipeline—RetrieverPipelineextracted fromcomponents/pipeline.py. Operates on Chunks; fuses sub-queries via RRF; supports the temporal-filter fallback knob.rrf— RRF as a generic free function withkey_fn.core/models/query.py— LiftedQuery,SearchQueries,TemporalPredicatefromcomponents/pipeline.py.services/storage/milvus_ray_shim.py—RetrievalSearcherimpl wrapping the Vectordb Ray actor. LangChain Documents convert to Chunks at this boundary; the new core retriever has zero Ray imports.REFACTORING_DECISION_LOG.md, createdFORWARD_PORT_LOG.md.Strangler Fig stage
This PR adds new code only. No legacy file is modified or shimmed in this PR — the new pipeline is dormant until Phase 8 wires it. The legacy
components/pipeline.py:RetrieverPipelinekeeps running unchanged. Re-export shims (5E) are deferred until Phase 8 has hooks ready.Layer compliance
scripts/check_layer_imports.pypasses.Test plan
uv run ruff check openrag/cleanuv run ruff format --check openrag/cleanpython scripts/check_layer_imports.pypassesOut of scope (followups)
core/indexing/contextualize.pySummary by CodeRabbit