Phase 1: Storage and Schema#36
Open
sid-ak wants to merge 19 commits into
Open
Conversation
…nd GIN) for pgvector support.
…ibility and generation model agnostic.
…n the root AGENTS.md.
…in memory store.
…specific one for db store.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds a pgvector-backed storage layer with a shared protocol and in-memory fake, wires integration tests and Postgres test infrastructure, and updates contributor guidance, ADRs, docs navigation, and review automation. ChangesStore implementation and test wiring
Documentation and review workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (4)
packages/rag_core/src/rag_core/store/schema.py (1)
40-42: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider tuning HNSW build parameters.
Default HNSW
m/ef_constructionmay be suboptimal for recall/latency at scale; consider setting explicit values once embedding volume is known.🤖 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 `@packages/rag_core/src/rag_core/store/schema.py` around lines 40 - 42, The HNSW index definition in the schema setup uses default build settings, which may not be ideal at scale. Update the chunks embedding index creation in the schema module to set explicit HNSW parameters for m and ef_construction once the expected embedding volume and recall/latency target are known. Keep the change localized to the index DDL used for idx_chunks_embedding.packages/rag_core/src/rag_core/store/client.py (1)
89-118: 🎯 Functional Correctness | 🔵 TrivialTODO may already be resolved by the transaction.
add_chunksruns inside the implicit transaction opened byget_connection();with self.get_connection() as conn:rolls back on any exception raised during theCOPY(e.g., a malformed chunk dict), so the batch is already all-or-nothing per call — matching the "treat batch writes as all-or-nothing" convention. Consider closing out the TODO or clarifying what additional "partial success" scenario it's tracking (e.g., across multipleadd_chunkscalls).🤖 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 `@packages/rag_core/src/rag_core/store/client.py` around lines 89 - 118, The TODO in add_chunks is likely redundant because get_connection() and the COPY block already make each call transactional and all-or-nothing on errors. Update add_chunks in the ChunkStore client to either remove or resolve the TODO by clarifying that partial success is only a concern across multiple calls, not within a single batch insert. If you keep the note, make it explicit around the get_connection/copy flow and the add_chunks method so the intended behavior is clear..github/workflows/ci.yaml (1)
10-52: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd explicit least-privilege
permissions:block.Static analysis flags the job as running with default (broad)
GITHUB_TOKENpermissions since nopermissions:block is declared. Scope it down, e.g.contents: read, especially now that the job runs a service container and integration tests.🔒 Suggested fix
checks: name: Lint, typecheck, test runs-on: ubuntu-latest + permissions: + contents: read # Integration tests (rag_core/store) run against a real pgvector instance.🤖 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 @.github/workflows/ci.yaml around lines 10 - 52, The checks job in the CI workflow is using the default broad GITHUB_TOKEN scope because no permissions block is declared. Add an explicit least-privilege permissions section for this job, keeping only the access needed for actions/checkout and the test steps (for example, read-only repository contents). Locate the job under the checks name and update the workflow configuration accordingly.Source: Linters/SAST tools
.env.example (1)
8-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: env keys out of alphabetical/grouped order per linter.
dotenv-linter flags
POSTGRES_TEST_DB/POSTGRES_HOST/POSTGRES_PORTas out of order. Purely cosmetic; reorder if you want a clean lint run.🤖 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 @.env.example around lines 8 - 10, The PostgreSQL env entries in the .env.example block are out of the linter’s expected order; reorder the POSTGRES_* keys in that group to satisfy dotenv-linter. Keep the fix limited to the env variable declarations so the existing values remain unchanged.Source: Linters/SAST tools
🤖 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 `@docs/decisions/0005-documentation-tooling.md`:
- Around line 16-22: The ADR text has inconsistent MkDocs config filenames,
which makes the historical note ambiguous. Update the wording in this paragraph
so it matches the filename used elsewhere in the same decision and stays
consistent with the rest of the document. Use the existing ADR prose around the
MkDocs setup to locate the reference and normalize it everywhere it appears.
In `@docs/decisions/0008-per-model-embedding-columns.md`:
- Around line 8-12: The ADR text is out of sync with the implementation because
the embedding dimension is not hardcoded to 1024 in store/schema.py; it is
derived from settings.embedding_dimension at import time. Update the sentence in
this section to describe the actual behavior of the chunks.embedding vector
column and its HNSW index using the settings-derived dimension, so the docs
match the current schema logic.
In `@packages/rag_core/src/rag_core/store/memory.py`:
- Around line 63-68: The read methods in MemoryStore are returning only shallow
copies, so nested mutable fields like metadata and embedding can still be
mutated through the returned object. Update get_document_by_hash and the other
affected read path around the matching copy logic at the referenced section to
return a deep copy of each document instead of dict(doc)/dict(c), using the same
cloning behavior consistently across these accessors so callers cannot modify
the internal store through nested objects.
- Around line 70-85: InMemoryStore.add_chunks currently accepts chunks even when
chunk["document_id"] does not exist, which diverges from the real Store schema
behavior. Update add_chunks to validate the document_id against the in-memory
documents state before storing each chunk, and raise the same kind of integrity
failure the DB-backed store would surface when the document is missing. Keep the
behavior aligned with StoreProtocol and the DB Store implementation so both
stores reject orphan chunks consistently.
---
Nitpick comments:
In @.env.example:
- Around line 8-10: The PostgreSQL env entries in the .env.example block are out
of the linter’s expected order; reorder the POSTGRES_* keys in that group to
satisfy dotenv-linter. Keep the fix limited to the env variable declarations so
the existing values remain unchanged.
In @.github/workflows/ci.yaml:
- Around line 10-52: The checks job in the CI workflow is using the default
broad GITHUB_TOKEN scope because no permissions block is declared. Add an
explicit least-privilege permissions section for this job, keeping only the
access needed for actions/checkout and the test steps (for example, read-only
repository contents). Locate the job under the checks name and update the
workflow configuration accordingly.
In `@packages/rag_core/src/rag_core/store/client.py`:
- Around line 89-118: The TODO in add_chunks is likely redundant because
get_connection() and the COPY block already make each call transactional and
all-or-nothing on errors. Update add_chunks in the ChunkStore client to either
remove or resolve the TODO by clarifying that partial success is only a concern
across multiple calls, not within a single batch insert. If you keep the note,
make it explicit around the get_connection/copy flow and the add_chunks method
so the intended behavior is clear.
In `@packages/rag_core/src/rag_core/store/schema.py`:
- Around line 40-42: The HNSW index definition in the schema setup uses default
build settings, which may not be ideal at scale. Update the chunks embedding
index creation in the schema module to set explicit HNSW parameters for m and
ef_construction once the expected embedding volume and recall/latency target are
known. Keep the change localized to the index DDL used for idx_chunks_embedding.
🪄 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: 0f6dd409-0206-4495-a125-e334537bc025
⛔ Files ignored due to path filters (2)
docs/assets/architecture.svgis excluded by!**/*.svguv.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
.env.example.github/workflows/ci.yaml.pre-commit-config.yamlAGENTS.mdCHEATSHEET.mdREADME.mdconftest.pydocs/architecture.mddocs/conf.pydocs/decisions/0000-stack.mddocs/decisions/0001-rag-retrieval-boundary.mddocs/decisions/0002-local-embedding-and-reranking.mddocs/decisions/0003-local-single-user-scope.mddocs/decisions/0004-cli-batch-ingestion.mddocs/decisions/0005-documentation-tooling.mddocs/decisions/0006-interface-first-architecture.mddocs/decisions/0007-generation-backend.mddocs/decisions/0008-per-model-embedding-columns.mddocs/decisions/README.mddocs/index.mddocs/reference/index.mdinfra/postgres/init.sqlpackages/rag_core/AGENTS.mdpackages/rag_core/pyproject.tomlpackages/rag_core/src/rag_core/config.pypackages/rag_core/src/rag_core/store/__init__.pypackages/rag_core/src/rag_core/store/client.pypackages/rag_core/src/rag_core/store/memory.pypackages/rag_core/src/rag_core/store/protocol.pypackages/rag_core/src/rag_core/store/schema.pypackages/rag_core/tests/conftest.pypackages/rag_core/tests/test_db_store.pypackages/rag_core/tests/test_smoke.pypackages/rag_core/tests/test_store.pypyproject.toml
Comment on lines
+16
to
+22
| `mkdocs.yaml`, a generated API reference page, and a deploy workflow. That work surfaced a | ||
| governance problem rather than a technical one: MkDocs 2.0 (announced by a maintainer who took over | ||
| the project mid-2024) removes the plugin system entirely, has no migration path for existing | ||
| projects, and is currently unlicensed. Since `mkdocstrings` — the entire reason to use this stack — | ||
| is itself a plugin, pinning `mkdocs<2` only defers the exposure; it does not remove it. For a | ||
| project being written from scratch with no legacy MkDocs investment to protect, that is a reason to | ||
| pick a foundation without that single point of failure, not to work around it. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the MkDocs filename consistent.
This paragraph now says mkdocs.yaml, but the same ADR still refers to mkdocs.yml later on. Please normalize the filename so the historical note stays unambiguous.
🤖 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 `@docs/decisions/0005-documentation-tooling.md` around lines 16 - 22, The ADR
text has inconsistent MkDocs config filenames, which makes the historical note
ambiguous. Update the wording in this paragraph so it matches the filename used
elsewhere in the same decision and stays consistent with the rest of the
document. Use the existing ADR prose around the MkDocs setup to locate the
reference and normalize it everywhere it appears.
Comment on lines
+8
to
+12
| [ADR 0002](0002-local-embedding-and-reranking.md) pins `bge-m3` and notes that its output dimension | ||
| is baked into the pgvector `vector(N)` column and its index, so changing embedders means a re-embed | ||
| plus a schema migration — the "main lock-in." Today `chunks` has a single `embedding vector(1024)` | ||
| column with one HNSW index, and the dimension is read from the global `settings.embedding_dimension` | ||
| at import time inside `store/schema.py`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Match the ADR to the actual schema.
store/schema.py does not hardcode 1024; it derives the dimension from settings.embedding_dimension. Please rewrite this sentence so the current implementation is documented accurately.
🤖 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 `@docs/decisions/0008-per-model-embedding-columns.md` around lines 8 - 12, The
ADR text is out of sync with the implementation because the embedding dimension
is not hardcoded to 1024 in store/schema.py; it is derived from
settings.embedding_dimension at import time. Update the sentence in this section
to describe the actual behavior of the chunks.embedding vector column and its
HNSW index using the settings-derived dimension, so the docs match the current
schema logic.
Comment on lines
+63
to
+68
| def get_document_by_hash(self, file_hash: str) -> dict[str, Any] | None: | ||
| """Return a copy of the document with this file_hash, or None if absent.""" | ||
| for doc in self.documents.values(): | ||
| if doc["file_hash"] == file_hash: | ||
| return dict(doc) | ||
| return None |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Shallow copies leak mutable nested state (metadata, embedding) back to the store.
dict(doc) / dict(c) only shallow-copy the top level; a caller mutating the returned metadata dict or embedding list mutates the internal store directly, unlike the DB-backed Store where each read builds fresh Python objects from the wire.
🛠️ Proposed fix
- for doc in self.documents.values():
- if doc["file_hash"] == file_hash:
- return dict(doc)
+ for doc in self.documents.values():
+ if doc["file_hash"] == file_hash:
+ return copy.deepcopy(doc)- return [dict(c) for c in self.chunks.values() if c["document_id"] == document_id]
+ return [copy.deepcopy(c) for c in self.chunks.values() if c["document_id"] == document_id]Also applies to: 87-89
🤖 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 `@packages/rag_core/src/rag_core/store/memory.py` around lines 63 - 68, The
read methods in MemoryStore are returning only shallow copies, so nested mutable
fields like metadata and embedding can still be mutated through the returned
object. Update get_document_by_hash and the other affected read path around the
matching copy logic at the referenced section to return a deep copy of each
document instead of dict(doc)/dict(c), using the same cloning behavior
consistently across these accessors so callers cannot modify the internal store
through nested objects.
Comment on lines
+70
to
+85
| def add_chunks( | ||
| self, | ||
| chunks: list[dict[str, Any]], | ||
| user_id: UUID | None = None, | ||
| ) -> None: | ||
| """Store each chunk by id: ``{id, document_id, content, metadata, embedding}``.""" | ||
| uid = user_id or settings.default_user_id | ||
| for chunk in chunks: | ||
| self.chunks[chunk["id"]] = { | ||
| "id": chunk["id"], | ||
| "document_id": chunk["document_id"], | ||
| "content": chunk["content"], | ||
| "metadata": chunk["metadata"], | ||
| "embedding": chunk["embedding"], | ||
| "user_id": uid, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
add_chunks doesn't enforce the document_id FK that the real schema requires.
The DB schema declares document_id UUID NOT NULL REFERENCES documents(id) ON DELETE CASCADE (schema.py, line 31), so inserting a chunk for a nonexistent document raises an integrity error against the real Store. InMemoryStore.add_chunks has no such check and will silently accept orphan chunks, so the two StoreProtocol implementations diverge on this behavior. Since the store conventions explicitly require both implementations to stay in lockstep for protocol-guaranteed behavior, this gap could let a bug pass the [memory] contract run while the [db] run fails (or vice versa masking a real integrity issue).
🛠️ Proposed fix
def add_chunks(
self,
chunks: list[dict[str, Any]],
user_id: UUID | None = None,
) -> None:
"""Store each chunk by id: ``{id, document_id, content, metadata, embedding}``."""
uid = user_id or settings.default_user_id
for chunk in chunks:
+ if chunk["document_id"] not in self.documents:
+ raise ValueError(f"Unknown document_id: {chunk['document_id']!r}")
self.chunks[chunk["id"]] = {
"id": chunk["id"],
"document_id": chunk["document_id"],
"content": chunk["content"],
"metadata": chunk["metadata"],
"embedding": chunk["embedding"],
"user_id": uid,
}Based on path instructions: "when changing the protocol, keep both InMemoryStore and DB Store lockstep using the 3-pass approach."
📝 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
| def add_chunks( | |
| self, | |
| chunks: list[dict[str, Any]], | |
| user_id: UUID | None = None, | |
| ) -> None: | |
| """Store each chunk by id: ``{id, document_id, content, metadata, embedding}``.""" | |
| uid = user_id or settings.default_user_id | |
| for chunk in chunks: | |
| self.chunks[chunk["id"]] = { | |
| "id": chunk["id"], | |
| "document_id": chunk["document_id"], | |
| "content": chunk["content"], | |
| "metadata": chunk["metadata"], | |
| "embedding": chunk["embedding"], | |
| "user_id": uid, | |
| } | |
| def add_chunks( | |
| self, | |
| chunks: list[dict[str, Any]], | |
| user_id: UUID | None = None, | |
| ) -> None: | |
| """Store each chunk by id: ``{id, document_id, content, metadata, embedding}``.""" | |
| uid = user_id or settings.default_user_id | |
| for chunk in chunks: | |
| if chunk["document_id"] not in self.documents: | |
| raise ValueError(f"Unknown document_id: {chunk['document_id']!r}") | |
| self.chunks[chunk["id"]] = { | |
| "id": chunk["id"], | |
| "document_id": chunk["document_id"], | |
| "content": chunk["content"], | |
| "metadata": chunk["metadata"], | |
| "embedding": chunk["embedding"], | |
| "user_id": uid, | |
| } |
🤖 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 `@packages/rag_core/src/rag_core/store/memory.py` around lines 70 - 85,
InMemoryStore.add_chunks currently accepts chunks even when chunk["document_id"]
does not exist, which diverges from the real Store schema behavior. Update
add_chunks to validate the document_id against the in-memory documents state
before storing each chunk, and raise the same kind of integrity failure the
DB-backed store would surface when the document is missing. Keep the behavior
aligned with StoreProtocol and the DB Store implementation so both stores reject
orphan chunks consistently.
Source: Path instructions
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/PULL_REQUEST_TEMPLATE.md (1)
11-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake commit hashes optional in the walkthrough.
Requiring short hashes here makes the template brittle after rebases or squash merges and adds manual lookup work for authors. Keep the facet-based bullets, but treat hashes as optional metadata instead of a hard requirement.
♻️ Proposed tweak
- 1. Walkthrough: ... cites it traces to by short-hash ... + 1. Walkthrough: ... names the facet it changes; add a short-hash only when it helps tie the bullet back to a specific commit.🤖 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 @.github/PULL_REQUEST_TEMPLATE.md around lines 11 - 15, Update the walkthrough guidance in the PR template so short commit hashes are optional rather than required. In the bullet describing how to cite commits, keep the focus on the unique or non-obvious facets of the change and allow authors to omit hashes when they are unavailable or unstable after rebases/squash merges. Adjust the wording in the template text so it no longer instructs users to always pull hashes from the commit log, while still permitting them as optional metadata.
🤖 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.
Nitpick comments:
In @.github/PULL_REQUEST_TEMPLATE.md:
- Around line 11-15: Update the walkthrough guidance in the PR template so short
commit hashes are optional rather than required. In the bullet describing how to
cite commits, keep the focus on the unique or non-obvious facets of the change
and allow authors to omit hashes when they are unavailable or unstable after
rebases/squash merges. Adjust the wording in the template text so it no longer
instructs users to always pull hashes from the commit log, while still
permitting them as optional metadata.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b4bf7e3c-691e-4219-9583-d8f511decaf9
📒 Files selected for processing (2)
.github/PULL_REQUEST_TEMPLATE.mdpackages/rag_core/src/rag_core/store/schema.py
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rag_core/src/rag_core/store/schema.py
Owner
Author
|
/agentic_review |
Code Review by Qodo
Context used✅ Tickets:
🎫 Record embedder model/dimension and stub re-embed migration 🎫 Augment golden eval set with synthetic query variations 🎫 Phase 3 — Embeddings + dense retrieval (rag_core/embed) +3 more✅ Compliance rules (platform):
6 rules 1. chunks.embedding lacks model id
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| document_id UUID NOT NULL REFERENCES documents(id) ON DELETE CASCADE, | ||
| content TEXT NOT NULL, | ||
| metadata JSONB NOT NULL DEFAULT '{{}}', | ||
| embedding vector({settings.embedding_dimension}) NOT NULL, |
There was a problem hiding this comment.
1. chunks.embedding lacks model id 📎 Requirement gap ⚙ Maintainability
The new schema stores embeddings without any explicit embedder model identifier or recorded dimension, so embeddings are not attributable to a model and future embedder swaps require in-place interpretation/migration. This breaks the additive-swap requirement for embeddings.
Agent Prompt
## Issue description
The schema stores embeddings in `chunks.embedding` but does not record an embedder model identifier or the embedding dimension explicitly alongside the stored vectors.
## Issue Context
Compliance requires embeddings be attributable to a model and dimension so embedders can be swapped additively without ambiguous interpretation of existing vectors.
## Fix Focus Areas
- packages/rag_core/src/rag_core/store/schema.py[28-38]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+34
to
+42
| embedding vector({settings.embedding_dimension}) NOT NULL, | ||
| tsv tsvector GENERATED ALWAYS AS (to_tsvector('english', content)) STORED, | ||
| user_id UUID NOT NULL, | ||
| created_at TIMESTAMPTZ NOT NULL DEFAULT NOW() | ||
| ); | ||
|
|
||
| -- HNSW index for vector similarity search (cosine distance) | ||
| CREATE INDEX IF NOT EXISTS idx_chunks_embedding ON chunks | ||
| USING hnsw (embedding vector_cosine_ops); |
There was a problem hiding this comment.
2. Single embedding column/index 📎 Requirement gap ⚙ Maintainability
The schema creates only one embedding vector column and one HNSW index, so multiple embedders cannot coexist additively. This makes embedder changes destructive (requires rewriting/reindexing the shared column).
Agent Prompt
## Issue description
The schema defines a single shared `embedding vector(N)` column and one vector index, rather than per-embedder columns + indexes.
## Issue Context
Compliance requires additive embedder swaps by maintaining one vector column + index per active model, keyed/tagged by a model identifier.
## Fix Focus Areas
- packages/rag_core/src/rag_core/store/schema.py[28-45]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+38
| """SQL schema for documents and chunks with pgvector support. | ||
|
|
||
| Schema evolution is handled by ``Store.initialize_schema`` running this idempotent | ||
| ``CREATE ... IF NOT EXISTS`` bootstrap. A versioned migration mechanism is deferred until the | ||
| first breaking change actually needs one — most likely the re-embed / ``vector(N)`` dimension | ||
| change tracked by issue #15. Until then there is no existing data to migrate. | ||
| """ | ||
|
|
||
| from rag_core.config import settings | ||
|
|
||
| # Extension and Tables | ||
| SCHEMA_SQL = f""" | ||
| -- Ensure vector extension exists | ||
| CREATE EXTENSION IF NOT EXISTS vector; | ||
|
|
||
| -- Documents table: Tracks source files | ||
| CREATE TABLE IF NOT EXISTS documents ( | ||
| id UUID PRIMARY KEY, | ||
| filename TEXT NOT NULL, | ||
| -- UNIQUE makes re-ingesting the same file idempotent (upsert target below) and | ||
| -- doubles as the lookup index for get_document_by_hash. Single-user scope keys on | ||
| -- file_hash alone; multi-user would migrate this to (user_id, file_hash). | ||
| file_hash TEXT NOT NULL UNIQUE, | ||
| user_id UUID NOT NULL, | ||
| created_at TIMESTAMPTZ NOT NULL DEFAULT NOW() | ||
| ); | ||
|
|
||
| -- Chunks table: Stores semantic segments and embeddings | ||
| CREATE TABLE IF NOT EXISTS chunks ( | ||
| id UUID PRIMARY KEY, | ||
| document_id UUID NOT NULL REFERENCES documents(id) ON DELETE CASCADE, | ||
| content TEXT NOT NULL, | ||
| metadata JSONB NOT NULL DEFAULT '{{}}', | ||
| embedding vector({settings.embedding_dimension}) NOT NULL, | ||
| tsv tsvector GENERATED ALWAYS AS (to_tsvector('english', content)) STORED, | ||
| user_id UUID NOT NULL, | ||
| created_at TIMESTAMPTZ NOT NULL DEFAULT NOW() | ||
| ); |
There was a problem hiding this comment.
3. No embeddings table/migrations 📎 Requirement gap ≡ Correctness
The Phase 1 storage layer is incomplete because it omits the required embeddings table and explicitly defers a versioned migrations mechanism, while the store interface and implementations only provide CRUD for documents and chunks with no embedding CRUD surface. This violates the Phase 1 storage foundation and compliance requirements.
Agent Prompt
## Issue description
Phase 1 storage does not meet compliance requirements because the schema only defines `documents` and `chunks` while deferring a versioned migrations mechanism, and the storage seam (`StoreProtocol`) plus concrete store implementations expose CRUD only for documents/chunks with no corresponding CRUD APIs for embeddings.
## Issue Context
Compliance requires a pgvector-backed storage foundation that includes `documents`, `chunks`, and `embeddings` tables, supports migrations that apply cleanly, and provides CRUD operations for documents, chunks, and embeddings through the store interface and its implementations.
## Fix Focus Areas
- packages/rag_core/src/rag_core/store/schema.py[1-38]
- packages/rag_core/src/rag_core/store/client.py[28-32]
- packages/rag_core/src/rag_core/store/protocol.py[17-57]
- packages/rag_core/src/rag_core/store/client.py[39-128]
- packages/rag_core/src/rag_core/store/memory.py[26-89]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
87
to
89
| - Schema changes need a fresh DB: `initialize_schema()` is `CREATE ... IF NOT EXISTS` and will not | ||
| retrofit constraints, so run `docker compose down -v && docker compose up -d` after editing | ||
| `store/schema.py`. |
There was a problem hiding this comment.
4. Root agents.md leaks paths 📘 Rule violation ⚙ Maintainability
Root-level AGENTS.md includes concrete, package-specific implementation references (e.g., rag_core and store/schema.py) rather than staying purely cross-cutting. This reduces clarity of global guidance and should be moved to the package-scoped AGENTS.md.
Agent Prompt
## Issue description
The root `AGENTS.md` contains package-specific implementation details that should live in per-package `AGENTS.md` files.
## Issue Context
Compliance requires the root `AGENTS.md` to avoid concrete instructions scoped to a single package.
## Fix Focus Areas
- AGENTS.md[39-40]
- AGENTS.md[87-89]
- packages/rag_core/AGENTS.md[24-53]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+70
to
+85
| def add_chunks( | ||
| self, | ||
| chunks: list[dict[str, Any]], | ||
| user_id: UUID | None = None, | ||
| ) -> None: | ||
| """Store each chunk by id: ``{id, document_id, content, metadata, embedding}``.""" | ||
| uid = user_id or settings.default_user_id | ||
| for chunk in chunks: | ||
| self.chunks[chunk["id"]] = { | ||
| "id": chunk["id"], | ||
| "document_id": chunk["document_id"], | ||
| "content": chunk["content"], | ||
| "metadata": chunk["metadata"], | ||
| "embedding": chunk["embedding"], | ||
| "user_id": uid, | ||
| } |
There was a problem hiding this comment.
5. Orphan chunks allowed 🐞 Bug ≡ Correctness
InMemoryStore.add_chunks() accepts chunks whose document_id doesn’t exist, but the Postgres schema enforces a foreign key on chunks.document_id. This can let unit tests pass against InMemoryStore while real ingestion fails against Store with FK violations.
Agent Prompt
### Issue description
`InMemoryStore.add_chunks()` currently stores chunks without verifying that the referenced `document_id` exists. The DB-backed `Store` will reject the same data because `chunks.document_id` has an FK to `documents(id)`.
### Issue Context
This mismatch reduces the value of DB-free unit tests: ingestion logic that forgets to create the document first will look correct in memory tests but fail in integration/prod.
### Fix Focus Areas
- packages/rag_core/src/rag_core/store/memory.py[70-85]
- packages/rag_core/src/rag_core/store/schema.py[29-33]
### Suggested fix
In `InMemoryStore.add_chunks`, for each chunk:
- Check `chunk["document_id"] in self.documents`.
- If missing, raise a `KeyError` or `ValueError` (pick one and document it) so unit tests catch the same class of bug the DB would.
Optionally add a contract test asserting both implementations reject orphan chunks (so they can’t drift again).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+120
to
+127
| def get_chunks_by_document(self, document_id: UUID) -> list[dict[str, Any]]: | ||
| """Return every chunk row belonging to ``document_id``, or ``[]`` if none exist.""" | ||
| with self.get_connection() as conn: | ||
| cur = conn.execute( | ||
| "SELECT id, document_id, content, metadata, embedding, user_id " | ||
| "FROM chunks WHERE document_id = %s", | ||
| (document_id,), | ||
| ) |
There was a problem hiding this comment.
6. Unordered chunk retrieval 🐞 Bug ☼ Reliability
Store.get_chunks_by_document() does not ORDER BY, so Postgres can return chunks in an arbitrary order, while InMemoryStore returns dict insertion order. This creates inconsistent behavior across implementations and can lead to nondeterministic results if callers assume stable ordering.
Agent Prompt
### Issue description
`get_chunks_by_document()` currently returns rows with no defined ordering in the DB-backed implementation, while the in-memory fake returns insertion order. This can lead to nondeterminism and implementation-dependent behavior.
### Issue Context
Even if no current caller relies on ordering, adding deterministic ordering now prevents subtle future bugs and flaky tests.
### Fix Focus Areas
- packages/rag_core/src/rag_core/store/client.py[120-128]
- packages/rag_core/src/rag_core/store/memory.py[87-89]
### Suggested fix
Pick and document an ordering contract, then implement it in both stores:
- Minimal: `ORDER BY id` (stable but semantically weak).
- Better: add/return an explicit sequence field (e.g., `chunk_index` in metadata or a dedicated column in a later migration) and order by it.
Update `InMemoryStore.get_chunks_by_document()` to sort accordingly (not rely on dict iteration order).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
.coderabbit.yaml (1)
8-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrafts + wildcard branches may increase review noise.
Combining
auto_review.drafts: truewithbase_branches: [".*"]triggers full reviews on every push to draft PRs across all branches, which may generate reviews on early, unstable WIP commits.🤖 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 @.coderabbit.yaml around lines 8 - 13, The auto_review configuration is too broad because drafts are enabled together with a wildcard base branch match, causing reviews on unstable WIP pushes. Update the .coderabbit.yaml auto_review settings so the review trigger is narrower by either disabling drafts or restricting base_branches in the auto_review block, keeping the change localized to the auto_review.enabled, auto_review.drafts, and auto_review.base_branches keys.
🤖 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 @.github/workflows/pr_agent.yaml:
- Around line 4-5: The pr_agent workflow trigger only covers opened, reopened,
and ready_for_review PR events, so updated PRs with new commits will not rerun.
Update the pull_request event in pr_agent.yaml to include synchronize so the
workflow matches the “new/updated PRs” behavior described in the job comments;
check the trigger block and any related workflow documentation text near the
auto-run description.
- Around line 6-10: The workflow trigger is too broad because pr_agent_job still
runs for all issue_comment events, not just pull request comments. Update the
github event filtering in the workflow so the job only starts when the comment
is on a PR, using the existing issue_comment trigger with a PR-specific
condition in pr_agent_job (or equivalent PR-only gate) while keeping the Bot
exclusion intact.
- Line 20: The workflow currently uses the third-party action
the-pr-agent/pr-agent via a mutable ref, which should be pinned to an immutable
version. Update the uses entry in the pr_agent workflow to reference a specific
release tag or commit SHA instead of main so the action version cannot change
unexpectedly. Keep the change limited to the existing action reference and
ensure the chosen ref is stable and reproducible.
- Around line 12-16: The GitHub Actions permissions block includes an unused
checks: write scope, which should be removed unless github.publish_as_check_run
is enabled. Update the permissions in pr_agent.yaml to keep only the required
scopes used by the workflow, and leave the existing issues, pull-requests, and
contents permissions intact.
---
Nitpick comments:
In @.coderabbit.yaml:
- Around line 8-13: The auto_review configuration is too broad because drafts
are enabled together with a wildcard base branch match, causing reviews on
unstable WIP pushes. Update the .coderabbit.yaml auto_review settings so the
review trigger is narrower by either disabling drafts or restricting
base_branches in the auto_review block, keeping the change localized to the
auto_review.enabled, auto_review.drafts, and auto_review.base_branches keys.
🪄 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: 0882d275-180a-4cf5-bea8-04523b179aaf
📒 Files selected for processing (2)
.coderabbit.yaml.github/workflows/pr_agent.yaml
Comment on lines
+6
to
+10
| issue_comment: | ||
|
|
||
| jobs: | ||
| pr_agent_job: | ||
| if: ${{ github.event.sender.type != 'Bot' }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Gate comment runs to PR comments.
Line 6 triggers on every issue comment, while Line 10 only excludes bots. That starts the action for ordinary issue comments too, with write-scoped token access and the Gemini secret available to the action.
Suggested fix
- if: ${{ github.event.sender.type != 'Bot' }}
+ if: ${{ github.event.sender.type != 'Bot' && (github.event_name != 'issue_comment' || github.event.issue.pull_request != null) }}📝 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
| issue_comment: | |
| jobs: | |
| pr_agent_job: | |
| if: ${{ github.event.sender.type != 'Bot' }} | |
| issue_comment: | |
| jobs: | |
| pr_agent_job: | |
| if: ${{ github.event.sender.type != 'Bot' && (github.event_name != 'issue_comment' || github.event.issue.pull_request != null) }} |
🤖 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 @.github/workflows/pr_agent.yaml around lines 6 - 10, The workflow trigger is
too broad because pr_agent_job still runs for all issue_comment events, not just
pull request comments. Update the github event filtering in the workflow so the
job only starts when the comment is on a PR, using the existing issue_comment
trigger with a PR-specific condition in pr_agent_job (or equivalent PR-only
gate) while keeping the Bot exclusion intact.
| name: Run PR Agent (Gemini free tier) on pull requests and comments | ||
| steps: | ||
| - name: PR Agent action step | ||
| uses: the-pr-agent/pr-agent@main |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect available immutable refs for PR Agent before choosing a pinned version.
git ls-remote --tags https://github.com/the-pr-agent/pr-agent.git | tail -n 20
git ls-remote https://github.com/the-pr-agent/pr-agent.git refs/heads/mainRepository: sid-ak/study_assistant
Length of output: 1372
🏁 Script executed:
sed -n '1,80p' .github/workflows/pr_agent.yaml | cat -nRepository: sid-ak/study_assistant
Length of output: 1373
Pin the-pr-agent/pr-agent to an immutable ref. @main is mutable for a third-party action; use a release tag or commit SHA instead.
🤖 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 @.github/workflows/pr_agent.yaml at line 20, The workflow currently uses the
third-party action the-pr-agent/pr-agent via a mutable ref, which should be
pinned to an immutable version. Update the uses entry in the pr_agent workflow
to reference a specific release tag or commit SHA instead of main so the action
version cannot change unexpectedly. Keep the change limited to the existing
action reference and ensure the chosen ref is stable and reproducible.
…review on new commits
Owner
Author
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
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
Adds the storage layer for the study assistant's shared retrieval library (
rag_core). It sets up a Postgres (pgvector) schema for source documents and the text chunks split out of them, plus a small client that reads and writes them. The client sits behind an interface, so the later ingestion and retrieval work builds on that abstraction instead of talking to the database directly.documentsandchunkswith an HNSW index for cosine similarity, a GIN index over agenerated
tsvectorcolumn for full-text search, and the single-useruser_idseam (fc5fb2f).COPY ... FORMAT BINARYrather than per-row
INSERT(ee64b04).StoreProtocolwith two implementations — the pgvector-backedStoreand anInMemoryStorefake — so callers bind to the interface and unit tests run without a database(5c3f016).
genuinely DB-only behavior (vector round-tripping, schema DDL idempotency) split into a separate
integration module; CI gains a pgvector service and a test database provisioned by
init.sql(544b180, 467e0b7).
Motivation
Phase 1 (issue #2): the storage layer every later phase builds on, stood up behind the ADR 0006
interface boundary so ingestion and retrieval depend on a
Protocolrather than raw pgvector.Type
Related issues
Closes #2.
Verification
pre-commit run --all-filesanduv run sphinx-build -b html docs site -W— the static gate anddocs build: hygiene hooks, ruff lint + format, mypy, and the Sphinx build with warnings-as-errors.
All green.
docker compose up -d && uv run pytest— the full test suite (unit plus DB-backed integration);20 passed. The database must be up or the integration tests fail to connect.
docker compose up -d && uv run pytest -m integration— 10 passed. The DB-backed tests round-tripa chunk through real pgvector (the
embeddingcolumn reads back as a numpy array), confirmdelete_documentcascades to its chunks, and confirminitialize_schema()is idempotent(test_db_store.py plus the
[db]leg of the contract suite).uv run pytest -m unit— 10 passed with no database: the parametrizedStoreProtocolcontractsuite runs against
InMemoryStore, and the conformance test asserts bothStoreandInMemoryStoresatisfyStoreProtocol(test_store.py).Decision records / ADRs
behind
Protocols; the seam this branch implements for the store.recorded here, implementation deferred to Phase 3 (Phase 3 — Embeddings + dense retrieval (rag_core/embed) #4).