Feat/benchmark query expansion#56
Merged
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR introduces a cross-session synthesis feature that aggregates pattern facts from active sessions using an LLM, with automatic triggers based on session count intervals. It extends storage backends with syntheses tables and APIs, integrates synthesis retrieval into the search pipeline, updates scoring to handle preference facts differently from events, and adds comprehensive tests and benchmark improvements. Changes
Sequence DiagramsequenceDiagram
participant Client
participant VektoriClient
participant Synthesizer
participant LLM
participant Embedder
participant Storage
participant Search
Client->>VektoriClient: add(fact_text)
VektoriClient->>Storage: insert_fact(...)
VektoriClient->>VektoriClient: check synthesis_interval trigger
alt synthesis due
VektoriClient->>Synthesizer: synthesize(user_id)
Synthesizer->>Storage: get_active_facts (up to 300)
Storage-->>Synthesizer: base_facts
alt enough facts
Synthesizer->>LLM: generate(prompt with facts)
LLM-->>Synthesizer: JSON with pattern facts
Synthesizer->>Embedder: embed_batch(texts)
Embedder-->>Synthesizer: embeddings
loop for each synthesis fact
Synthesizer->>Storage: search_syntheses(embedding)
Storage-->>Synthesizer: existing similar syntheses
alt no duplicate (sim < 0.85)
Synthesizer->>Storage: insert_synthesis(...)
Synthesizer->>Storage: insert_synthesis_fact(links)
Storage-->>Synthesizer: synthesis_id
end
end
end
end
Client->>Client: later: search(query)
Client->>Search: search(query_embedding)
Search->>Storage: search_syntheses(embedding)
Storage-->>Search: syntheses
Search->>Storage: get_syntheses_for_facts(fact_ids)
Storage-->>Search: related syntheses
Search-->>Client: {facts, syntheses, episodes}
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related Issues
Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vektori/ingestion/extractor.py (1)
790-797:⚠️ Potential issue | 🟠 MajorDon’t increment mentions for facts you’re about to supersede.
On the cross-session path, this bumps
mentionson the existing fact and then deactivates it a few lines later. That diverges from_process_facts()and overstates stale facts during replay.Minimal fix
dedup = await self._check_dedup( fact_data["text"], fact_emb, session_id, user_id, agent_id, subject ) if dedup is not None: existing_id, same_session = dedup - await self.db.increment_fact_mentions(existing_id) if same_session: + await self.db.increment_fact_mentions(existing_id) continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektori/ingestion/extractor.py` around lines 790 - 797, The code increments mentions for a deduped existing fact unconditionally which incorrectly inflates mentions when you immediately deactivate that fact on the cross-session path; change the logic in the dedup handling (the _check_dedup result handling and the call to db.increment_fact_mentions) so that increment_fact_mentions is only called when same_session is true (i.e., only bump mentions for in-session duplicates), leaving cross-session duplicates unincremented to match the behavior of _process_facts().
🧹 Nitpick comments (1)
vektori/retrieval/scoring.py (1)
10-17: Wirepreference_decay_ratethrough the search pipeline as well.Right now this new knob is only configurable inside
score_and_rank(). The current callers invektori/retrieval/search.py:218-222and constructor invektori/retrieval/search.py:260-280still only carrytemporal_decay_rate, so callers can’t tune preference decay from the public API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektori/retrieval/scoring.py` around lines 10 - 17, The new preference_decay_rate parameter in score_and_rank is not being exposed through the public search pipeline; update the Search constructor (e.g., class Search.__init__) to accept preference_decay_rate with a sensible default, store it on the instance, and update the search invocation (e.g., Search.search or the function that calls score_and_rank) to accept/forward preference_decay_rate from callers; finally, when score_and_rank is invoked inside the search pipeline, pass the instance/configured preference_decay_rate alongside temporal_decay_rate so external callers can tune it via the public API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test_conflict.py`:
- Around line 15-43: The test never triggers the LLM-based contradiction path
because MockLLM.generate returns None; update MockLLM.generate to return a
deterministic, non-None response shaped like the real LLM output that
FactExtractor._check_dedup expects (e.g., the classification/supersession
verdict or JSON string it parses) so the supersession branch runs, then modify
the test to assert that the second inserted fact was marked superseded or
replaced (use MemoryBackend.get_active_facts and any supersession flag) instead
of only printing results; reference MockLLM.generate,
FactExtractor._check_dedup, and MemoryBackend.get_active_facts to locate and
change the code.
In `@test_conflict2.py`:
- Around line 25-39: The file defines an ad-hoc demo async test that runs on
import (async def test() + asyncio.run(test())) and only prints results; convert
this into a proper automated test that uses assertions and does not execute on
import: replace the top-level asyncio.run call with a test function recognizable
by the test runner (e.g., async def test_fact_supersession() with
`@pytest.mark.asyncio` or an asyncio-compatible fixture), call MemoryBackend(),
FactExtractor(...), await extractor._process_facts(...), then assert expected
outcomes using db.get_active_facts("u1") (e.g., assert len(facts) == X and
expected texts present/absent) and remove any print statements so the module
does not execute when imported.
In `@test_llm_dedup_check.py`:
- Around line 15-33: The MockLLM.generate mock no longer matches the extractor's
expected LLM contract: _check_dedup() now expects a JSON key named
"supersedes_id" but MockLLM.generate returns "contradicts" and its body is
effectively a no-op, so the dedup branch is untested; update MockLLM.generate to
parse the prompt conditions and return the JSON shape the extractor expects
(i.e., include "supersedes_id" with appropriate id or null), and add a test
prompt in async test() that triggers the extractor._check_dedup path (use the
same trigger text you check for in MockLLM.generate) so the dedup behavior is
exercised by the test harness (refer to MockLLM.generate, _check_dedup,
FactExtractor, and the test() entrypoint).
In `@test_synthesis.py`:
- Around line 4-26: The current file runs an import-time script that prints
results; convert it into a proper pytest async test that asserts behavior.
Replace the top-level async test() and asyncio.run call with a pytest test
function (e.g., use `@pytest.mark.asyncio` async def
test_synthesis_creates_and_stores():), instantiate Vektori and call await
v._ensure_initialized(), mock v.llm.generate and v.embedder.embed_batch as
before, insert the 5 dummy facts, call n = await v.synthesize("u1") and assert
on n (expected synthesized count/structure) and on facts = await
v.db.get_active_facts("u1", limit=100") asserting the number of facts with
metadata.source == "synthesis" and their text, and finally clean up/close v if
needed; reference Vektori, v._ensure_initialized, v.llm.generate,
v.embedder.embed_batch, v.db.insert_fact, v.synthesize, and
v.db.get_active_facts to locate code.
In `@vektori/client.py`:
- Around line 172-191: The auto-synthesis background task can race with ongoing
extraction and is not drained on shutdown; to fix, only spawn _bg_synthesize
after the new session's facts are extracted/committed (i.e., move/create the
asyncio.create_task call to the code path that runs after ingest/extraction
completes) and ensure self._bg_tasks is properly tracked and awaited in close():
add tasks to self._bg_tasks (as already done) and implement close() to
cancel/await asyncio.gather(*self._bg_tasks, return_exceptions=True) (or wait
for each task with asyncio.wait) before tearing down DB/LLM; reference the
_bg_synthesize coroutine, the synthesize method, the config.async_extraction
flag, the _bg_tasks set, and the close() method when making these changes.
In `@vektori/ingestion/synthesis.py`:
- Around line 57-59: The synthesis step is showing "Session: unknown" because
Postgres facts don't include session_id; update either the Postgres loader
(vektori/storage/postgres.py) to include session_id on each fact (extracting it
from the metadata JSON and returning it as the top-level "session_id" key) or
change the builder in synthesis.py to read session_id from f.get('metadata',
{}).get('session_id') when constructing facts_list (the generator using
base_facts and SYNTHESIS_PROMPT), so the prompt receives a proper Session value
instead of "unknown".
- Around line 1-8: The import block in vektori.ingestion.synthesis.py contains
unused imports and is misordered; remove the unused imports (delete json and
Any) and reorder the remaining imports into standard-library first (logging,
datetime, timezone), then local package imports (from vektori.models.base import
LLMProvider, EmbeddingProvider; from vektori.storage.base import StorageBackend;
from vektori.ingestion.extractor import _parse_json_response) so Ruff/isort stop
failing and only used symbols remain.
- Around line 71-81: Filter and normalize new_facts before calling
self.embedder.embed_batch: iterate new_facts and build a validated list that
only includes dicts where "text" is a non-empty string and coerce/normalize
"confidence" to a numeric value (e.g., float between 0 and 1, defaulting to 1.0
if missing/invalid); use that validated list to create texts and call
embed_batch so a single malformed item won't abort synthesis, and then zip the
validated facts with embeddings (refer to new_facts, texts,
self.embedder.embed_batch, fact_dict, embeddings, and confidence).
- Around line 83-98: The current dedup logic uses self.db.search_facts with
limit=1 so if the top match is a base fact you can miss a near-identical
synthesis fact and create a duplicate; change the search to return multiple
candidates (e.g., increase limit to a small handful like 5), iterate the
returned existing results to find the nearest candidate whose metadata.source ==
"synthesis" and whose similarity (1.0 - distance) > 0.85, then call
self.db.increment_fact_mentions(best["id"]) for that matching candidate and set
skip=True; use the same user_id/agent_id/active_only filtering already passed to
self.db.search_facts so scope matches the extractor’s subject-scoped dedup.
---
Outside diff comments:
In `@vektori/ingestion/extractor.py`:
- Around line 790-797: The code increments mentions for a deduped existing fact
unconditionally which incorrectly inflates mentions when you immediately
deactivate that fact on the cross-session path; change the logic in the dedup
handling (the _check_dedup result handling and the call to
db.increment_fact_mentions) so that increment_fact_mentions is only called when
same_session is true (i.e., only bump mentions for in-session duplicates),
leaving cross-session duplicates unincremented to match the behavior of
_process_facts().
---
Nitpick comments:
In `@vektori/retrieval/scoring.py`:
- Around line 10-17: The new preference_decay_rate parameter in score_and_rank
is not being exposed through the public search pipeline; update the Search
constructor (e.g., class Search.__init__) to accept preference_decay_rate with a
sensible default, store it on the instance, and update the search invocation
(e.g., Search.search or the function that calls score_and_rank) to
accept/forward preference_decay_rate from callers; finally, when score_and_rank
is invoked inside the search pipeline, pass the instance/configured
preference_decay_rate alongside temporal_decay_rate so external callers can tune
it via the public API.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: df2643d3-a0b9-4c75-8d73-af146748414e
📒 Files selected for processing (10)
benchmarks/longmemeval/longmemeval_runner.pytest_conflict.pytest_conflict2.pytest_llm_dedup_check.pytest_synthesis.pyvektori/client.pyvektori/config.pyvektori/ingestion/extractor.pyvektori/ingestion/synthesis.pyvektori/retrieval/scoring.py
Comment on lines
+15
to
+43
| class MockLLM(LLMProvider): | ||
| async def generate(self, prompt, max_tokens=1000, **kwargs): | ||
| pass | ||
|
|
||
| async def test(): | ||
| db = MemoryBackend() | ||
| await db.initialize() | ||
| embedder = MockEmbedder() | ||
| llm = MockLLM() | ||
| extractor = FactExtractor(db, embedder, llm) | ||
|
|
||
| # insert first fact | ||
| await extractor._process_facts( | ||
| [{"text": "User loves apples"}], | ||
| session_id="s1", user_id="u1", agent_id=None, conversation="User: I love apples" | ||
| ) | ||
|
|
||
| # insert second fact | ||
| await extractor._process_facts( | ||
| [{"text": "User hates apples"}], | ||
| session_id="s2", user_id="u1", agent_id=None, conversation="User: Actually I hate apples" | ||
| ) | ||
|
|
||
| facts = await db.get_active_facts("u1") | ||
| print("Active facts count:", len(facts)) | ||
| for f in facts: | ||
| print(f["text"]) | ||
|
|
||
| asyncio.run(test()) |
There was a problem hiding this comment.
This harness never verifies the new contradiction path.
MockLLM.generate() returns None, so _check_dedup() falls back to the non-supersession path, and the module only prints the result afterward. As written, this file won’t catch regressions in the LLM-based conflict handling at all.
Suggested direction
class MockLLM(LLMProvider):
async def generate(self, prompt, max_tokens=1000, **kwargs):
- pass
+ if "loves apples" in prompt and "hates apples" in prompt:
+ old_id = prompt.split("- [")[1].split("]")[0]
+ return f'{{"supersedes_id": "{old_id}"}}'
+ return '{"supersedes_id": null}'
@@
- facts = await db.get_active_facts("u1")
- print("Active facts count:", len(facts))
- for f in facts:
- print(f["text"])
-
-asyncio.run(test())
+ facts = await db.get_active_facts("u1")
+ assert [f["text"] for f in facts] == ["User hates apples"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| class MockLLM(LLMProvider): | |
| async def generate(self, prompt, max_tokens=1000, **kwargs): | |
| pass | |
| async def test(): | |
| db = MemoryBackend() | |
| await db.initialize() | |
| embedder = MockEmbedder() | |
| llm = MockLLM() | |
| extractor = FactExtractor(db, embedder, llm) | |
| # insert first fact | |
| await extractor._process_facts( | |
| [{"text": "User loves apples"}], | |
| session_id="s1", user_id="u1", agent_id=None, conversation="User: I love apples" | |
| ) | |
| # insert second fact | |
| await extractor._process_facts( | |
| [{"text": "User hates apples"}], | |
| session_id="s2", user_id="u1", agent_id=None, conversation="User: Actually I hate apples" | |
| ) | |
| facts = await db.get_active_facts("u1") | |
| print("Active facts count:", len(facts)) | |
| for f in facts: | |
| print(f["text"]) | |
| asyncio.run(test()) | |
| class MockLLM(LLMProvider): | |
| async def generate(self, prompt, max_tokens=1000, **kwargs): | |
| if "loves apples" in prompt and "hates apples" in prompt: | |
| old_id = prompt.split("- [")[1].split("]")[0] | |
| return f'{{"supersedes_id": "{old_id}"}}' | |
| return '{"supersedes_id": null}' | |
| async def test(): | |
| db = MemoryBackend() | |
| await db.initialize() | |
| embedder = MockEmbedder() | |
| llm = MockLLM() | |
| extractor = FactExtractor(db, embedder, llm) | |
| # insert first fact | |
| await extractor._process_facts( | |
| [{"text": "User loves apples"}], | |
| session_id="s1", user_id="u1", agent_id=None, conversation="User: I love apples" | |
| ) | |
| # insert second fact | |
| await extractor._process_facts( | |
| [{"text": "User hates apples"}], | |
| session_id="s2", user_id="u1", agent_id=None, conversation="User: Actually I hate apples" | |
| ) | |
| facts = await db.get_active_facts("u1") | |
| assert [f["text"] for f in facts] == ["User hates apples"] | |
| asyncio.run(test()) |
🧰 Tools
🪛 GitHub Actions: CI
[warning] 25-25: Ruff W293: Blank line contains whitespace. Remove whitespace from blank line.
[warning] 31-31: Ruff W293: Blank line contains whitespace. Remove whitespace from blank line.
[warning] 37-37: Ruff W293: Blank line contains whitespace. Remove whitespace from blank line.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test_conflict.py` around lines 15 - 43, The test never triggers the LLM-based
contradiction path because MockLLM.generate returns None; update
MockLLM.generate to return a deterministic, non-None response shaped like the
real LLM output that FactExtractor._check_dedup expects (e.g., the
classification/supersession verdict or JSON string it parses) so the
supersession branch runs, then modify the test to assert that the second
inserted fact was marked superseded or replaced (use
MemoryBackend.get_active_facts and any supersession flag) instead of only
printing results; reference MockLLM.generate, FactExtractor._check_dedup, and
MemoryBackend.get_active_facts to locate and change the code.
Comment on lines
+25
to
+39
| async def test(): | ||
| db = MemoryBackend() | ||
| await db.initialize() | ||
| embedder = MockEmbedder() | ||
| llm = MockLLM() | ||
| extractor = FactExtractor(db, embedder, llm) | ||
|
|
||
| await extractor._process_facts([{"text": "User loves apples"}], "s1", "u1", None, "") | ||
| await extractor._process_facts([{"text": "User hates apples"}], "s2", "u1", None, "") | ||
|
|
||
| facts = await db.get_active_facts("u1") | ||
| print("Active facts count:", len(facts)) | ||
| for f in facts: print("-", f["text"]) | ||
|
|
||
| asyncio.run(test()) |
There was a problem hiding this comment.
This is still a demo script, not an automated test.
The module executes immediately and logs the outcome instead of asserting it. If supersession stops working, nothing here will fail the suite.
Suggested direction
async def test():
@@
facts = await db.get_active_facts("u1")
- print("Active facts count:", len(facts))
- for f in facts: print("-", f["text"])
-
-asyncio.run(test())
+ assert [f["text"] for f in facts] == ["User hates apples"]📝 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
| async def test(): | |
| db = MemoryBackend() | |
| await db.initialize() | |
| embedder = MockEmbedder() | |
| llm = MockLLM() | |
| extractor = FactExtractor(db, embedder, llm) | |
| await extractor._process_facts([{"text": "User loves apples"}], "s1", "u1", None, "") | |
| await extractor._process_facts([{"text": "User hates apples"}], "s2", "u1", None, "") | |
| facts = await db.get_active_facts("u1") | |
| print("Active facts count:", len(facts)) | |
| for f in facts: print("-", f["text"]) | |
| asyncio.run(test()) | |
| async def test(): | |
| db = MemoryBackend() | |
| await db.initialize() | |
| embedder = MockEmbedder() | |
| llm = MockLLM() | |
| extractor = FactExtractor(db, embedder, llm) | |
| await extractor._process_facts([{"text": "User loves apples"}], "s1", "u1", None, "") | |
| await extractor._process_facts([{"text": "User hates apples"}], "s2", "u1", None, "") | |
| facts = await db.get_active_facts("u1") | |
| assert [f["text"] for f in facts] == ["User hates apples"] |
🧰 Tools
🪛 GitHub Actions: CI
[warning] 31-31: Ruff W293: Blank line contains whitespace. Remove whitespace from blank line.
[warning] 34-34: Ruff W293: Blank line contains whitespace. Remove whitespace from blank line.
[error] 37-37: Ruff E701: Multiple statements on one line (colon).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test_conflict2.py` around lines 25 - 39, The file defines an ad-hoc demo
async test that runs on import (async def test() + asyncio.run(test())) and only
prints results; convert this into a proper automated test that uses assertions
and does not execute on import: replace the top-level asyncio.run call with a
test function recognizable by the test runner (e.g., async def
test_fact_supersession() with `@pytest.mark.asyncio` or an asyncio-compatible
fixture), call MemoryBackend(), FactExtractor(...), await
extractor._process_facts(...), then assert expected outcomes using
db.get_active_facts("u1") (e.g., assert len(facts) == X and expected texts
present/absent) and remove any print statements so the module does not execute
when imported.
Comment on lines
+15
to
+33
| class MockLLM(LLMProvider): | ||
| async def generate(self, prompt, max_tokens=1000, **kwargs): | ||
| if "contradict" in prompt.lower(): | ||
| # If it has "hate", it contradicts the "love" one | ||
| if "hates apples" in prompt and "loves apples" in prompt: | ||
| return '{"contradicts": "1"}' | ||
| return '{"contradicts": null}' | ||
| pass | ||
|
|
||
| async def test(): | ||
| db = MemoryBackend() | ||
| await db.initialize() | ||
| embedder = MockEmbedder() | ||
| llm = MockLLM() | ||
| extractor = FactExtractor(db, embedder, llm) | ||
|
|
||
| # Needs actual logic in Extractor... | ||
|
|
||
| asyncio.run(test()) |
There was a problem hiding this comment.
The mock contract no longer matches the extractor.
_check_dedup() now looks for supersedes_id, but this mock returns contradicts, so the new LLM dedup branch would still behave like a no-op. The body is also empty, so this feature remains untested.
Minimal fix
class MockLLM(LLMProvider):
async def generate(self, prompt, max_tokens=1000, **kwargs):
if "contradict" in prompt.lower():
if "hates apples" in prompt and "loves apples" in prompt:
- return '{"contradicts": "1"}'
- return '{"contradicts": null}'
- pass
+ old_id = prompt.split("- [")[1].split("]")[0]
+ return f'{{"supersedes_id": "{old_id}"}}'
+ return '{"supersedes_id": null}'
+ return '{"supersedes_id": null}'🧰 Tools
🪛 GitHub Actions: CI
[error] 29-29: Ruff F841: Local variable extractor is assigned to but never used. Remove unused assignment.
[warning] 30-30: Ruff W293: Blank line contains whitespace. Remove whitespace from blank line.
[warning] 32-32: Ruff W293: Blank line contains whitespace. Remove whitespace from blank line.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test_llm_dedup_check.py` around lines 15 - 33, The MockLLM.generate mock no
longer matches the extractor's expected LLM contract: _check_dedup() now expects
a JSON key named "supersedes_id" but MockLLM.generate returns "contradicts" and
its body is effectively a no-op, so the dedup branch is untested; update
MockLLM.generate to parse the prompt conditions and return the JSON shape the
extractor expects (i.e., include "supersedes_id" with appropriate id or null),
and add a test prompt in async test() that triggers the extractor._check_dedup
path (use the same trigger text you check for in MockLLM.generate) so the dedup
behavior is exercised by the test harness (refer to MockLLM.generate,
_check_dedup, FactExtractor, and the test() entrypoint).
Comment on lines
+4
to
+26
| async def test(): | ||
| v = Vektori(storage_backend="memory") | ||
| await v._ensure_initialized() | ||
| # Mock LLM to just return a synthesis fact | ||
| async def mock_gen(*args, **kwargs): | ||
| return '{"facts": [{"text": "User loves fruit overall."}]}' | ||
| async def mock_emb(*args, **kwargs): | ||
| return [[0.1, 0.2]] | ||
| v.llm.generate = mock_gen | ||
| v.embedder.embed_batch = mock_emb | ||
|
|
||
| # add dummy facts so synthesize triggers (needs >=5 base facts) | ||
| for i in range(5): | ||
| await v.db.insert_fact(f"fact {i}", [0.1, 0.2], "u1", confidence=1.0) | ||
|
|
||
| n = await v.synthesize("u1") | ||
| print("New synthesized facts:", n) | ||
| facts = await v.db.get_active_facts("u1", limit=100) | ||
| for f in facts: | ||
| if f.get("metadata", {}).get("source") == "synthesis": | ||
| print("Synthesized:", f["text"]) | ||
|
|
||
| asyncio.run(test()) |
There was a problem hiding this comment.
Convert this import-time script into an actual test.
This module executes during import and only prints results, so synthesis regressions won’t fail CI. Please assert on the synthesized count and stored facts instead of relying on print().
Suggested direction
-async def test():
+async def test_synthesize_inserts_fact():
v = Vektori(storage_backend="memory")
await v._ensure_initialized()
@@
n = await v.synthesize("u1")
- print("New synthesized facts:", n)
+ assert n == 1
facts = await v.db.get_active_facts("u1", limit=100)
- for f in facts:
- if f.get("metadata", {}).get("source") == "synthesis":
- print("Synthesized:", f["text"])
-
-asyncio.run(test())
+ synthesized = [
+ f["text"]
+ for f in facts
+ if f.get("metadata", {}).get("source") == "synthesis"
+ ]
+ assert synthesized == ["User loves fruit overall."]🧰 Tools
🪛 GitHub Actions: CI
[warning] 14-14: Ruff W293: Blank line contains whitespace. Remove whitespace from blank line.
[warning] 18-18: Ruff W293: Blank line contains whitespace. Remove whitespace from blank line.
[warning] 25-25: Ruff W293: Blank line contains whitespace. Remove whitespace from blank line.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test_synthesis.py` around lines 4 - 26, The current file runs an import-time
script that prints results; convert it into a proper pytest async test that
asserts behavior. Replace the top-level async test() and asyncio.run call with a
pytest test function (e.g., use `@pytest.mark.asyncio` async def
test_synthesis_creates_and_stores():), instantiate Vektori and call await
v._ensure_initialized(), mock v.llm.generate and v.embedder.embed_batch as
before, insert the 5 dummy facts, call n = await v.synthesize("u1") and assert
on n (expected synthesized count/structure) and on facts = await
v.db.get_active_facts("u1", limit=100") asserting the number of facts with
metadata.source == "synthesis" and their text, and finally clean up/close v if
needed; reference Vektori, v._ensure_initialized, v.llm.generate,
v.embedder.embed_batch, v.db.insert_fact, v.synthesize, and
v.db.get_active_facts to locate code.
Comment on lines
+57
to
+59
| facts_list = "\n".join(f"- {f['text']} (Session: {f.get('session_id', 'unknown')}, Date: {f.get('created_at', 'unknown')})" for f in base_facts) | ||
|
|
||
| prompt = SYNTHESIS_PROMPT.format(facts_list=facts_list, max_facts=5) |
There was a problem hiding this comment.
session_id is missing from the Postgres facts payload here.
vektori/storage/postgres.py:323-340 only returns id, text, confidence, is_active, superseded_by, created_at, and metadata, so this backend will render every entry as Session: unknown. That removes the main signal the prompt is asking the model to use to avoid single-session syntheses.
🧰 Tools
🪛 GitHub Actions: CI
[warning] 58-58: Ruff W293: Blank line contains whitespace. Remove whitespace from blank line.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vektori/ingestion/synthesis.py` around lines 57 - 59, The synthesis step is
showing "Session: unknown" because Postgres facts don't include session_id;
update either the Postgres loader (vektori/storage/postgres.py) to include
session_id on each fact (extracting it from the metadata JSON and returning it
as the top-level "session_id" key) or change the builder in synthesis.py to read
session_id from f.get('metadata', {}).get('session_id') when constructing
facts_list (the generator using base_facts and SYNTHESIS_PROMPT), so the prompt
receives a proper Session value instead of "unknown".
Comment on lines
+83
to
+98
| existing = await self.db.search_facts( | ||
| embedding=emb, | ||
| user_id=user_id, | ||
| agent_id=agent_id, | ||
| limit=1, | ||
| active_only=True | ||
| ) | ||
|
|
||
| skip = False | ||
| if existing: | ||
| best = existing[0] | ||
| sim = 1.0 - best.get("distance", 1.0) | ||
| if sim > 0.85 and best.get("metadata", {}).get("source") == "synthesis": | ||
| # We already have this synthesis fact, let's just update mentions | ||
| await self.db.increment_fact_mentions(best["id"]) | ||
| skip = True |
There was a problem hiding this comment.
This can miss an existing synthesis fact and insert a duplicate.
The query only asks for the single nearest active fact across everything. If rank 1 is a base fact, a near-identical synthesis fact at rank 2+ is never examined, so this path inserts a duplicate instead of incrementing mentions. At minimum, mirror the extractor’s subject-scoped dedup and inspect a few candidates here.
Proposed fix
- existing = await self.db.search_facts(
+ candidates = await self.db.search_facts(
embedding=emb,
user_id=user_id,
agent_id=agent_id,
- limit=1,
+ subject="user",
+ limit=5,
active_only=True
)
skip = False
- if existing:
- best = existing[0]
- sim = 1.0 - best.get("distance", 1.0)
- if sim > 0.85 and best.get("metadata", {}).get("source") == "synthesis":
- # We already have this synthesis fact, let's just update mentions
- await self.db.increment_fact_mentions(best["id"])
- skip = True
+ synthesis_match = next(
+ (
+ fact
+ for fact in candidates
+ if fact.get("metadata", {}).get("source") == "synthesis"
+ and 1.0 - fact.get("distance", 1.0) > 0.85
+ ),
+ None,
+ )
+ if synthesis_match:
+ await self.db.increment_fact_mentions(synthesis_match["id"])
+ skip = True🧰 Tools
🪛 GitHub Actions: CI
[warning] 90-90: Ruff W293: Blank line contains whitespace. Remove whitespace from blank line.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vektori/ingestion/synthesis.py` around lines 83 - 98, The current dedup logic
uses self.db.search_facts with limit=1 so if the top match is a base fact you
can miss a near-identical synthesis fact and create a duplicate; change the
search to return multiple candidates (e.g., increase limit to a small handful
like 5), iterate the returned existing results to find the nearest candidate
whose metadata.source == "synthesis" and whose similarity (1.0 - distance) >
0.85, then call self.db.increment_fact_mentions(best["id"]) for that matching
candidate and set skip=True; use the same user_id/agent_id/active_only filtering
already passed to self.db.search_facts so scope matches the extractor’s
subject-scoped dedup.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
vektori/storage/postgres.py (1)
1006-1017:⚠️ Potential issue | 🟡 Minor
delete_user()double-counts syntheses.
synthesesis counted and deleted twice here, so the returned total is inflated by the number of synthesis rows for that user. Drop the duplicateCOUNTandDELETE.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektori/storage/postgres.py` around lines 1006 - 1017, In delete_user(), the total calculation and deletion mistakenly include syntheses twice: remove the duplicate "(SELECT COUNT(*) FROM syntheses WHERE user_id = $1)" from the total SQL so syntheses are counted only once, and remove the duplicated await conn.execute("DELETE FROM syntheses WHERE user_id = $1", user_id) call so syntheses are deleted only once; update the total assignment (counts["total"]) logic remains the same.vektori/storage/sqlite.py (1)
732-747:⚠️ Potential issue | 🟠 Major
get_sentences_by_ids()is defined again under Episodes.This helper already exists above, so this third definition only hides the earlier copies and adds another F811 failure. Keep a single shared implementation near the other fact-source helpers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektori/storage/sqlite.py` around lines 732 - 747, There is a duplicate definition of get_sentences_by_ids (causing F811) inside the Episodes section; remove this redundant implementation and consolidate usage to the single shared helper already defined earlier near the other fact-source helpers. Replace any local calls in the Episodes class or module to reference the existing get_sentences_by_ids function (or move the original implementation to a common helper module if needed), and run tests/lint to ensure the F811 duplicate-definition error is resolved.
♻️ Duplicate comments (2)
vektori/ingestion/synthesis.py (2)
71-81:⚠️ Potential issue | 🟠 MajorValidate LLM items before indexing
text.A single malformed element in
new_factswill raise onf["text"]and abort the whole synthesis batch. Filter to dicts with a non-empty stringtextand normalizeconfidencebefore callingembed_batch().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektori/ingestion/synthesis.py` around lines 71 - 81, Filter and normalize new_facts before calling self.embedder.embed_batch: iterate over new_facts and keep only items that are dicts with a non-empty string at key "text" (strip whitespace) and ensure a numeric "confidence" field exists (coerce or set a default, e.g., 1.0), build texts = [f["text"] for f in valid_facts]; if valid_facts is empty return 0; then call await self.embedder.embed_batch(texts) and iterate over zip(valid_facts, embeddings) (not the original new_facts) when inserting to preserve alignment between facts and their embeddings.
57-59:⚠️ Potential issue | 🟠 MajorPostgres still feeds
Session: unknowninto this prompt.
Synthesizerexpects each fact to carrysession_id, butvektori/storage/postgres.py:get_active_facts()does not return it. On Postgres every line here degrades tounknown, which weakens the “cross-session only” instruction. Either includesession_idin that query or fall back to a value stored inmetadata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektori/ingestion/synthesis.py` around lines 57 - 59, The prompt building in vektori/ingestion/synthesis.py uses base_facts expecting a session_id but Postgres's get_active_facts (vektori/storage/postgres.py:get_active_facts) doesn't return session_id so facts_list becomes "Session: unknown"; update get_active_facts to SELECT and return session_id for each fact, or if session_id isn't available populate it from the fact's metadata (e.g., metadata.get("session_id")) before synthesis, so the loop that formats facts_list (variable base_facts / SYNTHESIS_PROMPT) emits a real session identifier instead of "unknown".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vektori/ingestion/synthesis.py`:
- Around line 101-110: The code currently calls insert_synthesis (in
synthesis.py) but never writes the corresponding synthesis_facts edges, so
get_syntheses_for_facts(...) cannot find linked syntheses; update the flow to
persist the fact links immediately after creating the synthesis row by calling
whatever DB method writes synthesis_facts (or add one) using the returned
synthesis_id and the originating fact IDs in fact_dict, and only consider the
synthesis fully created after those edges are written; also stop using the
sentinel session_id="synthesis"—set session_id to NULL (or move that marker into
metadata) when calling insert_synthesis so the row is not misclassified as a
session-scoped artifact.
In `@vektori/storage/base.py`:
- Around line 199-254: Remove the duplicated "Syntheses" block and keep a single
API surface that matches callers: collapse the two copies into one set of
declarations for insert_synthesis, insert_synthesis_fact,
get_syntheses_for_facts, search_syntheses, get_source_sentences, and
get_sentences_by_ids; change insert_synthesis's signature to match the real
callsite (use session_id instead of score/mentions/metadata) so it aligns with
vektori/ingestion/synthesis.py and concrete backends, and ensure the abstract
helpers get_source_sentences and get_sentences_by_ids are declared only once.
In `@vektori/storage/memory.py`:
- Around line 386-476: The file contains a duplicate block redefining
synthesis/sentence helper methods (insert_synthesis, insert_synthesis_fact,
get_syntheses_for_facts, search_syntheses, insert_fact_source,
get_source_sentences, get_sentences_by_ids) which causes an F811 redefinition
error; remove the later duplicate definitions (the block starting at the second
insert_synthesis) so only the original implementations remain, ensuring any
tests or references still point to the first definitions and preserving imports
and private helpers like _cosine_similarity and _utcnow_naive.
In `@vektori/storage/postgres.py`:
- Around line 617-714: Remove the duplicate "syntheses" helper block that
redefines insert_synthesis, insert_synthesis_fact, get_syntheses_for_facts,
search_syntheses and get_source_sentences (these functions are already defined
earlier), leaving only the original implementations; locate the repeated
definitions by the function names above and delete the entire second block so
there are no duplicate function definitions that cause the F811 redefinition
error.
In `@vektori/storage/schema.sql`:
- Around line 148-152: The unique index idx_syntheses_user_text on the syntheses
table currently enforces uniqueness on (user_id, text) which breaks agent-scoped
search_syntheses(); update the schema to include agent_id in the uniqueness
constraint (make idx_syntheses_user_text cover (user_id, agent_id, text)) and
update any backend deterministic UUID generation logic to include agent_id when
computing the id so that inserts are idempotent per (user_id, agent_id, text)
rather than per user/text; ensure any code paths referencing
idx_syntheses_user_text or generating synthesis UUIDs (deterministic ID
function) are adjusted consistently.
In `@vektori/storage/sqlite.py`:
- Around line 586-666: The file contains a duplicate implementation of
synthesis-related methods (insert_synthesis, insert_synthesis_fact,
get_syntheses_for_facts, search_syntheses, and get_sentences_by_ids) which
causes a redefinition (F811); remove the second copy (the later block shown in
the diff) so only the original implementations remain, ensuring the unique
functions insert_synthesis, insert_synthesis_fact, get_syntheses_for_facts,
search_syntheses, and get_sentences_by_ids are defined once and the rest of the
class uses those originals.
- Around line 136-180: The synthesis_facts table definition incorrectly
references a non-existent table `synthesiss(id)` and there are duplicated CREATE
TABLE/CREATE INDEX statements; update the foreign key in the `synthesis_facts`
CREATE TABLE to reference the correct `syntheses(id)`, remove the duplicate
`CREATE TABLE IF NOT EXISTS synthesis_facts` and duplicate `CREATE INDEX IF NOT
EXISTS idx_synthesis_facts_fact` statements, and ensure only one canonical
`synthesis_facts` table and one index remain so foreign key constraints work
when PRAGMA foreign_keys=ON.
---
Outside diff comments:
In `@vektori/storage/postgres.py`:
- Around line 1006-1017: In delete_user(), the total calculation and deletion
mistakenly include syntheses twice: remove the duplicate "(SELECT COUNT(*) FROM
syntheses WHERE user_id = $1)" from the total SQL so syntheses are counted only
once, and remove the duplicated await conn.execute("DELETE FROM syntheses WHERE
user_id = $1", user_id) call so syntheses are deleted only once; update the
total assignment (counts["total"]) logic remains the same.
In `@vektori/storage/sqlite.py`:
- Around line 732-747: There is a duplicate definition of get_sentences_by_ids
(causing F811) inside the Episodes section; remove this redundant implementation
and consolidate usage to the single shared helper already defined earlier near
the other fact-source helpers. Replace any local calls in the Episodes class or
module to reference the existing get_sentences_by_ids function (or move the
original implementation to a common helper module if needed), and run tests/lint
to ensure the F811 duplicate-definition error is resolved.
---
Duplicate comments:
In `@vektori/ingestion/synthesis.py`:
- Around line 71-81: Filter and normalize new_facts before calling
self.embedder.embed_batch: iterate over new_facts and keep only items that are
dicts with a non-empty string at key "text" (strip whitespace) and ensure a
numeric "confidence" field exists (coerce or set a default, e.g., 1.0), build
texts = [f["text"] for f in valid_facts]; if valid_facts is empty return 0; then
call await self.embedder.embed_batch(texts) and iterate over zip(valid_facts,
embeddings) (not the original new_facts) when inserting to preserve alignment
between facts and their embeddings.
- Around line 57-59: The prompt building in vektori/ingestion/synthesis.py uses
base_facts expecting a session_id but Postgres's get_active_facts
(vektori/storage/postgres.py:get_active_facts) doesn't return session_id so
facts_list becomes "Session: unknown"; update get_active_facts to SELECT and
return session_id for each fact, or if session_id isn't available populate it
from the fact's metadata (e.g., metadata.get("session_id")) before synthesis, so
the loop that formats facts_list (variable base_facts / SYNTHESIS_PROMPT) emits
a real session identifier instead of "unknown".
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6075b8f7-3d1f-428c-bdc2-39551e050989
📒 Files selected for processing (10)
.gitignorebenchmarks/locomo/locomo_runner.pytest_synthesis.pyvektori/ingestion/synthesis.pyvektori/retrieval/search.pyvektori/storage/base.pyvektori/storage/memory.pyvektori/storage/postgres.pyvektori/storage/schema.sqlvektori/storage/sqlite.py
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- benchmarks/locomo/locomo_runner.py
- test_synthesis.py
Comment on lines
+148
to
+152
| CREATE INDEX IF NOT EXISTS idx_syntheses_user ON syntheses (user_id); | ||
| CREATE INDEX IF NOT EXISTS idx_syntheses_embedding ON syntheses | ||
| USING ivfflat (embedding vector_cosine_ops) WITH (lists = 100); | ||
| -- Dedup: same synthesis text for same user is idempotent | ||
| CREATE UNIQUE INDEX IF NOT EXISTS idx_syntheses_user_text ON syntheses (user_id, text); |
There was a problem hiding this comment.
Include agent_id in synthesis dedup/idempotency.
search_syntheses() is agent-scoped, but this uniqueness rule only keys on (user_id, text). If the same user gets the same synthesis text under two agents, the second insert is dropped and that agent cannot retrieve its own row. Include agent_id in the unique key and mirror that in the deterministic UUID generation used by the backends.
Possible schema direction
-CREATE UNIQUE INDEX IF NOT EXISTS idx_syntheses_user_text ON syntheses (user_id, text);
+CREATE UNIQUE INDEX IF NOT EXISTS idx_syntheses_user_agent_text ON syntheses (
+ user_id,
+ COALESCE(agent_id, ''),
+ text
+);📝 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
| CREATE INDEX IF NOT EXISTS idx_syntheses_user ON syntheses (user_id); | |
| CREATE INDEX IF NOT EXISTS idx_syntheses_embedding ON syntheses | |
| USING ivfflat (embedding vector_cosine_ops) WITH (lists = 100); | |
| -- Dedup: same synthesis text for same user is idempotent | |
| CREATE UNIQUE INDEX IF NOT EXISTS idx_syntheses_user_text ON syntheses (user_id, text); | |
| CREATE INDEX IF NOT EXISTS idx_syntheses_user ON syntheses (user_id); | |
| CREATE INDEX IF NOT EXISTS idx_syntheses_embedding ON syntheses | |
| USING ivfflat (embedding vector_cosine_ops) WITH (lists = 100); | |
| -- Dedup: same synthesis text for same user is idempotent | |
| CREATE UNIQUE INDEX IF NOT EXISTS idx_syntheses_user_agent_text ON syntheses ( | |
| user_id, | |
| COALESCE(agent_id, ''), | |
| text | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vektori/storage/schema.sql` around lines 148 - 152, The unique index
idx_syntheses_user_text on the syntheses table currently enforces uniqueness on
(user_id, text) which breaks agent-scoped search_syntheses(); update the schema
to include agent_id in the uniqueness constraint (make idx_syntheses_user_text
cover (user_id, agent_id, text)) and update any backend deterministic UUID
generation logic to include agent_id when computing the id so that inserts are
idempotent per (user_id, agent_id, text) rather than per user/text; ensure any
code paths referencing idx_syntheses_user_text or generating synthesis UUIDs
(deterministic ID function) are adjusted consistently.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vektori/storage/postgres.py (1)
889-900:⚠️ Potential issue | 🟡 Minor
delete_user()now overcounts syntheses.
synthesesis included twice in the total and deleted twice. The second delete is harmless, but the returnedtotalis inflated by the number of syntheses for that user, which breaks the method’s contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektori/storage/postgres.py` around lines 889 - 900, The delete_user() implementation double-counts and double-deletes syntheses; fix the SELECT that computes counts to sum each table once (sentences, facts, episodes, syntheses, sessions) so counts["total"] is correct, and remove the duplicated await conn.execute("DELETE FROM syntheses WHERE user_id = $1", user_id) so each table is deleted exactly once; update the SQL aggregate and the delete calls near delete_user, counts, and the conn.execute delete statements accordingly.
♻️ Duplicate comments (6)
vektori/storage/sqlite.py (1)
136-180:⚠️ Potential issue | 🔴 CriticalFix the
synthesis_factsforeign key target before relying on SQLite linkage.
synthesis_facts.synthesis_idstill referencessynthesiss(id), which does not exist. WithPRAGMA foreign_keys=ON, inserts intosynthesis_factscan fail, so synthesis linkage is broken on SQLite. The duplicatedCREATE TABLE/INDEX IF NOT EXISTSstatements should be removed while fixing this block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektori/storage/sqlite.py` around lines 136 - 180, The synthesis_facts table definition references a non-existent table name "synthesiss(id)" and there are duplicated CREATE TABLE/INDEX blocks; update the foreign key target in the synthesis_facts definition to reference syntheses(id), remove the duplicated CREATE TABLE IF NOT EXISTS for syntheses and synthesis_facts, and eliminate the duplicate INDEX IF NOT EXISTS lines (leave a single idx_synthesis_facts_fact index); ensure the final SQL uses the corrected reference and only one set of each CREATE TABLE/CREATE INDEX statement.vektori/ingestion/synthesis.py (3)
54-56:⚠️ Potential issue | 🟠 MajorDon’t build the prompt from a
session_idfield Postgres never loads.
vektori/storage/postgres.py:get_active_facts()currently returnsid,text,confidence,is_active,superseded_by,created_at, andmetadata, so this renders every Postgres fact asSession: unknown. That removes the multi-session signal the prompt depends on. Either returnsession_idfrom that backend call or fall back to wherever you persist it in metadata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektori/ingestion/synthesis.py` around lines 54 - 56, The prompt uses f.get('session_id') but Postgres backend (vektori/storage/postgres.py:get_active_facts) doesn’t return session_id, so facts_rendering in synthesis.py shows "Session: unknown"; either update get_active_facts() to include session_id in each returned fact or change synthesis.py to extract session_id from the returned fact's metadata (e.g., f.get('metadata', {}).get('session_id', 'unknown')) before building facts_list and passing to SYNTHESIS_PROMPT, ensuring you reference base_facts and preserve existing created_at fallback.
98-107:⚠️ Potential issue | 🟠 MajorPersist the synthesis↔fact links before treating this as inserted.
get_syntheses_for_facts()in the backends relies onsynthesis_facts, but this path only inserts the synthesis row and never callsinsert_synthesis_fact(). As written, graph-based retrieval stays empty and only direct vector search can surface these rows. Thesession_id="synthesis"sentinel is also misleading for a cross-session artifact; keep that marker in metadata or leavesession_idnull.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektori/ingestion/synthesis.py` around lines 98 - 107, The code inserts a synthesis via insert_synthesis but never creates the synthesis↔fact link, so get_syntheses_for_facts can't find it; after calling insert_synthesis (in this block around insert_synthesis), call insert_synthesis_fact with the new synthesis id and the originating fact id(s) to persist the relationship, and remove or null out the sentinel session_id="synthesis" (instead store that provenance in metadata or a dedicated field) so cross-session syntheses aren’t tied to a bogus session value.
68-78:⚠️ Potential issue | 🟠 MajorValidate synthesized items before indexing
text.
new_factsis untrusted model output. A single non-dict item, or a dict without a non-empty stringtext, will raise here and abort the whole synthesis pass before embedding. Filter and normalize the candidates first, then embed only the valid ones.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektori/ingestion/synthesis.py` around lines 68 - 78, Filter and normalize new_facts before calling embedder: ensure each item in new_facts is a dict with a non-empty string 'text' (strip whitespace), build a validated list (e.g., valid_facts) and corresponding texts list, return 0 early if none valid, then call self.embedder.embed_batch(texts) and iterate zip(valid_facts, embeddings) instead of the original new_facts; update any variable references used in the loop (fact_dict, emb, inserted) to use the validated list to avoid aborting the synthesis pass on bad model output.vektori/storage/base.py (1)
202-215:⚠️ Potential issue | 🟠 MajorKeep the synthesis backend contract aligned with its callers.
insert_synthesis()here still advertisesscore/mentions/metadata, but the new callsite invektori/ingestion/synthesis.pyand the concrete backends invektori/storage/{sqlite,memory,postgres}.pyall usesession_idinstead.search_syntheses()also addsthreshold, which those backends do not accept. The interface is still out of sync, so code written againstStorageBackendcan drift or fail once these methods are used through the shared contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektori/storage/base.py` around lines 202 - 215, The StorageBackend interface is out of sync: update insert_synthesis(...) to match callers/backends by replacing the deprecated parameters (score, mentions, metadata) with session_id: str | None = None and keep the existing params (text, embedding, user_id, agent_id) and return type; also ensure insert_synthesis_fact, get_syntheses_for_facts, and especially search_syntheses(embedding: list[float], user_id: str, agent_id: str | None = None, limit: int = 10, threshold: float = 0.0) signatures match the concrete backends (add the threshold param to the abstract search_syntheses) so all implementations (vektori/storage/sqlite.py, memory.py, postgres.py) and the caller (vektori/ingestion/synthesis.py) share the same contract.vektori/client.py (1)
172-191:⚠️ Potential issue | 🟠 MajorAuto-synthesis still races extraction and shutdown.
When
async_extractionis enabled, this task is spawned immediately afteringest()returns, butIngestionPipelinecan still be extracting and committing facts in the background. That means the synthesis pass can run on an incomplete fact set, andclose()still never cancels/awaitsself._bg_tasksbefore tearing down the worker and DB.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektori/client.py` around lines 172 - 191, The auto-synthesis task (_bg_synthesize) races with ongoing extraction and is never awaited or canceled on shutdown; modify ingest() / where the task is created so that the background synthesis is only scheduled after the IngestionPipeline has finished committing facts (or subscribe to a pipeline-completion event), attach tasks to self._bg_tasks as you already do, and update close() to cancel and await all tasks in self._bg_tasks before tearing down the DB; reference async_extraction, _bg_tasks, _bg_synthesize, ingest(), IngestionPipeline and close() when implementing the sequencing and shutdown handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@vektori/storage/postgres.py`:
- Around line 889-900: The delete_user() implementation double-counts and
double-deletes syntheses; fix the SELECT that computes counts to sum each table
once (sentences, facts, episodes, syntheses, sessions) so counts["total"] is
correct, and remove the duplicated await conn.execute("DELETE FROM syntheses
WHERE user_id = $1", user_id) so each table is deleted exactly once; update the
SQL aggregate and the delete calls near delete_user, counts, and the
conn.execute delete statements accordingly.
---
Duplicate comments:
In `@vektori/client.py`:
- Around line 172-191: The auto-synthesis task (_bg_synthesize) races with
ongoing extraction and is never awaited or canceled on shutdown; modify ingest()
/ where the task is created so that the background synthesis is only scheduled
after the IngestionPipeline has finished committing facts (or subscribe to a
pipeline-completion event), attach tasks to self._bg_tasks as you already do,
and update close() to cancel and await all tasks in self._bg_tasks before
tearing down the DB; reference async_extraction, _bg_tasks, _bg_synthesize,
ingest(), IngestionPipeline and close() when implementing the sequencing and
shutdown handling.
In `@vektori/ingestion/synthesis.py`:
- Around line 54-56: The prompt uses f.get('session_id') but Postgres backend
(vektori/storage/postgres.py:get_active_facts) doesn’t return session_id, so
facts_rendering in synthesis.py shows "Session: unknown"; either update
get_active_facts() to include session_id in each returned fact or change
synthesis.py to extract session_id from the returned fact's metadata (e.g.,
f.get('metadata', {}).get('session_id', 'unknown')) before building facts_list
and passing to SYNTHESIS_PROMPT, ensuring you reference base_facts and preserve
existing created_at fallback.
- Around line 98-107: The code inserts a synthesis via insert_synthesis but
never creates the synthesis↔fact link, so get_syntheses_for_facts can't find it;
after calling insert_synthesis (in this block around insert_synthesis), call
insert_synthesis_fact with the new synthesis id and the originating fact id(s)
to persist the relationship, and remove or null out the sentinel
session_id="synthesis" (instead store that provenance in metadata or a dedicated
field) so cross-session syntheses aren’t tied to a bogus session value.
- Around line 68-78: Filter and normalize new_facts before calling embedder:
ensure each item in new_facts is a dict with a non-empty string 'text' (strip
whitespace), build a validated list (e.g., valid_facts) and corresponding texts
list, return 0 early if none valid, then call self.embedder.embed_batch(texts)
and iterate zip(valid_facts, embeddings) instead of the original new_facts;
update any variable references used in the loop (fact_dict, emb, inserted) to
use the validated list to avoid aborting the synthesis pass on bad model output.
In `@vektori/storage/base.py`:
- Around line 202-215: The StorageBackend interface is out of sync: update
insert_synthesis(...) to match callers/backends by replacing the deprecated
parameters (score, mentions, metadata) with session_id: str | None = None and
keep the existing params (text, embedding, user_id, agent_id) and return type;
also ensure insert_synthesis_fact, get_syntheses_for_facts, and especially
search_syntheses(embedding: list[float], user_id: str, agent_id: str | None =
None, limit: int = 10, threshold: float = 0.0) signatures match the concrete
backends (add the threshold param to the abstract search_syntheses) so all
implementations (vektori/storage/sqlite.py, memory.py, postgres.py) and the
caller (vektori/ingestion/synthesis.py) share the same contract.
In `@vektori/storage/sqlite.py`:
- Around line 136-180: The synthesis_facts table definition references a
non-existent table name "synthesiss(id)" and there are duplicated CREATE
TABLE/INDEX blocks; update the foreign key target in the synthesis_facts
definition to reference syntheses(id), remove the duplicated CREATE TABLE IF NOT
EXISTS for syntheses and synthesis_facts, and eliminate the duplicate INDEX IF
NOT EXISTS lines (leave a single idx_synthesis_facts_fact index); ensure the
final SQL uses the corrected reference and only one set of each CREATE
TABLE/CREATE INDEX statement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e67f278a-dc9a-4818-ad0a-49b3ee12d24e
📒 Files selected for processing (10)
test_conflict.pytest_conflict2.pytest_llm_dedup_check.pytest_synthesis.pyvektori/client.pyvektori/ingestion/synthesis.pyvektori/storage/base.pyvektori/storage/memory.pyvektori/storage/postgres.pyvektori/storage/sqlite.py
✅ Files skipped from review due to trivial changes (2)
- test_conflict2.py
- test_synthesis.py
🚧 Files skipped from review as they are similar to previous changes (2)
- test_conflict.py
- test_llm_dedup_check.py
This was referenced Apr 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Improvements