Feat/benchmark query expansion#57
Conversation
- Add `add_document()` and `connect()` to Vektori client - Create `Connector` base protocol and `AuthStore` in `vektori/connectors/` - Implement `GitHubConnector` using PyGithub to pull issues and comments - Add example script `github_connector_demo.py` - Document integration strategy in `INTEGRATIONS_PLAN.md`
…ektori-ai/vektori into feat/benchmark-query-expansion
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR introduces a complete connector and authentication framework for external data sources, OpenAI-compatible LLM providers, shared QA utilities for answer generation, a GitHub connector for ingesting issues and PRs, and benchmark evaluation tools including a judge evaluator and GEPA prompt optimizer. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Connector
participant AS as AuthStore
participant API as GitHub API
participant V as Vektori Client
participant DB as Storage
GH->>AS: get_token(user_id, "github")
AS-->>GH: access_token
GH->>API: fetch issues (repo, since)
API-->>GH: issues with comments
loop for each issue
GH->>GH: build document (title + comments)
GH->>V: add_document(content, metadata)
V->>DB: store document
DB-->>V: ✓
end
GH-->>GH: return count
sequenceDiagram
participant Runner as LoCoMo Runner
participant QA as QA Module
participant LLM as LLM Provider
participant DB as Storage
Runner->>DB: retrieve context
DB-->>Runner: facts + syntheses
Runner->>Runner: format context (normalize timestamps, sort, suffix temporal notes)
Runner->>QA: build_qa_prompt(question, context, date)
QA-->>Runner: final prompt
Runner->>QA: generate_answer(prompt, model, max_tokens)
QA->>LLM: generate(prompt, max_tokens)
LLM-->>QA: response text
QA->>QA: strip result
QA-->>Runner: answer
sequenceDiagram
participant Judge as Judge Evaluator
participant JSON as Input File
participant LLM as LLM Provider
participant Parser as Parser
participant Stats as Summarizer
Judge->>JSON: load entries (filter by QID, sample)
JSON-->>Judge: QA results list
loop for each entry
Judge->>Judge: build judge prompt
Judge->>LLM: generate(prompt)
LLM-->>Judge: verdict JSON/text
Judge->>Parser: parse_verdict(response)
Parser->>Parser: normalize + validate verdict
Parser->>Parser: detect abstention
Parser-->>Judge: JudgeVerdict
end
Judge->>Stats: summarize(results)
Stats->>Stats: compute aggregate counts & rates
Stats-->>Judge: summary stats
Judge->>JSON: write output (config, summary, results)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR introduces multiple interconnected systems (connectors, authentication, LLM providers, QA utilities, benchmark tools) with substantial new logic. While individual components are relatively focused, the heterogeneity of changes across packages, async operations, external API integration, and multi-stage evaluation pipelines necessitates careful review of integration points, error handling, and correctness in new scoring/formatting logic. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ 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: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (15)
tests/unit/test_factory.py-53-64 (1)
53-64:⚠️ Potential issue | 🟡 MinorTests will flake if
VLLM_BASE_URL/LMSTUDIO_BASE_URLare set in the environment.Both assertions on
base_urlpin the hardcoded localhost default, butVLLMLLM.__init__/LMStudioLLM.__init__read those env vars first. On any developer machine or CI runner where they're set (intentionally or inherited), these tests will fail with a base_url mismatch. Usemonkeypatch.delenv(..., raising=False)— or acceptpytest'smonkeypatchfixture — to isolate.🧪 Proposed fix
-def test_create_vllm_llm(): +def test_create_vllm_llm(monkeypatch): + monkeypatch.delenv("VLLM_BASE_URL", raising=False) + monkeypatch.delenv("OPENAI_COMPAT_BASE_URL", raising=False) llm = create_llm("vllm:Qwen/Qwen3-8B") assert isinstance(llm, VLLMLLM) assert llm.model == "Qwen/Qwen3-8B" assert llm.base_url == "http://localhost:8000/v1" -def test_create_lmstudio_llm(): +def test_create_lmstudio_llm(monkeypatch): + monkeypatch.delenv("LMSTUDIO_BASE_URL", raising=False) + monkeypatch.delenv("OPENAI_COMPAT_BASE_URL", raising=False) llm = create_llm("lmstudio:qwen3-8b") assert isinstance(llm, LMStudioLLM) assert llm.model == "qwen3-8b" assert llm.base_url == "http://localhost:1234/v1"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_factory.py` around lines 53 - 64, The tests assume default localhost base URLs but VLLMLLM.__init__ and LMStudioLLM.__init__ read VLLM_BASE_URL and LMSTUDIO_BASE_URL from the environment, causing flakes; update the tests test_create_vllm_llm and test_create_lmstudio_llm to remove those env vars before creating the LLMs (use pytest's monkeypatch fixture and call monkeypatch.delenv("VLLM_BASE_URL", raising=False) and monkeypatch.delenv("LMSTUDIO_BASE_URL", raising=False) respectively) so create_llm returns instances with the hardcoded defaults and the base_url assertions become deterministic.vektori/connectors/auth.py-28-32 (1)
28-32:⚠️ Potential issue | 🟡 MinorSilent swallowing of
JSONDecodeErrorhides corrupt token files.Returning
NoneonJSONDecodeErrormakes a corrupt credential file indistinguishable from a missing one — the connector will just log "no token" and skip ingestion forever. Logging a warning here (or re-raising) would make this debuggable.except json.JSONDecodeError: + # Corrupt credential file — surface it so callers can act. + import logging + logging.getLogger(__name__).warning( + "Corrupt token file at %s; ignoring", path + ) return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektori/connectors/auth.py` around lines 28 - 32, The current catch of json.JSONDecodeError silently returns None for a corrupt token file (the with open(path, "r", encoding="utf-8") as f: return json.load(f) block), making corruption indistinguishable from a missing token; change the except to log a warning that includes the file path and the exception details (e.g., logging.warning or module logger) so callers can distinguish and debug corrupt credentials — alternatively re-raise the JSONDecodeError after logging if upstream handling is preferred.vektori/utils/async_worker.py-108-123 (1)
108-123:⚠️ Potential issue | 🟡 Minor
timeoutis not honored on the fallback_processpath.When
_timers.get(key)returnsNonebut the buffer still has pending requests,await self._process(key)is called directly without any timeout wrapper._processinvokes the extractor (LLM calls gated only by the semaphore), which can stall much longer than the caller'stimeout=60.0. Since_auto_synthesize_if_duedepends on bounded idleness here, this can block the background synthesis loop indefinitely on a slow extractor.🛠️ Proposed fix
task = self._timers.get(key) if task is None: if key in self._buffers: - await self._process(key) + try: + await asyncio.wait_for( + asyncio.shield(asyncio.create_task(self._process(key))), + timeout=timeout, + ) + except asyncio.TimeoutError: + logger.warning( + "Timed out draining extraction buffer for key=%s", key + ) return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektori/utils/async_worker.py` around lines 108 - 123, When _timers.get(key) is None but self._buffers[key] exists we must enforce the same timeout used for waiting on the timer; replace the direct call await self._process(key) with a guarded call using asyncio.wait_for and asyncio.shield (e.g., await asyncio.wait_for(asyncio.shield(self._process(key)), timeout=timeout)), and handle asyncio.TimeoutError and asyncio.CancelledError exactly as the timer branch does (log the timeout warning and return, treat cancelled tasks the same). Update the loop containing _timers, _buffers, _process, timeout and the existing except handlers to apply this timeout wrapper on the fallback path so the background synthesis loop cannot be blocked indefinitely.examples/github_connector_demo.py-55-57 (1)
55-57:⚠️ Potential issue | 🟡 Minor
fact['content']raises KeyError — facts usetextfield.Facts returned from storage backends use
textas the content field. Across the codebase—invektori/storage/,vektori/retrieval/scoring.py(line 189),vektori/cli.py, and other examples—facts are accessed viafact['text']orfact.get('text'). Usingfact['content']will raiseKeyErrorwhen any fact is returned, caught silently by the broadexcept Exceptionhandler above.Suggested fix
- for fact in results.get("facts", []): - print(f"- {fact['content']}") + for fact in results.get("facts", []): + print(f"- {fact.get('text', '')}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/github_connector_demo.py` around lines 55 - 57, The loop that prints extracted facts uses fact['content'], which causes KeyError because stored facts use the 'text' field; update the printing logic (the loop over results.get("facts", []) in examples/github_connector_demo.py) to read the text safely using fact.get('text') with a fallback to fact.get('content') or an empty string (e.g., use a variable like text = fact.get('text', fact.get('content', '')) and print that) so missing keys won't raise exceptions.benchmarks/locomo/locomo_judge.py-290-316 (1)
290-316:⚠️ Potential issue | 🟡 MinorDead
ValueErrorhandler around_parse_qids.
_parse_qidsdoesn't raiseValueErroron any branch — the only exception it can raise isFileNotFoundError/OSErrorfromPath(qids_file).read_text(...). Thetry/except ValueErrorwrapper inmain()therefore never triggers. Either drop the wrapper or make_parse_qidsvalidate and raise (e.g., whenqids_filepath doesn't exist, wrap it asValueErrorwith a friendlier message).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/locomo/locomo_judge.py` around lines 290 - 316, The try/except in main is dead because _parse_qids never raises ValueError; update _parse_qids to catch file read errors (FileNotFoundError/OSError) when calling Path(qids_file).read_text(...) and re-raise a ValueError with a clear, user-facing message (include the original exception as the cause) so the existing parser.error in main will be triggered, or alternatively remove the try/except in main—reference the _parse_qids function and the main() caller when making the change.vektori/storage/schema.sql-132-167 (1)
132-167:⚠️ Potential issue | 🟡 MinorCopy/paste leftovers in the new synthesis section headers.
Two small cosmetic bugs in the new block headers that will confuse future readers:
- Lines 132–133: there are two consecutive
-- ============================================================lines — the original delimiter for the EPISODES section was not removed when the SYNTHESES section was inserted above it.- Lines 156–158: the banner says
EPISODE_FACTS: Links syntheses (L1) to the facts (L0)…but the table being defined issynthesis_facts. The heading should match the table name.✏️ Proposed fix
-- ============================================================ --- ============================================================ -- SYNTHESES: The middle layer (L1). LLM-generated episodic memory narratives. -- Discovered via graph traversal from matched facts, also directly vector-searched. -- ============================================================ @@ -- ============================================================ --- EPISODE_FACTS: Links syntheses (L1) to the facts (L0) they were derived from. +-- SYNTHESIS_FACTS: Links syntheses (L1) to the facts (L0) they were derived from. -- Graph edge: traversed after L0 vector search to surface syntheses. -- ============================================================🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektori/storage/schema.sql` around lines 132 - 167, Remove the stray duplicate delimiter line before the syntheses section and update the comment banner above the synthesis_facts table so the heading matches the table name; specifically, delete the extra "-- ============================================================" preceding the "SYNTHESES" block and change the "EPISODE_FACTS: Links syntheses (L1) to the facts (L0)..." banner to "SYNTHESIS_FACTS: Links syntheses (L1) to the facts (L0)..." so the comment aligns with the synthesis_facts table and syntheses/create statements.vektori/storage/sqlite.py-137-147 (1)
137-147:⚠️ Potential issue | 🟡 Minor
UNIQUE (user_id, agent_id, text)doesn't fire whenagent_idisNULL.SQLite (and ANSI SQL) treat
NULLs as distinct in unique constraints, so inserting the same(user_id, NULL, text)twice will not be rejected by this constraint. In practice you're shielded by the deterministicuuid5primary key (Line 521) so a second insert collides on the PK instead. Still, Postgres's migration explicitly usesCOALESCE(agent_id, '')(postgres.py Line 119) to close this gap, and it's worth doing the same here so the constraint semantics match across backends — e.g., either makeagent_idNOT NULL DEFAULT ''at the table level or replace the composite unique with an expression index.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektori/storage/sqlite.py` around lines 137 - 147, The UNIQUE constraint on the syntheses table (UNIQUE (user_id, agent_id, text)) does not enforce uniqueness when agent_id is NULL; change the schema to match Postgres behavior by either making agent_id NOT NULL DEFAULT '' in the CREATE TABLE for syntheses or replace the composite UNIQUE with a unique expression index using COALESCE(agent_id, '') (mirroring the approach used in postgres.py around the COALESCE(agent_id, '') usage) so that rows with NULL agent_id are treated as empty string and the uniqueness check will fire.vektori/client.py-198-203 (1)
198-203:⚠️ Potential issue | 🟡 MinorAuto-synthesis trigger is racy and may run concurrently with itself.
_auto_synthesize_if_duereadscount_sessionsand runssynthesize(...)without any per-user lock. If twoadd()calls for the same user both schedule_bg_synthesize_after_extractionaround the same session-count boundary, both may observecount % interval == 0(e.g., both read count=5 before either finishes) and you end up with two concurrentSynthesizer.synthesizeruns competing on the same underlying facts — duplicated LLM spend and racinginsert_synthesiscalls (Postgres hasON CONFLICT DO NOTHING, SQLite usesINSERT OR IGNORE, but the memory backend de-dupes by key so it's mostly tolerated — still wasted work).A lightweight guard is a per-
(user_id, agent_id)asyncio.Lockheld for the duration of the synthesis call, so concurrent triggers collapse into one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektori/client.py` around lines 198 - 203, Race conditions can cause duplicate concurrent synthesizes; protect the check-and-run in _auto_synthesize_if_due by introducing a per-(user_id,agent_id) asyncio.Lock stored on the client (e.g., self._synthesis_locks mapping) and acquire that lock before reading count_sessions and possibly calling synthesize so only one coroutine can observe and act on the boundary; release the lock after synthesize finishes and optionally clean up the lock entry. Update callers such as _bg_synthesize_after_extraction to rely on the existing _auto_synthesize_if_due behavior without adding additional locking.vektori/storage/postgres.py-627-627 (1)
627-627:⚠️ Potential issue | 🟡 MinorSection header comment mislabels the Episodes block.
Line 525 starts
# ── Syntheses ──(correct, followed by synthesis methods). Line 627 uses the same heading but the methods below it (insert_episode,insert_episode_fact,get_episodes_for_facts,search_episodes) are for Episodes. The same duplication exists invektori/storage/sqlite.py(Line 595) andvektori/storage/memory.py(Line 387). Rename all three to# ── Episodes ──.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektori/storage/postgres.py` at line 627, The section header comment currently labeled "# ── Syntheses ──" is incorrect for the block containing episode-related methods; rename that header to "# ── Episodes ──" wherever it appears above the episode functions (e.g., the block that precedes insert_episode, insert_episode_fact, get_episodes_for_facts, and search_episodes). Make the same change in the other modules that duplicate the wrong header so the comment accurately reflects the Episode methods.vektori/connectors/github.py-42-48 (1)
42-48:⚠️ Potential issue | 🟡 MinorUse the canonical import path for
GithubException.Change
from github.GithubException import GithubExceptiontofrom github import GithubException. PyGithub re-exports this exception at the top-level module, which is the documented canonical import path. The nested form depends on internal module structure that is not guaranteed to remain stable across versions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektori/connectors/github.py` around lines 42 - 48, Replace the non-canonical nested import of GithubException with the top-level import; in the import block where you currently have "from github import Github" and "from github.GithubException import GithubException", change the latter to "from github import GithubException" so the code uses the documented, stable export for the exception (update any references to GithubException accordingly).vektori/ingestion/extractor.py-875-890 (1)
875-890:⚠️ Potential issue | 🟡 MinorCross-session assumption when LLM flags a same-session contradiction.
On Line 888 the contradiction branch always returns
(supersedes_id, False), which_process_factsinterprets as cross-session near-dup → insert new + deactivate old. If the LLM picks a candidate that belongs to the same session (it's included incandidatesand therefore inplausible_conflicts), you still deactivate the earlier fact silently and insert a new one, which is a different behavior than the standard same-session dedup path (which increments mentions on the existing fact instead of inserting).Consider preserving the real
same_sessionvalue:🛠️ Suggested change
- if supersedes_id and any(c["id"] == supersedes_id for c in plausible_conflicts): - return (supersedes_id, False) + if supersedes_id: + match = next( + (c for c in plausible_conflicts if c["id"] == supersedes_id), + None, + ) + if match is not None: + return (supersedes_id, match.get("session_id") == session_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektori/ingestion/extractor.py` around lines 875 - 890, The contradiction branch currently always returns (supersedes_id, False) which forces cross-session behavior; change it to preserve the actual same_session flag from the chosen candidate. After you get supersedes_id from _parse_json_response, look up the matched candidate in plausible_conflicts (or candidates) and extract its same_session value (e.g. c.get("same_session", False)); then return (supersedes_id, same_session) so _process_facts sees the real same-session semantics. Ensure you default to False if the candidate is not found.vektori/storage/postgres.py-115-121 (1)
115-121:⚠️ Potential issue | 🟡 MinorGuard
synthesesindex operations against missing table.
_migrateunconditionally drops and recreates the index onsyntheseswithout verifying the table exists. Whileschema.sqlcreates it first in the normal flow, this breaks for old databases from before this PR or ifschema.sqlis modified without the table. Other migrations guard against missing columns (lines 99–112); apply the same pattern here:🛠️ Suggested guard
- await conn.execute("DROP INDEX IF EXISTS idx_syntheses_user_text") - await conn.execute( - """ - CREATE UNIQUE INDEX IF NOT EXISTS idx_syntheses_user_agent_text - ON syntheses (user_id, COALESCE(agent_id, ''), text) - """ - ) + has_syntheses = await conn.fetchval( + "SELECT to_regclass('public.syntheses') IS NOT NULL" + ) + if has_syntheses: + await conn.execute("DROP INDEX IF EXISTS idx_syntheses_user_text") + await conn.execute( + """ + 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/postgres.py` around lines 115 - 121, The index drop/create for the syntheses table in _migrate should be guarded by verifying the syntheses table exists before executing DROP INDEX / CREATE UNIQUE INDEX; modify the _migrate implementation to first check table existence (e.g., using a query like SELECT EXISTS FROM information_schema.tables WHERE table_name = 'syntheses' or SELECT to_regclass('syntheses')) and only run the two await conn.execute(...) statements for idx_syntheses_user_text and idx_syntheses_user_agent_text when that check returns true, matching the pattern used around lines 99–112 for other schema guards.vektori/connectors/github.py-88-88 (1)
88-88:⚠️ Potential issue | 🟡 Minor
.replace(tzinfo=timezone.utc)mislabels instead of converting for PyGithub 2.0+.
datetime.replace(tzinfo=...)only overwrites the tzinfo attribute — it does not convert the wall-clock time. PyGithub 2.0+ (current versions) returns timezone-aware datetimes (typically UTC). Applying.replace()stamps it as UTC without adjusting, creating incorrect timestamps. Safer:🛠️ Suggested fix
- document_time = issue.updated_at.replace(tzinfo=timezone.utc) + updated = issue.updated_at + document_time = ( + updated.replace(tzinfo=timezone.utc) + if updated.tzinfo is None + else updated.astimezone(timezone.utc) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vektori/connectors/github.py` at line 88, The code currently uses issue.updated_at.replace(tzinfo=timezone.utc) which mislabels rather than converts timezones; change this to convert properly by using issue.updated_at.astimezone(timezone.utc), and to be defensive handle naive datetimes by checking issue.updated_at.tzinfo is None and only then use replace(tzinfo=timezone.utc) or prefer parsing into an aware UTC datetime before converting; update the assignment to document_time to perform this correct conversion so timestamps are accurate.scripts/gepa_optimize_locomo_qa.py-196-219 (1)
196-219:⚠️ Potential issue | 🟡 Minor
_canonicalize_datespatterns are greedy on arbitrary word + year text.The second and third regexes (
r"\b(\d{1,2})(?:st|nd|rd|th)?\s+([a-z]+),?\s+(\d{4})\b"and its mirror) match any lowercase word between a small number and a 4-digit number, not just month names. Non-date phrases like "3 people 2024" or "15 songs 2020" get passed to_safe_iso(2024, 0, 15), faildatetime(...), and fall through to the"{year:04d} {month:02d} {day:02d}"branch — producing"2024 00 15"in the normalized string, which then influences F1/containment on unrelated answers.Cheap fix: only run
_safe_isowhen the token is a known month (otherwise leave the original substring untouched).🛡️ Proposed fix
- text = re.sub( - r"\b(\d{1,2})(?:st|nd|rd|th)?\s+([a-z]+),?\s+(\d{4})\b", - lambda m: _safe_iso(int(m.group(3)), MONTHS.get(m.group(2), 0), int(m.group(1))), - text, - ) - text = re.sub( - r"\b([a-z]+)\s+(\d{1,2})(?:st|nd|rd|th)?,?\s+(\d{4})\b", - lambda m: _safe_iso(int(m.group(3)), MONTHS.get(m.group(1), 0), int(m.group(2))), - text, - ) + def _dmy(m: re.Match[str]) -> str: + month = MONTHS.get(m.group(2), 0) + return _safe_iso(int(m.group(3)), month, int(m.group(1))) if month else m.group(0) + + def _mdy(m: re.Match[str]) -> str: + month = MONTHS.get(m.group(1), 0) + return _safe_iso(int(m.group(3)), month, int(m.group(2))) if month else m.group(0) + + text = re.sub(r"\b(\d{1,2})(?:st|nd|rd|th)?\s+([a-z]+),?\s+(\d{4})\b", _dmy, text) + text = re.sub(r"\b([a-z]+)\s+(\d{1,2})(?:st|nd|rd|th)?,?\s+(\d{4})\b", _mdy, text)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/gepa_optimize_locomo_qa.py` around lines 196 - 219, The regexes in _canonicalize_dates are replacing any word between a day and a 4-digit year even when it's not a month, causing MONTHS.get(...) to return 0 and _safe_iso to emit invalid "YYYY 00 DD" strings; update the two lambda handlers in _canonicalize_dates to first lookup the month token from MONTHS (using .lower() if needed) and only call _safe_iso when the lookup yields a valid month number, otherwise return the original matched substring (m.group(0)); keep _safe_iso unchanged and reference MONTHS, the second regex r"\b(\d{1,2})(?:st|nd|rd|th)?\s+([a-z]+),?\s+(\d{4})\b" (month in group 2) and the third regex r"\b([a-z]+)\s+(\d{1,2})(?:st|nd|rd|th)?,?\s+(\d{4})\b" (month in group 1) when implementing the check.scripts/gepa_optimize_locomo_qa.py-233-238 (1)
233-238:⚠️ Potential issue | 🟡 MinorAdd type validation to
load_dataset_metadatato handle unexpected JSON shapes.The function iterates over the loaded JSON with
for row in rowswithout verifying it's a list. Ifdata/locomo10_cooked.json(or any alternative dataset path) has a dict root, this causes aTypeErroror produces wrong results. The proposed guard is appropriate:🛡️ Suggested fix
with dataset_path.open("r", encoding="utf-8") as f: rows = json.load(f) - return {row["question_id"]: row for row in rows if row.get("question_id")} + if not isinstance(rows, list): + raise ValueError( + f"Expected a JSON list of question rows in {dataset_path}, got {type(rows).__name__}" + ) + return {row["question_id"]: row for row in rows if row.get("question_id")}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/gepa_optimize_locomo_qa.py` around lines 233 - 238, The loader assumes the JSON root is a list but may be a dict; update load_dataset_metadata to validate that the parsed JSON (rows) is a list before iterating. Specifically, after json.load(f) check isinstance(rows, list) and if it's a dict either convert it to an iterable of records (e.g., rows = list(rows.values())) or safely return {} / raise a clear ValueError; ensure the function still filters by row.get("question_id") and keep the return shape of dict[str, dict[str, Any]]. Use the function name load_dataset_metadata and variable rows when making the change so the guard is applied exactly where parsing occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/gepa_optimize_locomo_qa.py`:
- Around line 285-294: The current split logic can let val overlap train when
len(shuffled) <= train_size; modify the split so train and val are disjoint:
after checking len(shuffled) >= 2, compute effective_train = min(train_size,
len(shuffled) - min(val_size, len(shuffled) - 1)) (or simply ensure train_size +
val_size <= len(shuffled>) and adjust sizes), then set train =
shuffled[:effective_train] and val = shuffled[effective_train:effective_train +
val_size] (with a fallback that reduces val_size but still slices from the tail
after effective_train) so val is always taken from a disjoint tail; update the
checks around train and val creation (variables: shuffled, train_size, val_size,
train, val) accordingly.
- Around line 297-324: LocomoQAAdapter currently creates the LLM in __init__
(self.llm = create_llm(...)) but calls asyncio.run(self._generate_batch(...))
inside evaluate, which causes the LLM's async HTTP client to be bound to a
different event loop on repeated evaluate calls; to fix, defer creation of the
async LLM client into an async context (e.g., create the LLM inside
_generate_batch or on first await) or else make the adapter own a single event
loop and run all async work on that loop; update __init__, evaluate, and
_generate_batch so that self.llm is either lazily instantiated inside
_generate_batch (so self.llm.generate uses the currently running loop) or create
a dedicated loop and run _generate_batch on it to avoid cross-loop binding
errors when calling self.llm.generate.
In `@vektori/connectors/auth.py`:
- Around line 34-54: The set_token method currently writes tokens with default
permissions; change it to create the parent directory (used by _get_path) with
mode 0o700 and write the token file atomically with file mode 0o600: create a
temp file in the same directory, write JSON to it, fsync, set permissions to
0o600 (either via os.open with O_CREAT|O_WRONLY|O_TRUNC and mode 0o600 or by
os.chmod on the temp file), then os.replace the temp file to the final path so
replaces are atomic; ensure any helper that reads tokens (e.g., get_token)
continues to use _get_path and will see either the old or new complete file, not
a truncated one.
In `@vektori/connectors/base.py`:
- Around line 24-59: The Connector Protocol currently requires an optional watch
method which forces stubs like GitHubConnector to exist while Vektori.connect
only uses ingest and ConnectorResult is unused; fix by removing watch from
Connector and creating a new ObservableConnector(Connector, Protocol) that
defines async def watch(self, user_id: str, callback: Callable[[Any], None]) ->
None so only true observable connectors implement it, update any concrete
classes that currently implement watch (e.g., GitHubConnector) to either
implement ObservableConnector or drop the method, and then either remove or wire
ConnectorResult through Vektori.connect so return types are consistent (ensure
Vektori.connect returns ConnectorResult if you choose to keep it, or delete
ConnectorResult if unused).
In `@vektori/connectors/github.py`:
- Around line 32-114: The ingest function performs synchronous PyGithub calls
(Github, g.get_repo, repo.get_issues, iterating issues, issue.get_comments and
attribute access) inside an async context which blocks the event loop; fix by
moving all PyGithub I/O into a synchronous helper (e.g.
_fetch_issue_payloads(token, repo_name, since)) that collects plain serializable
payloads (issue number, title, state, body, author login, labels, is_pr flag,
updated_at, and a list of comment tuples), call that helper via
asyncio.to_thread or run_in_executor from ingest, then iterate the returned
payloads in ingest and await vektori.add_document(...) for each item; ensure you
still catch GithubException around the thread call and preserve metadata keys
("repo", "issue_number", "state", "author", "labels", "is_pr") when constructing
documents.
In `@vektori/ingestion/extractor.py`:
- Around line 790-792: replay_facts now calls _check_dedup which may invoke the
LLM for contradiction-checks, breaking the "No LLM call" guarantee; add an
optional parameter use_llm_contradiction: bool = True to _check_dedup and gate
the contradiction/LLM branch on it, then call _check_dedup(...,
use_llm_contradiction=False) from replay_facts (or alternatively update the
replay_facts docstring to reflect the LLM behavior if you prefer not to change
runtime behavior); ensure the new parameter is propagated through any internal
callers of _check_dedup and that tests/docstrings are updated to match.
In `@vektori/ingestion/synthesis.py`:
- Around line 91-123: The current loop uses a single source_fact_ids list for
all syntheses which creates N×M links; change to compute per-synthesis source
IDs by either (A) reading the source indices/IDs returned in each synthesis item
from the LLM (if fact_dict contains e.g. "source_ids" or "source_indices", use
those) or (B) compute semantic nearest base facts for each synthesis embedding
by computing cosine similarity between emb and each base_fact["embedding"] (use
base_facts entries and their "id" and "embedding"), then select a capped set
(e.g., top_k=5) and optionally apply a similarity threshold (e.g., >0.75); pass
that per-synthesis list to insert_synthesis_fact instead of the global
source_fact_ids; update variables referenced here: source_fact_ids, valid_facts,
embeddings, insert_synthesis, insert_synthesis_fact, and search_syntheses.
In `@vektori/retrieval/scoring.py`:
- Line 13: Add a new float parameter preference_decay_rate (default 0.0001) to
Vektori.__init__ and accept/pass it into SearchPipeline.__init__; store it on
the SearchPipeline instance so it’s available to scoring. Update
SearchPipeline.__init__ signature to accept preference_decay_rate and forward it
into the internal calls that invoke score_and_rank. Finally, update the three
score_and_rank invocations in search.py to pass preference_decay_rate through
(alongside temporal_decay_rate) so the scoring function in scoring.py receives
the tunable value.
---
Minor comments:
In `@benchmarks/locomo/locomo_judge.py`:
- Around line 290-316: The try/except in main is dead because _parse_qids never
raises ValueError; update _parse_qids to catch file read errors
(FileNotFoundError/OSError) when calling Path(qids_file).read_text(...) and
re-raise a ValueError with a clear, user-facing message (include the original
exception as the cause) so the existing parser.error in main will be triggered,
or alternatively remove the try/except in main—reference the _parse_qids
function and the main() caller when making the change.
In `@examples/github_connector_demo.py`:
- Around line 55-57: The loop that prints extracted facts uses fact['content'],
which causes KeyError because stored facts use the 'text' field; update the
printing logic (the loop over results.get("facts", []) in
examples/github_connector_demo.py) to read the text safely using
fact.get('text') with a fallback to fact.get('content') or an empty string
(e.g., use a variable like text = fact.get('text', fact.get('content', '')) and
print that) so missing keys won't raise exceptions.
In `@scripts/gepa_optimize_locomo_qa.py`:
- Around line 196-219: The regexes in _canonicalize_dates are replacing any word
between a day and a 4-digit year even when it's not a month, causing
MONTHS.get(...) to return 0 and _safe_iso to emit invalid "YYYY 00 DD" strings;
update the two lambda handlers in _canonicalize_dates to first lookup the month
token from MONTHS (using .lower() if needed) and only call _safe_iso when the
lookup yields a valid month number, otherwise return the original matched
substring (m.group(0)); keep _safe_iso unchanged and reference MONTHS, the
second regex r"\b(\d{1,2})(?:st|nd|rd|th)?\s+([a-z]+),?\s+(\d{4})\b" (month in
group 2) and the third regex
r"\b([a-z]+)\s+(\d{1,2})(?:st|nd|rd|th)?,?\s+(\d{4})\b" (month in group 1) when
implementing the check.
- Around line 233-238: The loader assumes the JSON root is a list but may be a
dict; update load_dataset_metadata to validate that the parsed JSON (rows) is a
list before iterating. Specifically, after json.load(f) check isinstance(rows,
list) and if it's a dict either convert it to an iterable of records (e.g., rows
= list(rows.values())) or safely return {} / raise a clear ValueError; ensure
the function still filters by row.get("question_id") and keep the return shape
of dict[str, dict[str, Any]]. Use the function name load_dataset_metadata and
variable rows when making the change so the guard is applied exactly where
parsing occurs.
In `@tests/unit/test_factory.py`:
- Around line 53-64: The tests assume default localhost base URLs but
VLLMLLM.__init__ and LMStudioLLM.__init__ read VLLM_BASE_URL and
LMSTUDIO_BASE_URL from the environment, causing flakes; update the tests
test_create_vllm_llm and test_create_lmstudio_llm to remove those env vars
before creating the LLMs (use pytest's monkeypatch fixture and call
monkeypatch.delenv("VLLM_BASE_URL", raising=False) and
monkeypatch.delenv("LMSTUDIO_BASE_URL", raising=False) respectively) so
create_llm returns instances with the hardcoded defaults and the base_url
assertions become deterministic.
In `@vektori/client.py`:
- Around line 198-203: Race conditions can cause duplicate concurrent
synthesizes; protect the check-and-run in _auto_synthesize_if_due by introducing
a per-(user_id,agent_id) asyncio.Lock stored on the client (e.g.,
self._synthesis_locks mapping) and acquire that lock before reading
count_sessions and possibly calling synthesize so only one coroutine can observe
and act on the boundary; release the lock after synthesize finishes and
optionally clean up the lock entry. Update callers such as
_bg_synthesize_after_extraction to rely on the existing _auto_synthesize_if_due
behavior without adding additional locking.
In `@vektori/connectors/auth.py`:
- Around line 28-32: The current catch of json.JSONDecodeError silently returns
None for a corrupt token file (the with open(path, "r", encoding="utf-8") as f:
return json.load(f) block), making corruption indistinguishable from a missing
token; change the except to log a warning that includes the file path and the
exception details (e.g., logging.warning or module logger) so callers can
distinguish and debug corrupt credentials — alternatively re-raise the
JSONDecodeError after logging if upstream handling is preferred.
In `@vektori/connectors/github.py`:
- Around line 42-48: Replace the non-canonical nested import of GithubException
with the top-level import; in the import block where you currently have "from
github import Github" and "from github.GithubException import GithubException",
change the latter to "from github import GithubException" so the code uses the
documented, stable export for the exception (update any references to
GithubException accordingly).
- Line 88: The code currently uses issue.updated_at.replace(tzinfo=timezone.utc)
which mislabels rather than converts timezones; change this to convert properly
by using issue.updated_at.astimezone(timezone.utc), and to be defensive handle
naive datetimes by checking issue.updated_at.tzinfo is None and only then use
replace(tzinfo=timezone.utc) or prefer parsing into an aware UTC datetime before
converting; update the assignment to document_time to perform this correct
conversion so timestamps are accurate.
In `@vektori/ingestion/extractor.py`:
- Around line 875-890: The contradiction branch currently always returns
(supersedes_id, False) which forces cross-session behavior; change it to
preserve the actual same_session flag from the chosen candidate. After you get
supersedes_id from _parse_json_response, look up the matched candidate in
plausible_conflicts (or candidates) and extract its same_session value (e.g.
c.get("same_session", False)); then return (supersedes_id, same_session) so
_process_facts sees the real same-session semantics. Ensure you default to False
if the candidate is not found.
In `@vektori/storage/postgres.py`:
- Line 627: The section header comment currently labeled "# ── Syntheses ──" is
incorrect for the block containing episode-related methods; rename that header
to "# ── Episodes ──" wherever it appears above the episode functions (e.g., the
block that precedes insert_episode, insert_episode_fact, get_episodes_for_facts,
and search_episodes). Make the same change in the other modules that duplicate
the wrong header so the comment accurately reflects the Episode methods.
- Around line 115-121: The index drop/create for the syntheses table in _migrate
should be guarded by verifying the syntheses table exists before executing DROP
INDEX / CREATE UNIQUE INDEX; modify the _migrate implementation to first check
table existence (e.g., using a query like SELECT EXISTS FROM
information_schema.tables WHERE table_name = 'syntheses' or SELECT
to_regclass('syntheses')) and only run the two await conn.execute(...)
statements for idx_syntheses_user_text and idx_syntheses_user_agent_text when
that check returns true, matching the pattern used around lines 99–112 for other
schema guards.
In `@vektori/storage/schema.sql`:
- Around line 132-167: Remove the stray duplicate delimiter line before the
syntheses section and update the comment banner above the synthesis_facts table
so the heading matches the table name; specifically, delete the extra "--
============================================================" preceding the
"SYNTHESES" block and change the "EPISODE_FACTS: Links syntheses (L1) to the
facts (L0)..." banner to "SYNTHESIS_FACTS: Links syntheses (L1) to the facts
(L0)..." so the comment aligns with the synthesis_facts table and
syntheses/create statements.
In `@vektori/storage/sqlite.py`:
- Around line 137-147: The UNIQUE constraint on the syntheses table (UNIQUE
(user_id, agent_id, text)) does not enforce uniqueness when agent_id is NULL;
change the schema to match Postgres behavior by either making agent_id NOT NULL
DEFAULT '' in the CREATE TABLE for syntheses or replace the composite UNIQUE
with a unique expression index using COALESCE(agent_id, '') (mirroring the
approach used in postgres.py around the COALESCE(agent_id, '') usage) so that
rows with NULL agent_id are treated as empty string and the uniqueness check
will fire.
In `@vektori/utils/async_worker.py`:
- Around line 108-123: When _timers.get(key) is None but self._buffers[key]
exists we must enforce the same timeout used for waiting on the timer; replace
the direct call await self._process(key) with a guarded call using
asyncio.wait_for and asyncio.shield (e.g., await
asyncio.wait_for(asyncio.shield(self._process(key)), timeout=timeout)), and
handle asyncio.TimeoutError and asyncio.CancelledError exactly as the timer
branch does (log the timeout warning and return, treat cancelled tasks the
same). Update the loop containing _timers, _buffers, _process, timeout and the
existing except handlers to apply this timeout wrapper on the fallback path so
the background synthesis loop cannot be blocked indefinitely.
---
Nitpick comments:
In @.gitignore:
- Line 46: Move the misplaced "/data" ignore entry out of the "Testing" section
and place it with other project/OS data ignores (e.g., near
"benchmark_results/") so semantics are correct; then remove redundant rules
"data/*.json" and "data/longmemeval_s_cleaned.json" since the top-level "/data"
already ignores that path. Update the .gitignore so only one canonical "/data"
entry remains in the appropriate block and delete the now-dead specific JSON
ignore lines.
In `@benchmarks/locomo/locomo_judge.py`:
- Around line 262-287: The current run() loop calls await evaluate_entry
sequentially; change it to run evaluate_entry concurrently with a bounded
semaphore to limit parallelism (e.g., 4–8). Implement a small wrapper coroutine
that acquires an asyncio.Semaphore, calls evaluate_entry(entry,
config.judge_model), releases the semaphore, and returns the result; then
schedule all wrappers with asyncio.gather (or create_task + gather) to collect
results, preserving ordering if needed, and use config or a new parameter to set
the concurrency level; update references in run(), entries iteration and results
collection accordingly.
- Around line 185-204: evaluate_entry currently calls create_llm(judge_model)
per entry which recreates the provider/HTTP client for every call; instead
instantiate the judge LLM once and reuse it by moving create_llm(judge_model)
out of evaluate_entry and passing the LLM instance into evaluate_entry (or store
it in a higher-scope variable) and update call sites (e.g., run) to create the
LLM once and pass that instance into evaluate_entry so you avoid repeated
provider construction and enable connection pooling.
- Around line 29-43: The abstention detection uses ABSTENTION_PHRASES (defined
as ASCII apostrophes) but matching is done without normalizing Unicode smart
quotes, so curly quotes in model outputs will not match; update the
normalization pipeline used before comparing against ABSTENTION_PHRASES (the
code that lowercases/collapses whitespace around where ABSTENTION_PHRASES is
checked — see the matching logic referenced near lines 107-111) to first replace
common smart-quote characters (e.g. U+2018/U+2019 and U+201C/U+201D) with their
ASCII equivalents (' and ") or otherwise normalize to NFKC, or add a small
helper normalize_quotes(text) and call it before lower()/whitespace-collapse and
membership checks against ABSTENTION_PHRASES.
In `@benchmarks/locomo/locomo_runner.py`:
- Around line 706-711: _date_prefix currently falls back to str(timestamp) which
turns None into "None"; update the _date_prefix function to explicitly treat
None (and any sentinel that _coerce_datetime might leave) as an empty value:
call out timestamp is None at the top and return "" immediately, otherwise
proceed with parsed = _coerce_datetime(timestamp) and the existing logic;
reference _date_prefix and _coerce_datetime so the change is easy to find and
ensure callers like _timestamp_for_context keep producing an empty prefix for
None.
- Around line 100-112: The constructor LoCoMoBenchmark.__init__ currently calls
_load_qa_prompt_override which does file I/O and can raise exceptions during
construction; defer that call into LoCoMoBenchmark.setup() instead: remove the
_load_qa_prompt_override(config.qa_prompt_path) invocation from __init__ (leave
self._qa_prompt_override = None) and add code in setup() to call
self._qa_prompt_override = _load_qa_prompt_override(self.config.qa_prompt_path)
so all startup I/O errors happen during setup() and go through the same
try/finally cleanup path; update any tests (e.g.,
test_locomo_runner_keeps_prompt_override_on_instance) to call await
runner.setup() or to call _load_qa_prompt_override directly if they expect the
override immediately.
In `@benchmarks/locomo/run_locomo.py`:
- Around line 80-86: The default for the "--eval-model" arg in run_locomo.py was
changed to "vllm:Qwen/Qwen3-8B" which requires a reachable vLLM server; update
the top-of-file "Required environment variables" / "Quick start" sections (in
run_locomo.py) to explicitly state that either a vLLM endpoint must be available
or document how to override "--eval-model" (e.g., set to a Gemini model) before
running the one-liner, or alternatively revert the "--eval-model" default back
to a Gemini-based value so users with only the documented env vars won't hit a
connection error during QA. Ensure you reference the "--eval-model" argument and
the QA prompt path when editing the docs so users know the runtime dependency.
In `@examples/github_connector_demo.py`:
- Around line 59-62: Replace the broad "except Exception as e" handler in the
demo with behavior that preserves the traceback: either remove the except so
exceptions propagate, or log the full traceback then re-raise; to do this,
import traceback, change the except block that currently prints "An error
occurred: {e}" (the block surrounding await v.close()) to call process/print
traceback.format_exc() and then raise, or simply let the exception propagate
while still ensuring await v.close() runs in the finally block.
In `@scripts/gepa_optimize_locomo_qa.py`:
- Around line 162-163: The expression min(score, 0.5 * score) is redundant
because score is non-negative; update the adjustment in the block that checks
used_relative_time/_looks_temporal/contains_relative_time to directly halve the
score (use 0.5 * score) or, if you intended a lower bound, replace the min with
max(LOWER_BOUND, 0.5 * score) and define that LOWER_BOUND; locate this logic
around the variables score, used_relative_time, _looks_temporal and
contains_relative_time and make the replacement accordingly.
In `@tests/unit/test_factory.py`:
- Around line 67-73: Update test_create_openai_compatible_llm to also verify
default base_url and model parsing: clear or unset OPENAI_COMPAT_BASE_URL in the
test, call create_llm("openai-compatible:Qwen/Qwen3-8B") without the base_url
kwarg, assert isinstance(..., OpenAICompatibleLLM), assert llm.model ==
"Qwen/Qwen3-8B", and assert llm.base_url equals the OpenAICompatibleLLM/default
base-class default (the vLLM default referenced in openai_compatible.py) so the
test locks in the chosen default behavior.
In `@tests/unit/test_locomo_judge.py`:
- Around line 6-58: Add unit tests to exercise edge branches: create a test
where parse_verdict receives malformed JSON and assert parse_error is True and
that it falls back to failure_mode "RETRIEVAL_FAILURE"; create a test where
parse_verdict receives prose containing an embedded JSON object (use a string
with surrounding text) and assert it successfully extracts and parses that JSON;
create a test where parse_verdict receives an unknown verdict string and assert
it normalizes to "WRONG" (or the behavior enforced by VALID_VERDICTS mapping);
and add a test for load_entries that requests a non-existent qid and asserts it
raises the expected error message; reference parse_verdict and load_entries to
locate where to add these tests.
In `@vektori/client.py`:
- Around line 108-115: Fix the typo in the inline comment ("syntheizer" →
"synthesizer") near the Synthesizer import/instantiation and resolve the
redundant hasattr guard in synthesize(): either remove the unreachable
hasattr(self, "_synthesizer") check (since _ensure_initialized() always sets
self._synthesizer) or keep it but add a concise explanatory comment (e.g.,
"defensive check for subclasses that override __init__") so intent is clear;
references: Synthesizer, self._synthesizer, _ensure_initialized(), and
synthesize().
In `@vektori/connectors/auth.py`:
- Line 6: The module currently imports typing generics (from typing import Any,
Dict, List, Optional) while __init__ already uses PEP 604 (str | None); remove
the typing imports and update all annotations in this file that use
Optional[..., Dict[...,], List[...] to use built-in generics and PEP 604 unions
(e.g., str | None, dict[str, Any], list[...] etc.), updating every
function/method signature and variable annotation in connectors/auth.py so the
file consistently uses the modern annotation style.
In `@vektori/ingestion/extractor.py`:
- Around line 1067-1079: The CONTRADICTION_PROMPT constant is defined at the
bottom but referenced from _check_dedup, causing an inconsistent layout; move
the CONTRADICTION_PROMPT definition into the existing "Prompts" section
alongside FACTS_PROMPT, EPISODES_PROMPT, and EPISODES_FALLBACK_PROMPT so all
prompt constants are grouped together; ensure the string content stays identical
and update any nearby comments/header to keep the section organized.
In `@vektori/ingestion/synthesis.py`:
- Around line 43-53: The hardcoded window (limit=300) and minimum threshold
(len(base_facts) < 5) in Synthesizer.synthesize should be made configurable: add
parameters to Synthesizer.__init__ (e.g., max_facts=300, min_facts=5) or read
them from VektoriConfig (use the new synthesis_interval or similarly named
fields), then use those instance/config values when calling
self.db.get_active_facts(limit=...) and when checking if len(base_facts) <
min_facts; keep the existing defaults to preserve behavior if no config is
provided and document the new init/config options.
- Around line 1-6: The file imports the private helper _parse_json_response from
vektori.ingestion.extractor, coupling Synthesizer to extractor internals;
extract that helper into a small shared module (e.g., create
vektori/ingestion/_json.py or vektori/utils/llm_json.py) and export it
(preferably without a leading underscore, e.g., parse_json_response), then
update both vektori/ingestion/extractor.py and vektori/ingestion/synthesis.py to
import parse_json_response from the new module and replace any uses of
_parse_json_response with the shared symbol so Synthesizer no longer depends on
extractor internals.
In `@vektori/models/openai_compatible.py`:
- Line 49: The annotation for kwargs in vektori/models/openai_compatible.py is
too loose; change kwargs: dict to kwargs: dict[str, Any] and add "from typing
import Any" to the imports so type-checkers accept the value-type; update the
signature/context where kwargs is defined (the kwargs variable or parameter near
the kwargs: dict declaration) to use dict[str, Any].
- Around line 27-29: OpenAICompatibleLLM currently falls back to vLLM defaults;
change its __init__ (OpenAICompatibleLLM) so base_url and model are taken only
from explicit args or OPENAI_COMPAT_BASE_URL / OPENAI_COMPAT_MODEL env vars (do
not use DEFAULT_VLLM_BASE_URL or DEFAULT_VLLM_MODEL here); after computing them,
if base_url is empty or model is empty raise ValueError with a clear message
instructing the caller to provide base_url/model (leave VLLMLLM to keep
DEFAULT_VLLM_* fallbacks), and keep api_key resolution from api_key or
OPENAI_COMPAT_API_KEY as before.
In `@vektori/qa/generator.py`:
- Around line 94-98: The broad except in the generate call (in generator.py
around the llm.generate usage) swallows all errors; change the handler to log
full tracebacks (use logger.warning(..., exc_info=True)) and only catch/return a
user-facing message for genuine API errors (catch the specific API client
exception class or a named ApiError), while allowing ValueError/TypeError/other
programming errors to propagate (or re-raise) so they fail fast and surface in
benchmark logs; update the except block surrounding (await llm.generate(...))
accordingly and reference the llm.generate call and the surrounding try/except
for the fix.
- Around line 46-62: The build_qa_prompt function currently .format()s
prompt_template with date_line, context and question which can silently drop or
raise KeyError for unknown placeholders; update build_qa_prompt to validate the
provided prompt_template (or QA_PROMPT) before formatting by inspecting its
format fields (e.g., via string.Formatter().parse()) to ensure it contains the
required placeholders "date_line", "context", and "question" and contains no
unknown placeholders (raise a clear ValueError if any required placeholder is
missing or any unexpected fields exist); keep this validation inside
build_qa_prompt so passing a custom --qa-prompt-file fails early with a
descriptive error.
In `@vektori/retrieval/scoring.py`:
- Around line 99-104: The code recomputes metadata instead of reusing the
already-built local var `meta`; replace occurrences of `(fact.get("metadata") or
{}).get("source", "user")` with `meta.get("source", "user")` (and any similar
repeated `(fact.get("metadata") or {})` uses) so the function that computes
`meta` (the local variable `meta` near the top of the scoring logic) is reused
for source lookups and other accesses; update all instances in the same block
(including the section referenced around lines 118–127) to use the `meta`
variable consistently.
- Line 13: Update the docstring Args to add a description for the new parameter
preference_decay_rate (in addition to the existing temporal_decay_rate) and
document the dispatch rule: explain that items whose metadata indicate an event
(via metadata.temporal_expr or metadata.type == "event") use
temporal_decay_rate, while non-event/preference items use preference_decay_rate;
reference the parameter names preference_decay_rate and temporal_decay_rate and
the metadata checks (metadata.temporal_expr / metadata.type == "event") so
readers can see which decay rate will be applied.
In `@vektori/retrieval/search.py`:
- Around line 258-260: The call sites using search_syntheses rely on
backend-specific defaults (base=10 vs concrete=5); update each call of
search_syntheses in this module to pass an explicit limit parameter (e.g.,
limit=top_k or the same value used for search_episodes) so returned syntheses
are consistent and tunable across backends; locate and modify the
search_syntheses invocations near the search flow (the same scope where
search_episodes is called) to include limit=top_k (or a configured variable) and
ensure the same limit value is used for both search_episodes and
search_syntheses calls for predictable behavior.
In `@vektori/storage/base.py`:
- Around line 199-225: The synthesis-related methods (insert_synthesis,
insert_synthesis_fact, get_syntheses_for_facts, search_syntheses) are currently
soft-optional; make them true abstract methods like the Episodes API by adding
the `@abstractmethod` decorator and removing the permissive default
implementations (remove the empty return [] bodies and the runtime-only
NotImplementedError path) so subclasses must implement these methods at class
construction time; update the class that defines these methods (the storage base
class containing
insert_synthesis/insert_synthesis_fact/get_syntheses_for_facts/search_syntheses)
to import and use abc.abstractmethod accordingly.
In `@vektori/storage/schema.sql`:
- Around line 152-153: The unique btree index idx_syntheses_user_agent_text on
syntheses currently includes the unbounded text column and can exceed Postgres
row-size limits; change it to index a fixed-size hash instead (e.g. use
md5(text) or digest(text, 'sha256')) while keeping the same keying on user_id
and COALESCE(agent_id, '') so uniqueness is preserved; update the index
definition for idx_syntheses_user_agent_text to UNIQUE (user_id,
COALESCE(agent_id, ''), md5(text)) (or the sha256 equivalent) and remove/replace
the old index definition so inserts no longer fail on long text.
In `@vektori/storage/sqlite.py`:
- Around line 178-200: Move the "event_time" column migration (the if
"event_time" not in cols: await self._conn.execute("ALTER TABLE facts ADD COLUMN
event_time" ) block) up into the group of facts-column migrations that build and
use the cols set (the checks that add session_id / subject / mentions), placing
it immediately after those checks and before the synthesis_facts FK-repair
block; keep using the same cols set and leave the synthesis_facts rename/
recreate/insert/drop sequence unchanged so the facts and synthesis_facts
migrations are clearly separated.
🪄 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: 4e0a7cdd-6e6c-4082-9d30-4e04c844455e
📒 Files selected for processing (33)
.gitignorebenchmarks/locomo/locomo_judge.pybenchmarks/locomo/locomo_runner.pybenchmarks/locomo/run_locomo.pybenchmarks/longmemeval/longmemeval_runner.pyexamples/github_connector_demo.pyscripts/gepa_optimize_locomo_qa.pytests/unit/test_factory.pytests/unit/test_llm_dedup.pytests/unit/test_locomo_judge.pytests/unit/test_locomo_runner.pytests/unit/test_longmemeval_runner.pytests/unit/test_synthesis.pyvektori/client.pyvektori/config.pyvektori/connectors/__init__.pyvektori/connectors/auth.pyvektori/connectors/base.pyvektori/connectors/github.pyvektori/ingestion/extractor.pyvektori/ingestion/synthesis.pyvektori/models/factory.pyvektori/models/openai_compatible.pyvektori/qa/__init__.pyvektori/qa/generator.pyvektori/retrieval/scoring.pyvektori/retrieval/search.pyvektori/storage/base.pyvektori/storage/memory.pyvektori/storage/postgres.pyvektori/storage/schema.sqlvektori/storage/sqlite.pyvektori/utils/async_worker.py
| if len(shuffled) < 2: | ||
| raise ValueError("Need at least 2 examples for GEPA train/val split") | ||
|
|
||
| train = shuffled[:train_size] | ||
| val = shuffled[train_size : train_size + val_size] | ||
| if not train: | ||
| raise ValueError("Train split is empty") | ||
| if not val: | ||
| val = shuffled[-min(len(shuffled), val_size or 1) :] | ||
| return train, val |
There was a problem hiding this comment.
Train/val overlap when the shuffled pool is smaller than train_size.
If len(shuffled) <= train_size (common when --max-examples is set lower than --train-size, or when the upstream _full_results.json has few usable rows), train = shuffled[:train_size] already consumes the whole list and val becomes empty. The fallback then assigns val = shuffled[-min(len(shuffled), val_size or 1):], which is guaranteed to be a subset of train. Validation examples leaking into training silently inflates val_aggregate_scores[result.best_idx] reported in the summary, which defeats the purpose of a held-out split.
Either enforce train_size + val_size <= len(shuffled) up front, or carve val from a disjoint tail:
🐛 Proposed fix
- if len(shuffled) < 2:
- raise ValueError("Need at least 2 examples for GEPA train/val split")
-
- train = shuffled[:train_size]
- val = shuffled[train_size : train_size + val_size]
- if not train:
- raise ValueError("Train split is empty")
- if not val:
- val = shuffled[-min(len(shuffled), val_size or 1) :]
- return train, val
+ if len(shuffled) < 2:
+ raise ValueError("Need at least 2 examples for GEPA train/val split")
+ if train_size <= 0:
+ raise ValueError("train_size must be positive")
+ if val_size <= 0:
+ raise ValueError("val_size must be positive")
+ if train_size + val_size > len(shuffled):
+ raise ValueError(
+ f"Not enough examples for disjoint split: have {len(shuffled)}, "
+ f"requested train={train_size} + val={val_size}"
+ )
+
+ train = shuffled[:train_size]
+ val = shuffled[train_size : train_size + val_size]
+ return train, val📝 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 len(shuffled) < 2: | |
| raise ValueError("Need at least 2 examples for GEPA train/val split") | |
| train = shuffled[:train_size] | |
| val = shuffled[train_size : train_size + val_size] | |
| if not train: | |
| raise ValueError("Train split is empty") | |
| if not val: | |
| val = shuffled[-min(len(shuffled), val_size or 1) :] | |
| return train, val | |
| if len(shuffled) < 2: | |
| raise ValueError("Need at least 2 examples for GEPA train/val split") | |
| if train_size <= 0: | |
| raise ValueError("train_size must be positive") | |
| if val_size <= 0: | |
| raise ValueError("val_size must be positive") | |
| if train_size + val_size > len(shuffled): | |
| raise ValueError( | |
| f"Not enough examples for disjoint split: have {len(shuffled)}, " | |
| f"requested train={train_size} + val={val_size}" | |
| ) | |
| train = shuffled[:train_size] | |
| val = shuffled[train_size : train_size + val_size] | |
| return train, val |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/gepa_optimize_locomo_qa.py` around lines 285 - 294, The current split
logic can let val overlap train when len(shuffled) <= train_size; modify the
split so train and val are disjoint: after checking len(shuffled) >= 2, compute
effective_train = min(train_size, len(shuffled) - min(val_size, len(shuffled) -
1)) (or simply ensure train_size + val_size <= len(shuffled>) and adjust sizes),
then set train = shuffled[:effective_train] and val =
shuffled[effective_train:effective_train + val_size] (with a fallback that
reduces val_size but still slices from the tail after effective_train) so val is
always taken from a disjoint tail; update the checks around train and val
creation (variables: shuffled, train_size, val_size, train, val) accordingly.
| class LocomoQAAdapter: | ||
| """GEPA adapter for frozen-context LoCoMo QA synthesis optimization.""" | ||
|
|
||
| def __init__(self, model: str, max_tokens: int = 500) -> None: | ||
| self.llm = create_llm(model) | ||
| self.max_tokens = max_tokens | ||
|
|
||
| def evaluate( | ||
| self, | ||
| batch: list[LocomoExample], | ||
| candidate: dict[str, str], | ||
| capture_traces: bool = False, | ||
| ) -> Any: | ||
| try: | ||
| from gepa.core.adapter import EvaluationBatch | ||
| except ImportError as exc: | ||
| raise RuntimeError( | ||
| "GEPA is not installed. Install it with: " | ||
| "python -m pip install git+https://github.com/gepa-ai/gepa.git" | ||
| ) from exc | ||
|
|
||
| prompt_template = candidate.get(PROMPT_COMPONENT, "") | ||
| outputs: list[dict[str, Any]] = [] | ||
| scores: list[float] = [] | ||
| objective_scores: list[dict[str, float]] = [] | ||
| trajectories: list[dict[str, Any]] | None = [] if capture_traces else None | ||
|
|
||
| predictions = asyncio.run(self._generate_batch(prompt_template, batch)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect create_llm to see whether the returned object holds loop-bound async state.
fd -t f 'factory.py' | xargs -I{} rg -n -C3 'def create_llm|AsyncClient|aiohttp|httpx|ClientSession' {}
rg -n -C3 'class .*LLM|async def generate' --type=py -g 'vektori/models/**'Repository: vektori-ai/vektori
Length of output: 10968
🏁 Script executed:
# First, let's find the file and examine the full LocomoQAAdapter implementation
find . -type f -name "gepa_optimize_locomo_qa.py" | head -1 | xargs wc -lRepository: vektori-ai/vektori
Length of output: 102
🏁 Script executed:
# Look at the full LocomoQAAdapter class to understand the complete picture
rg -n "class LocomoQAAdapter" -A 100 --type=py "scripts/gepa_optimize_locomo_qa.py"Repository: vektori-ai/vektori
Length of output: 4448
🏁 Script executed:
# Check the _generate_batch method specifically
rg -n "async def _generate_batch|def _generate_batch" -A 20 --type=py "scripts/"Repository: vektori-ai/vektori
Length of output: 1786
🏁 Script executed:
# Verify how create_llm is implemented and where it's defined
rg -n "def create_llm" -A 15 --type=pyRepository: vektori-ai/vektori
Length of output: 1106
🏁 Script executed:
# Get the rest of _generate_batch to see how self.llm is used
rg -n "async def _generate_batch" -A 50 "scripts/gepa_optimize_locomo_qa.py" | head -60Repository: vektori-ai/vektori
Length of output: 2398
🏁 Script executed:
# Check how generate is called in _generate_batch
rg -n "self.llm.generate|llm.generate" "scripts/gepa_optimize_locomo_qa.py" -C 3Repository: vektori-ai/vektori
Length of output: 439
🏁 Script executed:
# Look at OpenAILLM._get_client and how AsyncOpenAI is created
rg -n "class OpenAILLM" -A 20 "vektori/models/openai.py"Repository: vektori-ai/vektori
Length of output: 903
🏁 Script executed:
# Check AnthropicLLM similarly
rg -n "class AnthropicLLM" -A 20 "vektori/models/anthropic.py"Repository: vektori-ai/vektori
Length of output: 954
Event loop reuse across asyncio.run calls will cause client binding errors.
LocomoQAAdapter.evaluate is invoked multiple times during optimization (at least max_metric_calls / batch_size times), and each call creates and destroys a fresh event loop via asyncio.run (line 324). The LLM instance is created once in __init__ (line 301) and reused across calls. When self.llm.generate is first awaited (line 406), it caches an async HTTP client (e.g., OpenAI's AsyncOpenAI, Anthropic's AsyncAnthropic) bound to the active loop. On the second evaluate call, a different event loop is created, but the cached client is still bound to the first loop, causing RuntimeError: ... is bound to a different event loop or connection leaks.
Consider owning a single event loop on the adapter instance or deferring LLM creation until inside an async context:
♻️ Proposed fix
def __init__(self, model: str, max_tokens: int = 500) -> None:
- self.llm = create_llm(model)
self.max_tokens = max_tokens
+ self._model = model
+ self._llm = None
+ self._loop: asyncio.AbstractEventLoop | None = None
+
+ `@property`
+ def llm(self):
+ if self._llm is None:
+ self._llm = create_llm(self._model)
+ return self._llm
+
+ def _run_async(self, coro):
+ if self._loop is None or self._loop.is_closed():
+ self._loop = asyncio.new_event_loop()
+ asyncio.set_event_loop(self._loop)
+ return self._loop.run_until_complete(coro)
- predictions = asyncio.run(self._generate_batch(prompt_template, batch))
+ predictions = self._run_async(self._generate_batch(prompt_template, batch))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/gepa_optimize_locomo_qa.py` around lines 297 - 324, LocomoQAAdapter
currently creates the LLM in __init__ (self.llm = create_llm(...)) but calls
asyncio.run(self._generate_batch(...)) inside evaluate, which causes the LLM's
async HTTP client to be bound to a different event loop on repeated evaluate
calls; to fix, defer creation of the async LLM client into an async context
(e.g., create the LLM inside _generate_batch or on first await) or else make the
adapter own a single event loop and run all async work on that loop; update
__init__, evaluate, and _generate_batch so that self.llm is either lazily
instantiated inside _generate_batch (so self.llm.generate uses the currently
running loop) or create a dedicated loop and run _generate_batch on it to avoid
cross-loop binding errors when calling self.llm.generate.
| def set_token( | ||
| self, | ||
| user_id: str, | ||
| platform: str, | ||
| access_token: str, | ||
| refresh_token: Optional[str] = None, | ||
| expiry: Optional[str] = None, | ||
| scopes: Optional[List[str]] = None, | ||
| ) -> None: | ||
| """Save the authentication details securely.""" | ||
| path = self._get_path(user_id, platform) | ||
| path.parent.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| data = { | ||
| "access_token": access_token, | ||
| "refresh_token": refresh_token, | ||
| "expiry": expiry, | ||
| "scopes": scopes or [], | ||
| } | ||
| with open(path, "w", encoding="utf-8") as f: | ||
| json.dump(data, f, indent=2) |
There was a problem hiding this comment.
Restrict permissions on token files; tokens are being written world-readable.
set_token writes plaintext OAuth access/refresh tokens under ~/.vektori/tokens/... using the default umask, which on many systems produces mode 0644 (world-readable). For a local credential store this is a real exposure on multi-user machines and in shared containers. At minimum:
- Create the parent directory with mode
0700. - Create the file itself with mode
0600(open viaos.open+O_CREAT|O_WRONLY|O_TRUNCand0o600, oros.chmodafter write). - Write atomically (temp file +
os.replace) so a crash mid-write doesn't leave a truncated credential file thatget_tokenthen silently treats as missing.
🛡️ Proposed hardening
def set_token(
self,
user_id: str,
platform: str,
access_token: str,
refresh_token: Optional[str] = None,
expiry: Optional[str] = None,
scopes: Optional[List[str]] = None,
) -> None:
"""Save the authentication details securely."""
path = self._get_path(user_id, platform)
path.parent.mkdir(parents=True, exist_ok=True)
+ try:
+ os.chmod(path.parent, 0o700)
+ except OSError:
+ pass
data = {
"access_token": access_token,
"refresh_token": refresh_token,
"expiry": expiry,
"scopes": scopes or [],
}
- with open(path, "w", encoding="utf-8") as f:
- json.dump(data, f, indent=2)
+ tmp = path.with_suffix(path.suffix + ".tmp")
+ fd = os.open(tmp, os.O_CREAT | os.O_WRONLY | os.O_TRUNC, 0o600)
+ with os.fdopen(fd, "w", encoding="utf-8") as f:
+ json.dump(data, f, indent=2)
+ os.replace(tmp, path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vektori/connectors/auth.py` around lines 34 - 54, The set_token method
currently writes tokens with default permissions; change it to create the parent
directory (used by _get_path) with mode 0o700 and write the token file
atomically with file mode 0o600: create a temp file in the same directory, write
JSON to it, fsync, set permissions to 0o600 (either via os.open with
O_CREAT|O_WRONLY|O_TRUNC and mode 0o600 or by os.chmod on the temp file), then
os.replace the temp file to the final path so replaces are atomic; ensure any
helper that reads tokens (e.g., get_token) continues to use _get_path and will
see either the old or new complete file, not a truncated one.
| class Connector(Protocol): | ||
| """ | ||
| Protocol that every platform integration must implement. | ||
| Platforms are sources of memory natively synced to Vektori. | ||
| """ | ||
|
|
||
| @property | ||
| def platform(self) -> str: | ||
| """The identifier for this connector, e.g., 'gmail', 'github', 'notion'.""" | ||
| ... | ||
|
|
||
| async def ingest( | ||
| self, | ||
| user_id: str, | ||
| vektori: Vektori, | ||
| since: datetime | None = None, | ||
| ) -> int: | ||
| """ | ||
| Pull data from the platform and store it into Vektori. | ||
| Uses incremental sync if `since` is provided. | ||
|
|
||
| Args: | ||
| user_id: The owner of this data. | ||
| vektori: The active Vektori client instance. | ||
| since: Only fetch records created/modified after this date. | ||
|
|
||
| Returns: | ||
| The number of documents/records safely ingested. | ||
| """ | ||
| ... | ||
|
|
||
| async def watch(self, user_id: str, callback: Callable[[Any], None]) -> None: | ||
| """ | ||
| Optional webhook installation to ingest data in real-time. | ||
| """ | ||
| ... |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Python typing Protocol member optional vs required behavior
💡 Result:
In Python typing.Protocol, all declared members (attributes and methods) are required for structural subtyping compatibility. A concrete class is assignable to a Protocol P only if it implements all protocol members of P with compatible types. There is no built-in support for optional/required distinction within Protocols like in TypedDict (using NotRequired/Required or total=False). This design choice dates back to PEP 544, which explicitly deferred optional protocol members for simplicity, noting they could be revisited if needed. Official typing specs and Python docs (up to 3.14) confirm all protocol members are mandatory. Workarounds for optional-like behavior: - Omit truly optional members from the Protocol; use hasattr checks at runtime and accept Any/unknown type statically. - Use multiple Protocols (e.g., BaseProto and FullProto inheriting from it) and union types. - For runtime checks, use @runtime_checkable Protocols, but presence is still required for all members statically. Proposals to add NotRequired support (e.g., github.com/python/typing#601, discuss.python.org/t/discussion-optional-class-and-protocol-fields-and-methods/79254) exist but are not implemented as of 2026-04-17. Type checkers like mypy and pyright do not support optional Protocol members natively.
Citations:
- 1: https://typing.python.org/en/latest/spec/protocol.html
- 2: https://typing.readthedocs.io/en/latest/spec/protocol.html
- 3: https://typing.python.org/en/latest/reference/protocols.html
- 4: https://www.python.org/dev/peps/pep-0544/
- 5: https://typing.python.org/en/latest/spec/typeddict.html
- 6: Support total=False for protocols python/mypy#5504
- 7: https://peps.python.org/pep-0655
🏁 Script executed:
# First, let's check the structure of the repository
git ls-files | grep -E "(client|connector)" | head -20Repository: vektori-ai/vektori
Length of output: 227
🏁 Script executed:
# Examine vektori/client.py around line 239-256 mentioned in the review
wc -l vektori/client.pyRepository: vektori-ai/vektori
Length of output: 83
🏁 Script executed:
# Look at the Vektori.connect method to see if it calls watch
rg -n "def connect|\.watch\(" vektori/client.py -A 5 -B 2Repository: vektori-ai/vektori
Length of output: 240
🏁 Script executed:
# Check for ConnectorResult usage in the codebase
rg -n "ConnectorResult" --type pyRepository: vektori-ai/vektori
Length of output: 299
🏁 Script executed:
# Check if github connector exists and how it implements watch
fd -e py . | xargs rg -l "github.*[Cc]onnector|class.*GitHub.*Connector"Repository: vektori-ai/vektori
Length of output: 124
🏁 Script executed:
# Get the full connect method
sed -n '239,270p' vektori/client.pyRepository: vektori-ai/vektori
Length of output: 1278
🏁 Script executed:
# Check github connector implementation, especially watch
cat -n vektori/connectors/github.py | head -80Repository: vektori-ai/vektori
Length of output: 3228
🏁 Script executed:
# Check if ConnectorResult is actually used anywhere (not just imported)
rg "ConnectorResult\(" --type pyRepository: vektori-ai/vektori
Length of output: 44
🏁 Script executed:
# Look at the entire connect method return value
rg -n "async def connect" vektori/client.py -A 30Repository: vektori-ai/vektori
Length of output: 1333
🏁 Script executed:
# Get full github connector to see if watch is implemented
wc -l vektori/connectors/github.pyRepository: vektori-ai/vektori
Length of output: 94
🏁 Script executed:
# See full github.py
cat -n vektori/connectors/github.pyRepository: vektori-ai/vektori
Length of output: 5124
🏁 Script executed:
# Check ConnectorResult definition
sed -n '12,30p' vektori/connectors/base.pyRepository: vektori-ai/vektori
Length of output: 543
watch is part of the Protocol but documented as "Optional" — this forces implementers to provide it even when unsupported.
Connector is a structural Protocol, so any concrete type must implement all three members (platform, ingest, and watch) to satisfy the contract. The docstring calling watch "Optional" contradicts this requirement. Evidence: GitHubConnector implements watch but raises NotImplementedError("Real-time webhooks for GitHub are not yet implemented."), proving that even streaming-unaware connectors are forced to provide a stub.
Meanwhile, Vektori.connect() (line 256) only calls connector.ingest() and never calls watch, so the entire streaming surface is unused today.
Two clean options:
-
Drop
watchfromConnectorand expose it via a separate, narrower protocol:class Connector(Protocol): `@property` def platform(self) -> str: ... async def ingest(self, user_id: str, vektori: Vektori, since: datetime | None = None) -> int: ... class ObservableConnector(Connector, Protocol): async def watch(self, user_id: str, callback: Callable[[Any], None]) -> None: ...
-
Keep
watchbut make it truly optional by moving to anabc.ABCbase class with a default no-op, or by documenting "raiseNotImplementedErrorif unsupported" and havingVektoricatch that.
Also: ConnectorResult is defined in base.py but Vektori.connect() returns the raw int from connector.ingest(), leaving ConnectorResult completely unused. Either wire it through or remove it to avoid API inconsistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vektori/connectors/base.py` around lines 24 - 59, The Connector Protocol
currently requires an optional watch method which forces stubs like
GitHubConnector to exist while Vektori.connect only uses ingest and
ConnectorResult is unused; fix by removing watch from Connector and creating a
new ObservableConnector(Connector, Protocol) that defines async def watch(self,
user_id: str, callback: Callable[[Any], None]) -> None so only true observable
connectors implement it, update any concrete classes that currently implement
watch (e.g., GitHubConnector) to either implement ObservableConnector or drop
the method, and then either remove or wire ConnectorResult through
Vektori.connect so return types are consistent (ensure Vektori.connect returns
ConnectorResult if you choose to keep it, or delete ConnectorResult if unused).
| async def ingest( | ||
| self, | ||
| user_id: str, | ||
| vektori: Vektori, | ||
| since: datetime | None = None, | ||
| ) -> int: | ||
| """ | ||
| Pulls GitHub issues and their comments for the configured repository. | ||
| """ | ||
| try: | ||
| from github import Github | ||
| from github.GithubException import GithubException | ||
| except ImportError as e: | ||
| raise ImportError( | ||
| "PyGithub is required for the GitHub connector. " | ||
| "Install it with: pip install PyGithub" | ||
| ) from e | ||
|
|
||
| token_data = self.auth_store.get_token(user_id, self.platform) | ||
| if not token_data or not token_data.get("access_token"): | ||
| logger.error(f"No GitHub access token found for user {user_id}") | ||
| return 0 | ||
|
|
||
| g = Github(token_data["access_token"]) | ||
| count = 0 | ||
|
|
||
| try: | ||
| repo = g.get_repo(self.repo_name) | ||
|
|
||
| kwargs: dict[str, Any] = {"state": "all"} | ||
| if since: | ||
| kwargs["since"] = since | ||
|
|
||
| issues = repo.get_issues(**kwargs) | ||
|
|
||
| for issue in issues: | ||
| # We want to skip raw pull requests if we just want issues | ||
| # (PyGithub returns PRs as issues too, but we can detect them) | ||
|
|
||
| source_id = f"{self.repo_name}/issues/{issue.number}" | ||
|
|
||
| # Combine issue body and comments into a readable chronological "document" | ||
| content_parts = [ | ||
| f"Title: {issue.title}", | ||
| f"State: {issue.state}", | ||
| f"Author: {issue.user.login}", | ||
| f"Body:\n{issue.body or 'No description provided.'}", | ||
| ] | ||
|
|
||
| comments = issue.get_comments() | ||
| for comment in comments: | ||
| content_parts.append( | ||
| f"\n--- Comment by {comment.user.login} at {comment.created_at} ---\n{comment.body}" | ||
| ) | ||
|
|
||
| document_content = "\n".join(content_parts) | ||
| document_time = issue.updated_at.replace(tzinfo=timezone.utc) | ||
|
|
||
| # Insert into Vektori | ||
| await vektori.add_document( | ||
| content=document_content, | ||
| source=self.platform, | ||
| source_id=source_id, | ||
| user_id=user_id, | ||
| document_time=document_time, | ||
| metadata={ | ||
| "repo": self.repo_name, | ||
| "issue_number": issue.number, | ||
| "state": issue.state, | ||
| "author": issue.user.login, | ||
| "labels": [lbl.name for lbl in issue.labels], | ||
| "is_pr": issue.pull_request is not None, | ||
| } | ||
| ) | ||
|
|
||
| count += 1 | ||
| logger.debug(f"Ingested GitHub issue: {source_id}") | ||
|
|
||
| except GithubException as e: | ||
| logger.error(f"GitHub API error during ingestion: {e}") | ||
| return count | ||
|
|
||
| return count |
There was a problem hiding this comment.
PyGithub is synchronous — this blocks the event loop for the entire ingest.
Github(...), g.get_repo(...), repo.get_issues(...), iterating issues, issue.get_comments(), and every attribute access like issue.user.login / issue.labels / issue.pull_request can trigger synchronous HTTP calls (PyGithub is lazy — pagination and attribute hydration are blocking). Inside an async def, all of that runs on the event loop and stalls every other coroutine (including the ongoing _pipeline extraction the new add_document schedules). For a medium repo this is easily tens of seconds of blocked loop.
Wrap the blocking work in asyncio.to_thread (Py ≥ 3.9) or a dedicated run_in_executor. The cleanest split is to do GitHub I/O in a thread and await vektori.add_document(...) back on the loop:
🛠️ Sketch
import asyncio
def _fetch_issue_payloads(token: str, repo_name: str, since: datetime | None):
from github import Github
g = Github(token)
repo = g.get_repo(repo_name)
kwargs: dict[str, Any] = {"state": "all"}
if since:
kwargs["since"] = since
payloads = []
for issue in repo.get_issues(**kwargs):
comments = [
(c.user.login, c.created_at, c.body) for c in issue.get_comments()
]
payloads.append({
"number": issue.number,
"title": issue.title,
"state": issue.state,
"body": issue.body,
"author": issue.user.login,
"labels": [lbl.name for lbl in issue.labels],
"is_pr": issue.pull_request is not None,
"updated_at": issue.updated_at,
"comments": comments,
})
return payloads
async def ingest(self, user_id, vektori, since=None):
...
payloads = await asyncio.to_thread(
_fetch_issue_payloads, token_data["access_token"], self.repo_name, since
)
for p in payloads:
await vektori.add_document(...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vektori/connectors/github.py` around lines 32 - 114, The ingest function
performs synchronous PyGithub calls (Github, g.get_repo, repo.get_issues,
iterating issues, issue.get_comments and attribute access) inside an async
context which blocks the event loop; fix by moving all PyGithub I/O into a
synchronous helper (e.g. _fetch_issue_payloads(token, repo_name, since)) that
collects plain serializable payloads (issue number, title, state, body, author
login, labels, is_pr flag, updated_at, and a list of comment tuples), call that
helper via asyncio.to_thread or run_in_executor from ingest, then iterate the
returned payloads in ingest and await vektori.add_document(...) for each item;
ensure you still catch GithubException around the thread call and preserve
metadata keys ("repo", "issue_number", "state", "author", "labels", "is_pr")
when constructing documents.
| dedup = await self._check_dedup( | ||
| fact_data["text"], fact_emb, session_id, user_id, agent_id, subject | ||
| ) |
There was a problem hiding this comment.
replay_facts no longer guarantees "No LLM call".
_check_dedup was extended to issue an LLM contradiction-check on same-subject near-misses (sim > 0.65). Since replay_facts calls _check_dedup as well (Line 790), the replay path can now trigger an LLM call per replayed fact whose nearest neighbor falls in the 0.65–0.85 band. This contradicts the docstring at Lines 764–766 ("No LLM call") and can materially slow down the benchmark cache-hit path that this method is meant to accelerate.
Consider either (a) skipping the contradiction branch during replay (e.g., add a use_llm_contradiction: bool flag to _check_dedup defaulting to True and pass False from replay_facts), or (b) updating the docstring to acknowledge the new behavior.
🤖 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 - 792, replay_facts now
calls _check_dedup which may invoke the LLM for contradiction-checks, breaking
the "No LLM call" guarantee; add an optional parameter use_llm_contradiction:
bool = True to _check_dedup and gate the contradiction/LLM branch on it, then
call _check_dedup(..., use_llm_contradiction=False) from replay_facts (or
alternatively update the replay_facts docstring to reflect the LLM behavior if
you prefer not to change runtime behavior); ensure the new parameter is
propagated through any internal callers of _check_dedup and that
tests/docstrings are updated to match.
| inserted = 0 | ||
| source_fact_ids = [f["id"] for f in base_facts if f.get("id")] | ||
|
|
||
| for fact_dict, emb in zip(valid_facts, embeddings): | ||
| # Check dedup against existing synthesis | ||
| existing = await self.db.search_syntheses( | ||
| embedding=emb, user_id=user_id, agent_id=agent_id, limit=5 | ||
| ) | ||
|
|
||
| skip = False | ||
| if existing: | ||
| best = existing[0] | ||
| sim = 1.0 - best.get("distance", 1.0) | ||
| if sim > 0.85: | ||
| # We already have this synthesis, let's just skip it | ||
| skip = True | ||
|
|
||
| if skip: | ||
| continue | ||
|
|
||
| try: | ||
| synthesis_id = await self.db.insert_synthesis( | ||
| text=fact_dict["text"], | ||
| embedding=emb, | ||
| user_id=user_id, | ||
| agent_id=agent_id, | ||
| session_id=None, | ||
| ) | ||
| for fact_id in source_fact_ids: | ||
| await self.db.insert_synthesis_fact(synthesis_id, fact_id) | ||
| inserted += 1 | ||
| except Exception as e: | ||
| logger.warning("Failed to insert synthesis: %s", e) |
There was a problem hiding this comment.
Every synthesis is linked to every base fact — N×M blow-up and loses provenance.
source_fact_ids is collected once over all up-to-300 base facts (Line 92), then every newly inserted synthesis links back to the entire set (Lines 119–120). With the 5-synthesis / 300-fact defaults that's 1,500 insert_synthesis_fact calls per run on a happy path, and the fact→synthesis graph loses all meaning: get_syntheses_for_facts([some_fact_id]) will surface every synthesis for that user regardless of relevance, which directly degrades the retrieval-time "syntheses" pathway added elsewhere in this PR.
A minimal fix is to ask the LLM to include the source fact indices (or IDs) in each returned synthesis item and only link those. If you want to keep this first cut simple, at least cap the fan-out per synthesis and make the selection semantic (e.g., top-k base_facts by cosine similarity to the synthesis embedding).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vektori/ingestion/synthesis.py` around lines 91 - 123, The current loop uses
a single source_fact_ids list for all syntheses which creates N×M links; change
to compute per-synthesis source IDs by either (A) reading the source indices/IDs
returned in each synthesis item from the LLM (if fact_dict contains e.g.
"source_ids" or "source_indices", use those) or (B) compute semantic nearest
base facts for each synthesis embedding by computing cosine similarity between
emb and each base_fact["embedding"] (use base_facts entries and their "id" and
"embedding"), then select a capped set (e.g., top_k=5) and optionally apply a
similarity threshold (e.g., >0.75); pass that per-synthesis list to
insert_synthesis_fact instead of the global source_fact_ids; update variables
referenced here: source_fact_ids, valid_facts, embeddings, insert_synthesis,
insert_synthesis_fact, and search_syntheses.
| def score_and_rank( | ||
| facts: list[dict[str, Any]], | ||
| temporal_decay_rate: float = 0.001, | ||
| preference_decay_rate: float = 0.0001, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm no caller passes preference_decay_rate and surface all score_and_rank invocations.
rg -nP -C3 '\bscore_and_rank\s*\('
rg -nP -C1 '\bpreference_decay_rate\b'Repository: vektori-ai/vektori
Length of output: 5549
🏁 Script executed:
#!/bin/bash
# Find Vektori class definition and initialization
rg -nP 'class Vektori' -A 20
# Check how temporal_decay_rate is stored and passed
rg -nP 'self\.temporal_decay_rate' -C 2
# Check Vektori __init__ signature
rg -nP 'def __init__' vektori/retrieval/search.py -A 25Repository: vektori-ai/vektori
Length of output: 8649
🏁 Script executed:
#!/bin/bash
# Get full Vektori.__init__ from client.py
rg -nP 'class Vektori:' vektori/client.py -A 100 | head -120
# Find SearchNode instantiation in client.py
rg -nP 'SearchNode\(' vektori/client.py -B 2 -A 5Repository: vektori-ai/vektori
Length of output: 4566
🏁 Script executed:
#!/bin/bash
# Get rest of _initialize method to find SearchPipeline instantiation
sed -n '82,200p' vektori/client.pyRepository: vektori-ai/vektori
Length of output: 4867
Expose preference_decay_rate through Vektori and SearchPipeline to match temporal_decay_rate pattern.
Currently, temporal_decay_rate is tunable via the Vektori client, but preference_decay_rate is hardcoded to its default (0.0001) with no override path. Both parameters affect the scoring function (used conditionally at vektori/retrieval/scoring.py:103), so users should be able to tune both or neither. Add preference_decay_rate to Vektori.__init__(), pass it through SearchPipeline.__init__(), and forward it to all three score_and_rank() calls in vektori/retrieval/search.py (lines 219, 396, 522).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vektori/retrieval/scoring.py` at line 13, Add a new float parameter
preference_decay_rate (default 0.0001) to Vektori.__init__ and accept/pass it
into SearchPipeline.__init__; store it on the SearchPipeline instance so it’s
available to scoring. Update SearchPipeline.__init__ signature to accept
preference_decay_rate and forward it into the internal calls that invoke
score_and_rank. Finally, update the three score_and_rank invocations in
search.py to pass preference_decay_rate through (alongside temporal_decay_rate)
so the scoring function in scoring.py receives the tunable value.
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: laxmanclo <137504925+laxmanclo@users.noreply.github.com>
Resolved by merging No UI changes in this PR, so no screenshot is applicable. |
Summary by CodeRabbit
Release Notes
New Features
Tests