feat: pgvector embedding_vec migration + chat similarity cutover#979
feat: pgvector embedding_vec migration + chat similarity cutover#979northdpole wants to merge 4 commits into
Conversation
northdpole
requested review from
Pa04rth,
paoga87 and
robvanderveer
as code owners
|
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:
WalkthroughAdds PostgreSQL pgvector migration and backfill support, stores embeddings as ChangesPgvector migration and retrieval
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
migrations/versions/c7d8e9f0a1b2_add_embedding_vec_pgvector.py (1)
30-50: 🚀 Performance & Scalability | 🔵 TrivialFails loudly and correctly on mixed dims; consider pairing with a similarity index.
require_single_embedding_dimraisingMultiDimEmbeddingErrorhere (uncaught) correctly aborts the migration rather than silently proceeding with wrong dims, matching the#977requirement. As noted onpgvector_utils.py, this migration also doesn't create an ANN index onembedding_vec— since this is the natural place to provision schema for the new column, consider adding anIF NOT EXISTSHNSW/IVFFlat index here (or a documented fast-follow migration) once dimensions/backfill are settled.🤖 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 `@migrations/versions/c7d8e9f0a1b2_add_embedding_vec_pgvector.py` around lines 30 - 50, Add schema provisioning for an ANN index on embedding_vec within upgrade after require_single_embedding_dim and backfill_embedding_vec complete, using the project’s established pgvector index type and naming convention with IF NOT EXISTS. Ensure the index is created only for PostgreSQL and after the column has been populated; if index creation is intentionally deferred, document the fast-follow migration instead.application/database/pgvector_utils.py (1)
138-154: 🚀 Performance & Scalability | 🔵 TrivialMissing similarity index — full scan on every
<=>query.Nothing here or in the migration creates an ANN index (HNSW/IVFFlat) on
embedding_vec, soORDER BY embedding_vec <=> ...will sequentially scan and compute distance for every row inembeddingson every chat/import lookup. Worth addingCREATE INDEX ... USING hnsw (embedding_vec vector_cosine_ops)in a follow-up migration once the table is populated, or documenting the plan to add it before this ships to a non-trivial dataset size.The Ruff S608 SQL-injection hint on this function is a false positive —
id_columnis validated against an allow-list (cre_id/node_id) before interpolation.🤖 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 `@application/database/pgvector_utils.py` around lines 138 - 154, Add an ANN vector index migration for the embeddings table, using HNSW with vector_cosine_ops on embedding_vec, and ensure it is applied after the table is populated or otherwise document and enforce that rollout timing before shipping. Keep most_similar_id_sql unchanged; its allow-listed id_column interpolation is safe and does not require refactoring.
🤖 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 `@application/cmd/cre_main.py`:
- Around line 1123-1124: Update the backend-selection logic near the warning in
cre_main.py to use the pgvector readiness helper from
application/database/db.py, or directly verify the embedding_vec column, rather
than checking only the database dialect. Ensure PostgreSQL instances lacking
Alembic revision c7d8e9f0a1b2 readiness do not select PgVectorRetriever, while
preserving the existing warning behavior for unavailable vector support.
In `@application/database/db.py`:
- Around line 2563-2597: Add defensive error handling around the pgvector query
in find_most_similar_embedding_id: catch database or driver exceptions from
bind.execute and subsequent row processing, log the failure using the class’s
existing logging mechanism, and return (None, None) instead of propagating the
error. Preserve the current threshold and successful-match behavior.
---
Nitpick comments:
In `@application/database/pgvector_utils.py`:
- Around line 138-154: Add an ANN vector index migration for the embeddings
table, using HNSW with vector_cosine_ops on embedding_vec, and ensure it is
applied after the table is populated or otherwise document and enforce that
rollout timing before shipping. Keep most_similar_id_sql unchanged; its
allow-listed id_column interpolation is safe and does not require refactoring.
In `@migrations/versions/c7d8e9f0a1b2_add_embedding_vec_pgvector.py`:
- Around line 30-50: Add schema provisioning for an ANN index on embedding_vec
within upgrade after require_single_embedding_dim and backfill_embedding_vec
complete, using the project’s established pgvector index type and naming
convention with IF NOT EXISTS. Ensure the index is created only for PostgreSQL
and after the column has been populated; if index creation is intentionally
deferred, document the fast-follow migration instead.
🪄 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.yml
Review profile: CHILL
Plan: Pro
Run ID: 5a698d90-e4f7-446a-b265-dfeca174e06e
📒 Files selected for processing (10)
.env.exampleapplication/cmd/cre_main.pyapplication/database/db.pyapplication/database/pgvector_utils.pyapplication/prompt_client/prompt_client.pyapplication/tests/db_test.pyapplication/tests/pgvector_utils_test.pyapplication/tests/prompt_client_pgvector_similarity_test.pyapplication/utils/librarian/candidate_retriever.pymigrations/versions/c7d8e9f0a1b2_add_embedding_vec_pgvector.py
Use can_use_pgvector_similarity for Librarian backend readiness, degrade similarity query errors to no-match, and document deferred HNSW on Essential-1. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@Makefile`:
- Around line 60-64: Makefile lines 60-64: update the Docker Postgres readiness
loop to verify readiness after all 20 attempts and fail with a clear timeout
message before running psql. scripts/run_benchmark.sh lines 44-47: replace the
unbounded readiness until loop with a bounded iteration loop that exits
successfully when pg_isready succeeds and explicitly fails with a timeout
message when all attempts are exhausted, matching the pattern used by
setup-heroku-staging.sh.
🪄 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.yml
Review profile: CHILL
Plan: Pro
Run ID: 3a501ef4-d165-44fc-a04c-d92873cc7852
📒 Files selected for processing (9)
AGENTS.mdMakefileapplication/cmd/cre_main.pyapplication/database/db.pyapplication/database/pgvector_utils.pyapplication/tests/prompt_client_pgvector_similarity_test.pymigrations/versions/c7d8e9f0a1b2_add_embedding_vec_pgvector.pyscripts/run_benchmark.shscripts/setup-heroku-staging.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- migrations/versions/c7d8e9f0a1b2_add_embedding_vec_pgvector.py
- application/database/pgvector_utils.py
| for i in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20; do \ | ||
| docker exec cre-postgres pg_isready -U cre -d cre >/dev/null 2>&1 && break; \ | ||
| sleep 1; \ | ||
| done; \ | ||
| docker exec cre-postgres psql -U cre -d cre -c "CREATE EXTENSION IF NOT EXISTS vector;" >/dev/null |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Readiness loops for Docker Postgres lack explicit failure on timeout.
The Docker Postgres readiness loops in the Makefile and benchmark script don't fail explicitly when the container doesn't become ready — the Makefile falls through to psql with a confusing error, and the benchmark script's unbounded until loop can hang indefinitely. scripts/setup-heroku-staging.sh (L243–L251) demonstrates the correct pattern with a bounded loop and explicit die on timeout.
Makefile#L60-L64: After the 20-iteration loop, verify readiness with an explicit failure before runningpsql, so the target fails with a clear message instead of a confusing connection error.scripts/run_benchmark.sh#L44-L47: Replace the unboundeduntilloop with a bounded iteration loop and explicit failure, matching the pattern inscripts/setup-heroku-staging.sh.
🔧 Proposed fix for Makefile readiness check
for i in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20; do \
docker exec cre-postgres pg_isready -U cre -d cre >/dev/null 2>&1 && break; \
sleep 1; \
done; \
- docker exec cre-postgres psql -U cre -d cre -c "CREATE EXTENSION IF NOT EXISTS vector;" >/dev/null
+ docker exec cre-postgres pg_isready -U cre -d cre >/dev/null 2>&1 || { \
+ echo "ERROR: cre-postgres did not become ready in 20s"; exit 1; \
+ }; \
+ docker exec cre-postgres psql -U cre -d cre -c "CREATE EXTENSION IF NOT EXISTS vector;" >/dev/null🔧 Proposed fix for run_benchmark.sh readiness check
echo "Waiting for $container to be ready..."
- until docker exec "$container" pg_isready -U cre -d cre; do
- sleep 1
- done
+ local ready=0
+ for _ in $(seq 1 30); do
+ if docker exec "$container" pg_isready -U cre -d cre >/dev/null 2>&1; then
+ ready=1
+ break
+ fi
+ sleep 1
+ done
+ [[ "$ready" == "1" ]] || { echo "ERROR: $container did not become ready in 30s"; exit 1; }
docker exec "$container" psql -U cre -d cre -c "CREATE EXTENSION IF NOT EXISTS vector;" >/dev/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.
| for i in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20; do \ | |
| docker exec cre-postgres pg_isready -U cre -d cre >/dev/null 2>&1 && break; \ | |
| sleep 1; \ | |
| done; \ | |
| docker exec cre-postgres psql -U cre -d cre -c "CREATE EXTENSION IF NOT EXISTS vector;" >/dev/null | |
| for i in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20; do \ | |
| docker exec cre-postgres pg_isready -U cre -d cre >/dev/null 2>&1 && break; \ | |
| sleep 1; \ | |
| done; \ | |
| docker exec cre-postgres pg_isready -U cre -d cre >/dev/null 2>&1 || { \ | |
| echo "ERROR: cre-postgres did not become ready in 20s"; exit 1; \ | |
| }; \ | |
| docker exec cre-postgres psql -U cre -d cre -c "CREATE EXTENSION IF NOT EXISTS vector;" >/dev/null |
📍 Affects 2 files
Makefile#L60-L64(this comment)scripts/run_benchmark.sh#L44-L47
🤖 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 `@Makefile` around lines 60 - 64, Makefile lines 60-64: update the Docker
Postgres readiness loop to verify readiness after all 20 attempts and fail with
a clear timeout message before running psql. scripts/run_benchmark.sh lines
44-47: replace the unbounded readiness until loop with a bounded iteration loop
that exits successfully when pg_isready succeeds and explicitly fails with a
timeout message when all attempts are exhausted, matching the pattern used by
setup-heroku-staging.sh.
Merge dual Alembic heads into one revision that adds embedding_vec, backfills from CSV, dual-writes on add_embedding, and prefers Postgres <=> for chat/import matching. Closes #977. Co-authored-by: Cursor <cursoragent@cursor.com>
session.get_bind() returns an Engine which no longer has .execute; open a Connection so can_use_pgvector_similarity and similarity queries work on Postgres.
make docker-postgres (and benchmark/staging seed helpers) default to pgvector/pgvector:pg16 and ensure CREATE EXTENSION vector on start.
northdpole
force-pushed
the
feat/pgvector-migration
branch
from
9743b79 to
2dc4062
Compare
|
Hey @northdpole, reviewed this from the Module C side and it lines up really well with the Librarian's C.1 retriever (#937). The migration adds the embedding_vec column, backfills it from the existing embeddings, and matches with <=>, which is exactly what our PgVectorRetriever already queries (embedding_vec, <=>, doc_type='CRE'). So once this merges, Module C just flips CRE_LIBRARIAN_RETRIEVER_BACKEND=pgvector and C.1 runs on it, with no code changes on my side. The dim handling looks solid too: detecting a single project-wide dim and hard-failing on mixed dims is the right call. Our CRE embeddings are 3072-dim, the CRE embeddings come out 3072-dim (from the shared gemini-embedding-001 embedder), and storing/searching vector(3072) is fine, so it matches what the retriever expects. Index is deferred here, which is totally fine. One thing worth flagging for when we do add it: pgvector's HNSW/IVFFlat index caps at ~2000 dims for the plain vector type, and ours is 3072-dim, so that follow-up index will need halfvec (which indexes up to 4000 dims) or a reduced dim. Storing and searching at 3072 aren't affected, it's only the index that hits the cap. Happy to help sort that out when we get to indexing. this' awesome as of now, |
Drop the legacy CSV embeddings column in c7d8e9f0a1b2 after backfill, port readers/writers (including Module C) to embedding_vec, and SystemExit when pgvector similarity is requested without Postgres. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
application/tests/db_test.py (1)
2393-2422: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate test to reflect preservation of the legacy CSV column.
The test actively asserts that the
embeddingscolumn is removed (self.assertNotIn("embeddings", ...)), which contradicts the PR requirement to preserve and dual-write to it. Update the test to expect both columns to be present and correctly populated.💡 Proposed fix
def test_add_embedding_writes_embedding_vec_literal(self): - """Vectors are stored only in embedding_vec (`#977` option C).""" + """Vectors are dual-written to both embedding_vec and the legacy CSV column.""" from application.database.pgvector_utils import to_pgvector_literal dbsa = db.Node( subsection="", section="Sec", name="DualWriteStd", link="https://example.com/dw", ntype=defs.Credoctypes.Standard.value, ) self.collection.session.add(dbsa) self.collection.session.commit() vec = [0.1, 0.2, 0.3] self.collection.add_embedding( db_object=dbsa, doctype=defs.Credoctypes.Standard.value, embeddings=vec, embedding_text="vec-only", ) row = self.collection.get_embedding(dbsa.id)[0] - self.assertNotIn("embeddings", type(row).__table__.c) + self.assertIn("embeddings", type(row).__table__.c) self.assertEqual(row.embeddings_content, "vec-only") self.assertEqual(row.embedding_vec, to_pgvector_literal(vec)) + self.assertEqual(row.embeddings, ",".join(str(x) for x in vec)) self.assertEqual(row.embedding_dim, 3) by_type = self.collection.get_embeddings_by_doc_type( defs.Credoctypes.Standard.value ) self.assertEqual(by_type[dbsa.id], vec)🤖 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 `@application/tests/db_test.py` around lines 2393 - 2422, Update test_add_embedding_writes_embedding_vec_literal to expect the legacy embeddings column to remain present and verify it is populated with the embedding vector, while retaining the existing embedding_vec, embedding_dim, and retrieval assertions.application/database/db.py (1)
2537-2562: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winDual-write to the legacy
embeddingsCSV column.The PR objectives require dual-writing new embeddings to both the new
embedding_vecand the legacy CSVembeddingscolumn. Ensure the legacy column is populated during both inserts and updates.💡 Proposed fix
cre_id=db_object.id, doc_type=cre_defs.Credoctypes.CRE.value, embeddings_content=embedding_text, embedding_model_id=embedding_model_id, embedding_dim=embedding_dim, embedding_vec=embedding_vec_literal, + embeddings=",".join(str(x) for x in embeddings), ) else: emb = Embeddings( node_id=db_object.id, doc_type=db_object.ntype, embeddings_content=embedding_text, embeddings_url=resolved_node_url, embedding_model_id=embedding_model_id, embedding_dim=embedding_dim, embedding_vec=embedding_vec_literal, + embeddings=",".join(str(x) for x in embeddings), ) self.session.add(emb) self.session.commit() return emb else: logger.debug(f"knew of embedding for object {db_object.id} ,updating") existing[0].embeddings_content = embedding_text existing[0].embedding_model_id = embedding_model_id existing[0].embedding_dim = embedding_dim existing[0].embedding_vec = embedding_vec_literal + existing[0].embeddings = ",".join(str(x) for x in embeddings)🤖 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 `@application/database/db.py` around lines 2537 - 2562, The embedding persistence flow around the Embeddings insert and existing[0] update currently writes only embedding_vec; also populate the legacy embeddings CSV column during both paths. Reuse the embedding vector’s established serialized representation for inserts and updates, keeping the new embedding_vec write unchanged.
🧹 Nitpick comments (1)
scripts/link_pci_dss_cre.py (1)
88-101: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMove the import statement outside the loop.
Importing
parse_stored_embedding_vecinside thefor db_node in pci_rows:loop causes unnecessary execution overhead during each iteration. Consider moving it to the beginning of themain()function alongside the other application imports.🛠️ Proposed refactor
from application.database import db from application.defs import cre_defs as defs from application.prompt_client import prompt_client + from application.database.pgvector_utils import parse_stored_embedding_vec collection = cre_main.db_connect(args.cache_file)And then remove it from the loop:
- from application.database.pgvector_utils import parse_stored_embedding_vec - control_embeddings = parse_stored_embedding_vec(emb_row.embedding_vec)🤖 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 `@scripts/link_pci_dss_cre.py` around lines 88 - 101, Move the parse_stored_embedding_vec import out of the for db_node in pci_rows loop and place it at the beginning of main() alongside the other application imports, then remove the inner-loop import while preserving the existing parsing behavior.
🤖 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 `@application/database/db.py`:
- Line 2384: Update get_embeddings_by_doc_type and
get_embeddings_by_doc_type_paginated to pass embedding_vec to
parse_stored_embedding_vec when available, otherwise fall back to the legacy
embeddings value via safe attribute access. Apply this change at
application/database/db.py lines 2384 and 2434.
- Around line 193-196: Restore the legacy nullable `embeddings` column alongside
`embedding_vec` in the model, preserving `embedding_vec` for the pgvector/SQLite
representation. Update the related persistence paths to dual-write both columns
so existing CSV consumers remain compatible.
In
`@application/utils/external_project_parsers/parsers/cloud_native_security_controls.py`:
- Line 61: Update the embedding-presence checks in
cloud_native_security_controls.py:61, juiceshop.py:71, and pci_dss.py:272 to
treat either embedding_vec or the legacy embeddings field as sufficient,
preserving the existing skip-re-embedding behavior when either value is present.
In `@application/utils/external_project_parsers/parsers/pci_dss.py`:
- Around line 108-110: Update the embedding selection in the node-processing
logic around parse_stored_embedding_vec to use emb_row.embedding_vec when
available, otherwise fall back to the legacy emb_row.embeddings value. Skip the
row only when neither column contains a usable vector, and preserve the existing
parsing and matching flow.
In `@migrations/versions/c7d8e9f0a1b2_add_embedding_vec_pgvector.py`:
- Around line 58-73: Preserve the legacy CSV embeddings column across the
migration. In migrations/versions/c7d8e9f0a1b2_add_embedding_vec_pgvector.py
lines 58-73, remove the leftover validation, embeddings DROP, and embedding_vec
SET NOT NULL; in lines 80-96, keep downgrade limited to dropping embedding_vec.
Update the migration docstrings at lines 8-12 and 37, and
application/database/pgvector_utils.py lines 6-7 and 21-26, to state that the
CSV column is preserved and adjust HEROKU_REEMBED_WARNING accordingly.
In `@scripts/rewrite_sqlite_embeddings_to_vec.py`:
- Around line 57-100: Make the table-recreation sequence in the embeddings
migration rerun-safe by removing any stale embeddings_new table before the
CREATE TABLE embeddings_new statement, or by enclosing the full recreate
sequence in an explicit transaction. Preserve the existing data-copy and rename
behavior in the migration flow.
- Around line 37-45: Update the SQL transformation in the embeddings rewrite
flow to preserve values already enclosed in both leading and trailing brackets,
instead of wrapping them again. Mirror the normalization behavior of
_as_vec_literal using explicit first-character and last-character checks rather
than LIKE '[%]', while continuing to bracket only non-empty unbracketed
embeddings and preserving the existing update conditions.
In `@scripts/sync_embeddings_table.py`:
- Around line 87-93: Update _as_vec_literal and the execute_batch flow to
preserve missing embeddings as NULL or exclude those rows before constructing
the %s::vector expression. Do not convert None or blank values to "[]"; ensure
valid non-empty vectors continue through the existing sync path without causing
pgvector errors.
---
Outside diff comments:
In `@application/database/db.py`:
- Around line 2537-2562: The embedding persistence flow around the Embeddings
insert and existing[0] update currently writes only embedding_vec; also populate
the legacy embeddings CSV column during both paths. Reuse the embedding vector’s
established serialized representation for inserts and updates, keeping the new
embedding_vec write unchanged.
In `@application/tests/db_test.py`:
- Around line 2393-2422: Update test_add_embedding_writes_embedding_vec_literal
to expect the legacy embeddings column to remain present and verify it is
populated with the embedding vector, while retaining the existing embedding_vec,
embedding_dim, and retrieval assertions.
---
Nitpick comments:
In `@scripts/link_pci_dss_cre.py`:
- Around line 88-101: Move the parse_stored_embedding_vec import out of the for
db_node in pci_rows loop and place it at the beginning of main() alongside the
other application imports, then remove the inner-loop import while preserving
the existing parsing behavior.
🪄 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.yml
Review profile: CHILL
Plan: Pro
Run ID: 58720d86-7771-422f-b709-568f8f6ad833
📒 Files selected for processing (17)
application/cmd/cre_main.pyapplication/database/db.pyapplication/database/pgvector_utils.pyapplication/tests/db_test.pyapplication/tests/librarian/candidate_retriever_test.pyapplication/tests/pci_dss_parser_test.pyapplication/tests/pgvector_utils_test.pyapplication/tests/prompt_client_pgvector_similarity_test.pyapplication/utils/external_project_parsers/parsers/cloud_native_security_controls.pyapplication/utils/external_project_parsers/parsers/juiceshop.pyapplication/utils/external_project_parsers/parsers/pci_dss.pyapplication/utils/librarian/candidate_retriever.pymigrations/versions/c7d8e9f0a1b2_add_embedding_vec_pgvector.pyscripts/benchmark_retriever.pyscripts/link_pci_dss_cre.pyscripts/rewrite_sqlite_embeddings_to_vec.pyscripts/sync_embeddings_table.py
| # Postgres: real ``vector(N)`` via Alembic (c7d8e9f0a1b2) — sole store for | ||
| # vectors (legacy CSV ``embeddings`` column is dropped in that migration). | ||
| # SQLite/tests: Text holding a pgvector literal ``[1.0,2.0,...]``. | ||
| embedding_vec = sqla.Column(sqla.Text, nullable=False) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Restore the legacy embeddings CSV column.
The PR objectives and linked Issue #977 explicitly require preserving the CSV embeddings column for backwards compatibility and dual-writing. Removing the legacy column and making embedding_vec the sole store violates this requirement.
💡 Proposed fix
embedding_dim = sqla.Column(sqla.Integer, nullable=True, default=None)
+ embeddings = sqla.Column(sqla.String, nullable=True, default=None)
# Postgres: real ``vector(N)`` via Alembic (c7d8e9f0a1b2) — sole store for
# vectors (legacy CSV ``embeddings`` column is dropped in that migration).
# SQLite/tests: Text holding a pgvector literal ``[1.0,2.0,...]``.
embedding_vec = sqla.Column(sqla.Text, nullable=False)📝 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.
| # Postgres: real ``vector(N)`` via Alembic (c7d8e9f0a1b2) — sole store for | |
| # vectors (legacy CSV ``embeddings`` column is dropped in that migration). | |
| # SQLite/tests: Text holding a pgvector literal ``[1.0,2.0,...]``. | |
| embedding_vec = sqla.Column(sqla.Text, nullable=False) | |
| embedding_dim = sqla.Column(sqla.Integer, nullable=True, default=None) | |
| embeddings = sqla.Column(sqla.String, nullable=True, default=None) | |
| # Postgres: real ``vector(N)`` via Alembic (c7d8e9f0a1b2) — sole store for | |
| # vectors (legacy CSV ``embeddings`` column is dropped in that migration). | |
| # SQLite/tests: Text holding a pgvector literal ``[1.0,2.0,...]``. | |
| embedding_vec = sqla.Column(sqla.Text, nullable=False) |
🤖 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 `@application/database/db.py` around lines 193 - 196, Restore the legacy
nullable `embeddings` column alongside `embedding_vec` in the model, preserving
`embedding_vec` for the pgvector/SQLite representation. Update the related
persistence paths to dual-write both columns so existing CSV consumers remain
compatible.
| ) | ||
| if embeddings: | ||
| for entry in embeddings: | ||
| vec = parse_stored_embedding_vec(entry.embedding_vec) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fallback to legacy embeddings if embedding_vec is unavailable.
To safely support the transition period before the backfill completes, and to honor the PR requirement to preserve the CSV column, the embedding retrieval methods should fall back to reading the legacy embeddings column when embedding_vec is empty.
application/database/db.py#L2384-L2384: Inget_embeddings_by_doc_type, change tovec = parse_stored_embedding_vec(getattr(entry, "embedding_vec", None) or getattr(entry, "embeddings", None)).application/database/db.py#L2434-L2434: Inget_embeddings_by_doc_type_paginated, change tovec = parse_stored_embedding_vec(getattr(entry, "embedding_vec", None) or getattr(entry, "embeddings", None)).
📍 Affects 1 file
application/database/db.py#L2384-L2384(this comment)application/database/db.py#L2434-L2434
🤖 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 `@application/database/db.py` at line 2384, Update get_embeddings_by_doc_type
and get_embeddings_by_doc_type_paginated to pass embedding_vec to
parse_stored_embedding_vec when available, otherwise fall back to the legacy
embeddings value via safe attribute access. Apply this change at
application/database/db.py lines 2384 and 2434.
| if existing: | ||
| embeddings = cache.get_embeddings_for_doc(existing[0]) | ||
| if embeddings: | ||
| if embeddings and getattr(embeddings, "embedding_vec", None): |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Prevent LLM re-embedding for existing records with legacy embeddings.
The PR objectives explicitly state that re-running must not trigger LLM re-embedding. By exclusively requiring embedding_vec to skip re-embedding, the parsers will trigger an external API call for records that only have the legacy embeddings column (e.g., before the backfill completes). Check for the presence of either column to safely skip re-embedding.
application/utils/external_project_parsers/parsers/cloud_native_security_controls.py#L61-L61: Update the condition toif embeddings and (getattr(embeddings, "embedding_vec", None) or getattr(embeddings, "embeddings", None)):.application/utils/external_project_parsers/parsers/juiceshop.py#L71-L71: Update the condition toif embeddings and (getattr(embeddings, "embedding_vec", None) or getattr(embeddings, "embeddings", None)):.application/utils/external_project_parsers/parsers/pci_dss.py#L272-L272: Update the condition toif embeddings and (getattr(embeddings, "embedding_vec", None) or getattr(embeddings, "embeddings", None)):.
📍 Affects 3 files
application/utils/external_project_parsers/parsers/cloud_native_security_controls.py#L61-L61(this comment)application/utils/external_project_parsers/parsers/juiceshop.py#L71-L71application/utils/external_project_parsers/parsers/pci_dss.py#L272-L272
🤖 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
`@application/utils/external_project_parsers/parsers/cloud_native_security_controls.py`
at line 61, Update the embedding-presence checks in
cloud_native_security_controls.py:61, juiceshop.py:71, and pci_dss.py:272 to
treat either embedding_vec or the legacy embeddings field as sufficient,
preserving the existing skip-re-embedding behavior when either value is present.
| if not emb_row or not emb_row.embedding_vec: | ||
| continue | ||
| node_embedding = parse_stored_embedding_vec(emb_row.embedding_vec) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fallback to legacy embeddings to avoid missing matches before backfill.
If embedding_vec is not yet populated (e.g., prior to completion of the backfill), this logic will silently skip nodes that have valid vectors in the legacy embeddings column. Fall back to the legacy column to maintain consistent matching.
💡 Proposed fix
- if not emb_row or not emb_row.embedding_vec:
+ if not emb_row or not (getattr(emb_row, "embedding_vec", None) or getattr(emb_row, "embeddings", None)):
continue
- node_embedding = parse_stored_embedding_vec(emb_row.embedding_vec)
+ node_embedding = parse_stored_embedding_vec(getattr(emb_row, "embedding_vec", None) or getattr(emb_row, "embeddings", None))📝 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.
| if not emb_row or not emb_row.embedding_vec: | |
| continue | |
| node_embedding = parse_stored_embedding_vec(emb_row.embedding_vec) | |
| if not emb_row or not (getattr(emb_row, "embedding_vec", None) or getattr(emb_row, "embeddings", None)): | |
| continue | |
| node_embedding = parse_stored_embedding_vec(getattr(emb_row, "embedding_vec", None) or getattr(emb_row, "embeddings", None)) |
🤖 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 `@application/utils/external_project_parsers/parsers/pci_dss.py` around lines
108 - 110, Update the embedding selection in the node-processing logic around
parse_stored_embedding_vec to use emb_row.embedding_vec when available,
otherwise fall back to the legacy emb_row.embeddings value. Skip the row only
when neither column contains a usable vector, and preserve the existing parsing
and matching flow.
| # Refuse to drop CSV if any row still lacks a vector (should be empty after | ||
| # a clean backfill; wrong-dim / empty CSV rows must be fixed offline). | ||
| leftover = conn.execute( | ||
| sa.text("SELECT COUNT(*) FROM embeddings WHERE embedding_vec IS NULL") | ||
| ).scalar() | ||
| if leftover: | ||
| raise RuntimeError( | ||
| f"{leftover} embeddings row(s) still have NULL embedding_vec after " | ||
| "backfill — refuse to DROP CSV column. Reconcile offline " | ||
| "(do not re-embed on Heroku). " + HEROKU_REEMBED_WARNING | ||
| ) | ||
|
|
||
| op.execute(sa.text("ALTER TABLE embeddings DROP COLUMN IF EXISTS embeddings")) | ||
| op.execute( | ||
| sa.text("ALTER TABLE embeddings ALTER COLUMN embedding_vec SET NOT NULL") | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
Preserve the legacy CSV embeddings column per PR objectives.
The PR objectives explicitly mandate preserving the legacy CSV embeddings column for SQLite/CI fallback paths and ongoing dual-writes, stating that dropping it is out of scope. The current implementation and docstrings incorrectly remove this column.
migrations/versions/c7d8e9f0a1b2_add_embedding_vec_pgvector.py#L58-L73: Remove theleftovercheck, theDROP COLUMNstatement forembeddings, and theSET NOT NULLconstraint forembedding_vec.migrations/versions/c7d8e9f0a1b2_add_embedding_vec_pgvector.py#L80-L96: Remove the logic that restores the CSV column in thedowngrade()function; only executeDROP COLUMN IF EXISTS embedding_vec.migrations/versions/c7d8e9f0a1b2_add_embedding_vec_pgvector.py#L8-L12: Update the docstring to clarify that the legacy CSV column is preserved.migrations/versions/c7d8e9f0a1b2_add_embedding_vec_pgvector.py#L37-L37: Remove "drop legacy CSV column" from the function docstring.application/database/pgvector_utils.py#L6-L7: Update the module docstring to reflect that the CSV column is preserved instead of dropped.application/database/pgvector_utils.py#L21-L26: Update theHEROKU_REEMBED_WARNINGmessage to state that the CSV column is preserved.
📍 Affects 2 files
migrations/versions/c7d8e9f0a1b2_add_embedding_vec_pgvector.py#L58-L73(this comment)migrations/versions/c7d8e9f0a1b2_add_embedding_vec_pgvector.py#L80-L96migrations/versions/c7d8e9f0a1b2_add_embedding_vec_pgvector.py#L8-L12migrations/versions/c7d8e9f0a1b2_add_embedding_vec_pgvector.py#L37-L37application/database/pgvector_utils.py#L6-L7application/database/pgvector_utils.py#L21-L26
🤖 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 `@migrations/versions/c7d8e9f0a1b2_add_embedding_vec_pgvector.py` around lines
58 - 73, Preserve the legacy CSV embeddings column across the migration. In
migrations/versions/c7d8e9f0a1b2_add_embedding_vec_pgvector.py lines 58-73,
remove the leftover validation, embeddings DROP, and embedding_vec SET NOT NULL;
in lines 80-96, keep downgrade limited to dropping embedding_vec. Update the
migration docstrings at lines 8-12 and 37, and
application/database/pgvector_utils.py lines 6-7 and 21-26, to state that the
CSV column is preserved and adjust HEROKU_REEMBED_WARNING accordingly.
| conn.execute( | ||
| """ | ||
| UPDATE embeddings | ||
| SET embedding_vec = '[' || embeddings || ']' | ||
| WHERE embeddings IS NOT NULL | ||
| AND trim(embeddings) <> '' | ||
| AND (embedding_vec IS NULL OR trim(embedding_vec) = '') | ||
| """ | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file list ==\n'
git ls-files 'scripts/*embeddings*' 'scripts/rewrite_sqlite_embeddings_to_vec.py' 'scripts/sync_embeddings_table.py' '*/parse*embedding*' '*/embedding*' | sed 's#^`#-` #'
printf '\n== outline: rewrite script ==\n'
ast-grep outline scripts/rewrite_sqlite_embeddings_to_vec.py --view expanded || true
printf '\n== outline: sync script ==\n'
ast-grep outline scripts/sync_embeddings_table.py --view expanded || true
printf '\n== locate parse_stored_embedding_vec ==\n'
rg -n "parse_stored_embedding_vec|_as_vec_literal|embedding_vec" scripts . -g '!**/.git/**' -g '!**/node_modules/**'Repository: OWASP/OpenCRE
Length of output: 19873
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== read rewrite script around target lines ==\n'
sed -n '1,140p' scripts/rewrite_sqlite_embeddings_to_vec.py | nl -ba
printf '\n== read sync script around helper ==\n'
sed -n '1,220p' scripts/sync_embeddings_table.py | nl -ba
printf '\n== read any parser references ==\n'
rg -n "parse_stored_embedding_vec" -S .Repository: OWASP/OpenCRE
Length of output: 238
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [
Path('scripts/rewrite_sqlite_embeddings_to_vec.py'),
Path('scripts/sync_embeddings_table.py'),
]:
print(f"\n## {path}")
text = path.read_text()
for needle in ["parse_stored_embedding_vec", "_as_vec_literal", "UPDATE embeddings", "embedding_vec"]:
if needle in text:
print(f"contains: {needle}")
PY
printf '\n== targeted reads ==\n'
sed -n '1,140p' scripts/rewrite_sqlite_embeddings_to_vec.py | nl -ba
printf '\n---\n'
sed -n '1,220p' scripts/sync_embeddings_table.py | nl -baRepository: OWASP/OpenCRE
Length of output: 401
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== scripts/rewrite_sqlite_embeddings_to_vec.py ==\n'
sed -n '1,140p' scripts/rewrite_sqlite_embeddings_to_vec.py | cat -n
printf '\n== scripts/sync_embeddings_table.py ==\n'
sed -n '80,120p' scripts/sync_embeddings_table.py | cat -n
printf '\n== application/database/pgvector_utils.py ==\n'
sed -n '165,235p' application/database/pgvector_utils.py | cat -n
printf '\n== sqlite LIKE probe ==\n'
python3 - <<'PY'
import sqlite3
conn = sqlite3.connect(':memory:')
cases = ['[1,2,3]', '[[1,2,3]]', '1,2,3', '[', '[]']
for s in cases:
row = conn.execute("SELECT ? LIKE '[%]'", (s,)).fetchone()[0]
print(f"{s!r} -> {row}")
PYRepository: OWASP/OpenCRE
Length of output: 9970
Handle already-bracketed embeddings values in scripts/rewrite_sqlite_embeddings_to_vec.py.
'[' || embeddings || ']' will turn existing pgvector literals into [[...]], which downstream parsing won’t accept. Mirror _as_vec_literal with an explicit leading/trailing bracket check; LIKE '[%]' is not enough here.
🤖 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 `@scripts/rewrite_sqlite_embeddings_to_vec.py` around lines 37 - 45, Update the
SQL transformation in the embeddings rewrite flow to preserve values already
enclosed in both leading and trailing brackets, instead of wrapping them again.
Mirror the normalization behavior of _as_vec_literal using explicit
first-character and last-character checks rather than LIKE '[%]', while
continuing to bracket only non-empty unbracketed embeddings and preserving the
existing update conditions.
| conn.commit() | ||
| # SQLite lacks reliable DROP COLUMN on older builds; recreate table. | ||
| src_cols = { | ||
| r[1] for r in conn.execute("PRAGMA table_info(embeddings)").fetchall() | ||
| } | ||
| select_id = "id" if "id" in src_cols else "NULL" | ||
| select_model = ( | ||
| "embedding_model_id" if "embedding_model_id" in src_cols else "NULL" | ||
| ) | ||
| select_dim = "embedding_dim" if "embedding_dim" in src_cols else "NULL" | ||
| select_url = "embeddings_url" if "embeddings_url" in src_cols else "NULL" | ||
| select_content = ( | ||
| "embeddings_content" if "embeddings_content" in src_cols else "NULL" | ||
| ) | ||
| conn.execute( | ||
| """ | ||
| CREATE TABLE embeddings_new ( | ||
| id TEXT PRIMARY KEY, | ||
| doc_type TEXT NOT NULL, | ||
| cre_id TEXT, | ||
| node_id TEXT, | ||
| embeddings_url TEXT, | ||
| embeddings_content TEXT, | ||
| embedding_model_id TEXT, | ||
| embedding_dim INTEGER, | ||
| embedding_vec TEXT NOT NULL | ||
| ) | ||
| """ | ||
| ) | ||
| conn.execute( | ||
| f""" | ||
| INSERT INTO embeddings_new ( | ||
| id, doc_type, cre_id, node_id, embeddings_url, | ||
| embeddings_content, embedding_model_id, embedding_dim, embedding_vec | ||
| ) | ||
| SELECT | ||
| {select_id}, doc_type, cre_id, node_id, {select_url}, | ||
| {select_content}, {select_model}, {select_dim}, embedding_vec | ||
| FROM embeddings | ||
| """ | ||
| ) | ||
| conn.execute("DROP TABLE embeddings") | ||
| conn.execute("ALTER TABLE embeddings_new RENAME TO embeddings") | ||
| conn.commit() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== file outline ==\n'
ast-grep outline scripts/rewrite_sqlite_embeddings_to_vec.py --view expanded || true
printf '\n== relevant lines ==\n'
nl -ba scripts/rewrite_sqlite_embeddings_to_vec.py | sed -n '1,220p'
printf '\n== search for embeddings_new / transaction handling ==\n'
rg -n "embeddings_new|BEGIN|commit\(\)|rollback\(\)" scripts/rewrite_sqlite_embeddings_to_vec.py . || true
printf '\n== sqlite autocommit probe ==\n'
python3 - <<'PY'
import sqlite3, os, tempfile, traceback
path = tempfile.mktemp(suffix=".sqlite3")
conn = sqlite3.connect(path)
print("initial isolation_level:", repr(conn.isolation_level))
conn.execute("CREATE TABLE embeddings (id TEXT PRIMARY KEY, doc_type TEXT NOT NULL, embedding_vec TEXT NOT NULL)")
conn.execute("INSERT INTO embeddings VALUES ('1','x','vec')")
conn.commit()
# mimic the script's post-commit state
print("in_transaction after commit:", conn.in_transaction)
# non-DML statement
conn.execute("CREATE TABLE embeddings_new (id TEXT PRIMARY KEY, doc_type TEXT NOT NULL, embedding_vec TEXT NOT NULL)")
print("in_transaction after CREATE TABLE:", conn.in_transaction)
# check persistence after close without explicit commit
conn.close()
conn2 = sqlite3.connect(path)
tables = conn2.execute("SELECT name FROM sqlite_master WHERE type='table' ORDER BY name").fetchall()
print("tables after close/reopen:", tables)
conn2.close()
os.remove(path)
PYRepository: OWASP/OpenCRE
Length of output: 337
🏁 Script executed:
python3 - <<'PY'
import sqlite3, tempfile, os, traceback
path = tempfile.mktemp(suffix=".sqlite3")
conn = sqlite3.connect(path)
conn.execute("CREATE TABLE embeddings (id TEXT PRIMARY KEY, doc_type TEXT NOT NULL, embedding_vec TEXT NOT NULL)")
conn.execute("INSERT INTO embeddings VALUES ('1','x','vec')")
conn.commit()
# orphan table exists because CREATE TABLE was autocommitted and process "crashed" before cleanup
conn.execute("CREATE TABLE embeddings_new (id TEXT PRIMARY KEY, doc_type TEXT NOT NULL, embedding_vec TEXT NOT NULL)")
conn.close()
conn = sqlite3.connect(path)
print("preexisting tables:", conn.execute("SELECT name FROM sqlite_master WHERE type='table' ORDER BY name").fetchall())
try:
conn.execute("CREATE TABLE embeddings_new (id TEXT PRIMARY KEY, doc_type TEXT NOT NULL, embedding_vec TEXT NOT NULL)")
except Exception as e:
print(type(e).__name__, str(e))
finally:
conn.close()
os.remove(path)
PYRepository: OWASP/OpenCRE
Length of output: 263
🏁 Script executed:
python3 - <<'PY'
import sqlite3, tempfile, os
path = tempfile.mktemp(suffix=".sqlite3")
conn = sqlite3.connect(path)
conn.execute("CREATE TABLE embeddings (id TEXT PRIMARY KEY, doc_type TEXT NOT NULL, embedding_vec TEXT NOT NULL)")
conn.commit()
print("before CREATE TABLE, in_transaction =", conn.in_transaction)
conn.execute("CREATE TABLE embeddings_new (id TEXT PRIMARY KEY, doc_type TEXT NOT NULL, embedding_vec TEXT NOT NULL)")
print("after CREATE TABLE, in_transaction =", conn.in_transaction)
conn.close()
conn = sqlite3.connect(path)
print("tables after close/reopen =", conn.execute("SELECT name FROM sqlite_master WHERE type='table' ORDER BY name").fetchall())
try:
conn.execute("CREATE TABLE embeddings_new (id TEXT PRIMARY KEY, doc_type TEXT NOT NULL, embedding_vec TEXT NOT NULL)")
except Exception as e:
print("rerun error:", type(e).__name__, str(e))
finally:
conn.close()
os.remove(path)
PYRepository: OWASP/OpenCRE
Length of output: 371
Make the recreate step rerun-safe scripts/rewrite_sqlite_embeddings_to_vec.py:57-100 CREATE TABLE embeddings_new persists if the process stops before the final commit, so a rerun fails with table embeddings_new already exists. Add DROP TABLE IF EXISTS embeddings_new before creating it, or wrap the recreate sequence in an explicit transaction.
🧰 Tools
🪛 OpenGrep (1.25.0)
[ERROR] 86-97: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🪛 Ruff (0.15.21)
[error] 87-96: Possible SQL injection vector through string-based query construction
(S608)
🤖 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 `@scripts/rewrite_sqlite_embeddings_to_vec.py` around lines 57 - 100, Make the
table-recreation sequence in the embeddings migration rerun-safe by removing any
stale embeddings_new table before the CREATE TABLE embeddings_new statement, or
by enclosing the full recreate sequence in an explicit transaction. Preserve the
existing data-copy and rename behavior in the migration flow.
| def _as_vec_literal(value: Any) -> str: | ||
| s = str(value or "").strip() | ||
| if not s: | ||
| return "[]" | ||
| if s.startswith("[") and s.endswith("]"): | ||
| return s | ||
| return _csv_to_literal(s) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file first.
ast-grep outline scripts/sync_embeddings_table.py --view expanded
# Show the relevant section around the reported lines.
nl -ba scripts/sync_embeddings_table.py | sed -n '1,260p'
# Find the vector conversion and insert paths.
rg -n "_as_vec_literal|_csv_to_literal|execute_batch|::vector|embedding_vec|public\\.embeddings" scripts/sync_embeddings_table.pyRepository: OWASP/OpenCRE
Length of output: 907
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' scripts/sync_embeddings_table.py
printf '\n---\n'
python3 - <<'PY'
from pathlib import Path
text = Path('scripts/sync_embeddings_table.py').read_text()
for needle in ['def _blank_to_none', 'def _normalize_embedding_row', 'def _as_vec_literal',
'def _fetch_postgres_embeddings', 'def _replace_postgres_embeddings']:
idx = text.find(needle)
print(f"\n### {needle} ###")
if idx >= 0:
start = max(0, idx - 200)
end = min(len(text), idx + 1800)
print(text[start:end])
PYRepository: OWASP/OpenCRE
Length of output: 17681
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '80,230p' scripts/sync_embeddings_table.py
printf '\n--- execute_batch / DELETE context ---\n'
python3 - <<'PY'
from pathlib import Path
lines = Path('scripts/sync_embeddings_table.py').read_text().splitlines()
for i in range(80, 231):
if i <= len(lines):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: OWASP/OpenCRE
Length of output: 9625
🌐 Web query:
pgvector empty vector literal [] zero dimension rejected NULL::vector
💡 Result:
In pgvector, empty vector literals (e.g., '[]' or '{}') are explicitly rejected because the extension requires vectors to have at least one dimension [1][2][3]. Any attempt to cast an empty array to a vector type will result in the error: "vector must have at least 1 dimension" [1][4][2]. This behavior is enforced by design within the pgvector source code [3]. Specifically, when converting arrays to vectors, the CheckDim function validates that the number of elements is greater than zero [3]. Because this validation occurs during the casting process, empty inputs are rejected before they can be stored [5][3]. Key technical details: - Error Message: "vector must have at least 1 dimension" [1][2]. - Scope: This applies to both string literals (e.g., '[]'::vector) and array-to-vector casts (e.g., '{}'::float[]::vector) [1][2]. - NULL vs Empty: Note that while a SQL NULL (NULL::vector) is a valid value for a nullable vector column, an empty vector literal is treated as a zero-dimension vector, which is invalid [6][7]. - Handling Nulls: If you need to represent the absence of a vector in your application logic, use a proper SQL NULL rather than an empty array or empty string [6][7]. If your ORM or application layer is defaulting empty fields to '[]', you must configure the application to send a true SQL NULL instead [8].
Citations:
- 1: https://github.com/pgvector/pgvector/blob/35ab919b/test/expected/cast.out
- 2: sql: add validation for edge case values in array-to-vector cast cockroachdb/cockroach#167092
- 3: https://github.com/pgvector/pgvector/blob/master/src/vector.c
- 4: https://community.n8n.io/t/how-to-fix-problem-in-node-postgres-pgvector-store/133266?tl=en
- 5: https://deepwiki.com/pgvector/pgvector/3.5-type-conversions-and-casting
- 6: https://github.com/pgvector/pgvector?tab=readme-ov-file
- 7: https://pgxn.org/dist/vector/0.8.2/README.html
- 8: Django support for empty fields pgvector/pgvector-python#67
Skip missing embeddings before %s::vector scripts/sync_embeddings_table.py:87-93,149-163
None/blank values are turned into "[]", and pgvector rejects empty vectors (vector must have at least 1 dimension). A single missing embedding will abort the sync; keep missing values as NULL or filter those rows before execute_batch.
🤖 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 `@scripts/sync_embeddings_table.py` around lines 87 - 93, Update
_as_vec_literal and the execute_batch flow to preserve missing embeddings as
NULL or exclude those rows before constructing the %s::vector expression. Do not
convert None or blank values to "[]"; ensure valid non-empty vectors continue
through the existing sync path without causing pgvector errors.
2 participants
Summary
9b1c2d3e4f50,ab12cd34ef56) in one revisionc7d8e9f0a1b2that enablesvector, addsembeddings.embedding_vec, and idempotently backfills from CSV (hard-fails on mixed dims; no LLM re-embed on Heroku).embedding_vecinadd_embedding; shared helpers inapplication/database/pgvector_utils.py.<=>when the column is present; SQLite/CI keep sklearn paths.embedding_vec;.env.examplenotes the migration.Closes #977.
Test plan
make lintpython -m unittest application.tests.pgvector_utils_test application.tests.prompt_client_pgvector_similarity_test application.tests.librarian.candidate_retriever_testdb_testmake test— 521 ran; 1 unrelated librarian golden-dataset drift failureflask db upgrade(CSV→vector only) → smoke chat + optionalCRE_LIBRARIAN_RETRIEVER_BACKEND=pgvectorOut of scope
embeddingscolumnalembic_versionsurgery / rewriting historical migrationsMade with Cursor
Follow-up in this PR