allowing scope retrieval via a list of file_ids - #761
Merged
Ahmath-Gadji merged 17 commits intoAug 25, 2026
Conversation
…at/completions, used a lot of the workspace filter mechanism, adding a lists of the file_id effectively indexed on the extra.attachments, in order to know which one of the file_ids' lists were not indexed in RAG
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughQueryService now resolves indexed attachment IDs within requested partition scopes, applies them to retrieval, and exposes the resolved IDs in synchronous and streaming response metadata. Workspace repositories support cross-partition existence checks, and source filtering accepts additional payload fields. ChangesCozy attachment support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant QueryService
participant WorkspaceService
participant WorkspaceRepository
participant Retriever
Client->>QueryService: Send chat request with attachments
QueryService->>WorkspaceService: Resolve indexed attachment IDs
WorkspaceService->>WorkspaceRepository: Query existing file IDs
WorkspaceRepository-->>WorkspaceService: Return indexed IDs
WorkspaceService-->>QueryService: Return resolved IDs
QueryService->>Retriever: Retrieve with file_id filter
Retriever-->>QueryService: Return chunks
QueryService-->>Client: Return response or stream with attachments
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 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/services/orchestrators/query_service.py`:
- Around line 704-707: The attachment extraction logic in query_service.py
(around metadata attachments) must treat malformed payloads as unscoped
retrieval: require attachments to be a list and include only entries whose id is
a non-empty string before passing IDs to _existing_file_ids(). Add unit coverage
in tests/unit/services/orchestrators/test_query_service.py at lines 546-562 for
scalar attachment payloads and non-string IDs, asserting retrieval proceeds
without an exception and remains unscoped.
- Around line 507-509: Update the attachment-ID resolution logic around the
concrete partition calculation in
openrag/services/orchestrators/query_service.py:507-509 to expand "all" into the
concrete partitions used by retrieval, look up each partition’s existing IDs,
and union them before building the filter and response metadata; do not return
every requested ID directly. Add an "all" test case in
tests/unit/services/orchestrators/test_query_service.py:619-660 asserting
nonexistent IDs are excluded from both the retrieval filter and
extra.attachments.
🪄 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 Plus
Run ID: 8c89373f-05f9-4c3f-ac35-e13a7daa323e
📒 Files selected for processing (3)
openrag/core/utils/source_filtering.pyopenrag/services/orchestrators/query_service.pytests/unit/services/orchestrators/test_query_service.py
paultranvan
reviewed
Jul 23, 2026
PgWorkspaceRepository implemented it and WorkspaceService (typed against WorkspaceRepository) already called it, but it was missing from the abstract interface.
Collaborator
|
@CodeRabbit review |
✅ Action performedReview finished.
|
…at/completions, used a lot of the workspace filter mechanism, adding a lists of the file_id effectively indexed on the extra.attachments, in order to know which one of the file_ids' lists were not indexed in RAG
PgWorkspaceRepository implemented it and WorkspaceService (typed against WorkspaceRepository) already called it, but it was missing from the abstract interface.
The comment justified skipping an ownership check on the attachment ids by saying file_id is ANDed with the server-fixed partition. That stopped being the whole story when the "all" wildcard switched to an unscoped catalog lookup: on that path there is no partition to AND against, and the safety argument is the SUPER_ADMIN_MODE invariant instead. Spell both out, since the comment exists to justify the absence of a check.
Wording requested in review. It is also the accurate word: the list reports what retrieval was scoped to, not what the answer drew on — that is extra.sources.
…tgres get_existing_file_ids_any_partition backs attachment scoping for the openrag-all wildcard, but every test reaching it went through a fake, so its SQL was never executed anywhere in CI. Same for the partition-scoped sibling. Both are now exercised by the repos suite, which runs against a real database on every PR. The two scoping cases are mutation-checked: dropping the partition clause from the scoped query, or adding one to the unscoped query, each fails a test rather than passing quietly.
The list is client-supplied, so repeats would otherwise be echoed into the retrieval filter and back out through extra.attachments. The dedupe is in the code; the docstring still described a plain subset.
Ahmath-Gadji
force-pushed
the
feat/chat-attachments-filter
branch
from
July 29, 2026 10:07
f104591 to
87bb7b0
Compare
EnjoyBacon7
previously approved these changes
Jul 29, 2026
EnjoyBacon7
left a comment
EnjoyBacon7
left a comment
Collaborator
There was a problem hiding this comment.
Approving. Scoping retrieval by a client-supplied attachment list, with the indexed subset reported back in extra.attachments.
Review trail — all raised issues resolved:
openrag-allno longer blocks attachments forSUPER_ADMIN_MODEadmins. The wildcard takes a single unscoped catalog lookup rather than fail-closing to zero results, guarded against a mixed["all", ...]list.- Per-partition lookups run concurrently instead of serially.
- Client-supplied duplicate ids are deduplicated before they reach the filter and the response.
get_existing_file_ids_any_partitionis declared on theWorkspaceRepositoryport, not just the Postgres implementation.
Verified beyond the diff
- The filterless fallback in
RetrieverPipelinedrops only the temporal filter and preservesfilter_params, so an attachment-scoped query that matches nothing can never silently widen to the whole partition. ["all"]reaches this layer solely for aSUPER_ADMIN_MODEadmin —get_partition_nameexpands it to memberships for everyone else, and"all"is a reserved partition name, so it cannot arrive via a membership list.- Both scoping queries are mutation-checked: changing either partition clause fails a test.
Left as follow-ups, not blockers
- A present-but-entirely-malformed
attachmentspayload (e.g.[{"file_id": ...}]instead of[{"id": ...}]) degrades to an unscoped partition search rather than an error. Deliberate and tested, but the absent case and the all-invalid case are distinguishable if you later want them to behave differently. - The attachment id list is unbounded and client-controlled; a cap would need a product call on truncate-vs-400.
Disclosure: four doc/test commits here are mine. The substantive code is @ewan102's, and I reviewed it before contributing.
Collaborator
Suggested ImprovementsI've reviewed the PR and identified two improvements worth considering: 1️⃣ Defensive Guard for Empty PartitionsAdd an early return when the partitions list is empty to avoid unnecessary database queries: async def _existing_file_ids(self, file_ids: Sequence[str], partitions: Sequence[str]) -> list[str]:
"""Order-preserving, deduplicated subset of ``file_ids`` indexed in ``partitions``.
``file_ids`` is client-supplied, so repeats are dropped rather than
echoed back into the filter and into ``extra.attachments``.
``"all"`` (``SUPER_ADMIN_MODE`` wildcard) takes an unscoped lookup instead
of a per-partition one.
"""
if not partitions:
return []
if "all" in partitions:
if len(partitions) > 1:
raise ValueError("`partitions` cannot mix the wildcard with explicit values.")
found = set(await self._workspace.get_existing_file_ids_any_partition(file_ids))
else:
results = await asyncio.gather(*(self._workspace.get_existing_file_ids(p, file_ids) for p in partitions))
found = {fid for r in results for fid in r}
return [fid for fid in dict.fromkeys(file_ids) if fid in found]Why: When no partitions are accessible, the current code still tries to look up files. This guard short-circuits the method cleanly. 2️⃣ API Documentation for AttachmentsAdd a row to the metadata table in
Example request: curl -X POST http://localhost:8080/v1/chat/completions \\
-H "Authorization: Bearer $AUTH_TOKEN" \\
-H "Content-Type: application/json" \\
-d '{
"model": "openrag-partition-name",
"messages": [{"role": "user", "content": "summarize these files"}],
"metadata": {
"attachments": [
{"id": "file-001"},
{"id": "file-002"},
{"id": "file-003"}
]
}
}'Response: The Both changes are optional polish — the PR implementation is solid as-is. ✅ |
Ahmath-Gadji
approved these changes
Aug 25, 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
4 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.
lists sent by a POST /v1/chat/completions, used a lot of the workspace filter mechanism, adding a lists of the file_id effectively indexed on the extra.attachments, in order to know which one of the file_ids' lists were not indexed in RAG
Summary by CodeRabbit
partitions=["all"]uses union-style resolution, while mixing"all"with explicit partitions raises an error.