refactor: Ideal State Purification (Mypy Strict & Ruff Clean) - #69
Conversation
|
Warning Review limit reached
Next review available in: 45 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
WalkthroughThe pull request tightens type annotations across the application, normalizes configuration and database values, updates MCP tool hook dispatch, refactors RAG route matching, and improves handling of missing or non-dictionary data. ChangesCore data and configuration normalization
MCP tool contracts and hook dispatch
RAG routing and search flow
Shared state, lifecycle, and crypto handling
Static analysis and supporting type fixes
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
Greptile SummaryThe PR broadly strengthens typing and normalizes database-facing values across memory, wiki, graph, lifecycle, and MCP tool code.
Confidence Score: 2/5The PR is not safe to merge because episode persistence, wiki tag serialization, and hook-dispatch indirection remain broken on current paths. Episode saves still fail from the SQL binding mismatch, wiki searches still return character-split tags, and modular tool calls still bypass the hook symbol patched by the established integration path. Files Needing Attention: core/episodic.py, mcp_server/tools/wiki.py, mcp_server/tools/memory.py, mcp_server/tools/episodic.py, mcp_server/tools/graph.py, mcp_server/tools/ops.py, mcp_server/tools/session.py, mcp_server/tools_layer.py
|
| Filename | Overview |
|---|---|
| core/episodic.py | Adds stricter result conversion and row typing around episodic persistence. |
| mcp_server/tools/wiki.py | Refactors wiki tool result normalization and return annotations. |
| mcp_server/tools/memory.py | Replaces tools-layer hook references with directly imported hook dispatch and adds stricter response typing. |
| mcp_server/tools_layer.py | Reorganizes the compatibility and dispatch surface for modular MCP tools. |
| mcp_server/context.py | Adds constructor typing while retaining explicit registration of user and agent hook instances. |
Reviews (4): Last reviewed commit: "style: apply ruff format" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| cursor = await conn.execute( | ||
| "INSERT INTO episodes (user_id, summary, emotional_weight, tags, created_at) VALUES (?, ?, ?, ?, ?)", | ||
| (user_id, summary, emotional_weight, json.dumps(tags or []), time.time()), | ||
| (user_id, summary, emotional_weight, json.dumps(tags or [])), |
There was a problem hiding this comment.
When any episode is saved, this four-value tuple is bound to an INSERT with five placeholders, causing SQLite to raise a binding-count error before the episode is persisted.
| (user_id, summary, emotional_weight, json.dumps(tags or [])), | |
| (user_id, summary, emotional_weight, json.dumps(tags or []), time.time()), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: core/episodic.py
Line: 51
Comment:
**Episode INSERT binding mismatch**
When any episode is saved, this four-value tuple is bound to an `INSERT` with five placeholders, causing SQLite to raise a binding-count error before the episode is persisted.
```suggestion
(user_id, summary, emotional_weight, json.dumps(tags or []), time.time()),
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| _invalidate_cache, | ||
| _get_recall_cache, | ||
| _set_recall_cache, | ||
| _fire_hook, |
There was a problem hiding this comment.
When the existing hook tests replace tools_layer._fire_hook, these direct imports retain their own binding to base._fire_hook, so the tracking callback is never invoked and hook assertions fail across the changed memory, episodic, graph, ops, and session tools.
Prompt To Fix With AI
This is a comment left during a code review.
Path: mcp_server/tools/memory.py
Line: 19
Comment:
**Hook dispatch bypasses indirection**
When the existing hook tests replace `tools_layer._fire_hook`, these direct imports retain their own binding to `base._fire_hook`, so the tracking callback is never invoked and hook assertions fail across the changed memory, episodic, graph, ops, and session tools.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (2)
lifecycle/compactor.py (1)
53-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve SQLite row identifiers in compaction errors.
The connection manager configures SQLite rows as
aiosqlite.Row, notdict. Therefore, this branch logs"unknown"for every archive failure and loses the candidate ID. Extractrow["id"]with a safe fallback instead of checking onlydict.Proposed fix
except (KeyError, RuntimeError): - rid = row["id"] if isinstance(row, dict) else "unknown" + try: + rid = row["id"] + except (KeyError, IndexError, TypeError): + rid = "unknown" logger.exception("Failed to archive memory %s", rid)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lifecycle/compactor.py` around lines 53 - 54, Update the row-ID extraction in the archive error handling around the compactor’s logger.exception call to support aiosqlite.Row and other mapping-like rows, while retaining a safe fallback when the ID cannot be read. Ensure candidate IDs are preserved in compaction failure logs instead of defaulting to "unknown" for non-dict rows.mcp_server/tools_layer.py (1)
34-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the asynchronous return type.
base._fire_hookis async and returnsdict[str, Any](mcp_server/tools/base.pyLines 120-128). ReturningAnyhides the awaitable contract.Make
_fire_hook_wrapperasync with adict[str, Any]return type, or annotate the current wrapper as returningAwaitable[dict[str, Any]].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp_server/tools_layer.py` at line 34, Update _fire_hook_wrapper to preserve base._fire_hook’s asynchronous contract by making it async with a dict[str, Any] return annotation, or by annotating its current form as Awaitable[dict[str, Any]]. Ensure the wrapper continues returning the awaited hook result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/__init__.py`:
- Around line 35-43: Update recall so cached values are returned only when
cached is a list; treat any non-list value as a cache miss, continue through the
existing l4.search and episodic search flow, and overwrite the invalid cache
entry with the resulting memories.
In `@core/episodic.py`:
- Around line 51-54: Add the missing created_at timestamp bind value to the
parameter tuple in save, matching the five placeholders in its SQL statement;
preserve the existing user_id, summary, emotional_weight, and serialized tags
bindings and commit/return behavior.
In `@mcp_server/middlewares.py`:
- Line 26: Update the allowed_origins configuration used by CORSMiddleware to
validate that config.get("cors", "allowed_origins") returns a list containing
only strings, falling back safely when absent or invalid. Replace wildcard-port
entries such as http://localhost:* with exact origins or configure an equivalent
allow_origin_regex so localhost and 127.0.0.1 ports match correctly.
In `@mcp_server/tools/ops.py`:
- Around line 374-381: Update _delete_graph to delete related epi_tags and
epi_edges rows within the same transaction before deleting matching epi_nodes.
Use the selected node IDs or equivalent predicates to target only dependents of
the specified user_id and cutoff, preserving the existing purge scope and
transaction handling.
- Around line 355-381: Update _delete_staging, _delete_audit, and _delete_graph
to use the active context connection manager instead of constructing services
with their default global manager. Pass that manager into DreamBuffer,
AuditTrail, and EpistemicGraph, or reuse the corresponding app-owned instances,
while preserving the existing purge queries and results.
- Around line 335-353: Remove the finally-block connection closures from
_delete_core and _delete_episodes, since
app.mm.user_memory(...)._cm.get(DB_NAME) may return the same cached connection
to concurrent purge tasks. Keep commit and result handling intact, and rely on
the connection manager for the shared connection lifecycle (or otherwise use
explicitly dedicated, coordinated connections).
- Around line 187-188: Update the async memory_api_key and memory_backup
handlers to avoid executing synchronous storage operations directly on the event
loop. Use the available async APIs, or route EncryptedStore save/revoke and
backup, restore, manifest, and directory operations through a serialized
asyncio.to_thread path while preserving existing behavior and responses.
In `@mcp_server/tools/wiki.py`:
- Around line 27-41: Update the result construction in wiki_search to
deserialize each result’s tags value before returning it, using the shared tag
parser or equivalent safe JSON-list normalization. Ensure JSON-encoded tag
strings become lists of tag names, while missing or invalid values produce an
empty list, and preserve the existing title, type, and count behavior.
In `@rag/ingestor.py`:
- Around line 82-84: Update the page ID retrieval in the ingestion flow to raise
an error when SELECT last_insert_rowid() returns no row or NULL, instead of
assigning 0. Ensure the failure occurs before chunk insertion so the transaction
rolls back, while preserving normal insertion when a valid page ID is returned.
In `@rag/search.py`:
- Line 126: Update the search_rrf call in searcher.py to pass the existing
default_bin_for callback as binary_for_fn, matching the callback used by the
other retrieval path. Ensure the RRF flow performs binary retrieval and
preserves hybrid result ranking; alternatively, provide an equivalent default
encoder within search_rrf.
- Line 229: Update the result mapping in RAGEngine.search so the wiki_type value
from metadata.get("wiki_type") remains None when absent instead of being
converted to the string "None"; otherwise apply the API’s established
empty-value contract while preserving non-null values.
- Line 138: Update the bin_ranks construction in search_binary to key results by
page_id rather than chunk id, deduplicating multiple chunks from the same page
by retaining the best (lowest) binary rank. Keep the resulting keys consistent
with the page lookups at Lines 159-160 so binary-only pages remain included and
ranks cannot collide across chunk IDs.
In `@wiki/shared.py`:
- Around line 26-33: Update get_enabled_types to validate wiki_cfg and layer_cfg
with isinstance(..., dict) before calling .get(); treat any non-dict wiki or
layer configuration as empty and return all_types. Apply the same nested-mapping
validation to the corresponding configuration access in the additional affected
code path.
---
Nitpick comments:
In `@lifecycle/compactor.py`:
- Around line 53-54: Update the row-ID extraction in the archive error handling
around the compactor’s logger.exception call to support aiosqlite.Row and other
mapping-like rows, while retaining a safe fallback when the ID cannot be read.
Ensure candidate IDs are preserved in compaction failure logs instead of
defaulting to "unknown" for non-dict rows.
In `@mcp_server/tools_layer.py`:
- Line 34: Update _fire_hook_wrapper to preserve base._fire_hook’s asynchronous
contract by making it async with a dict[str, Any] return annotation, or by
annotating its current form as Awaitable[dict[str, Any]]. Ensure the wrapper
continues returning the awaited hook result.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0cd427af-be61-4e6e-bed4-203bb3965106
📒 Files selected for processing (55)
.pytest_cache/v/cache/lastfailedconfig.pycore/__init__.pycore/episodic.pycore/memory.pycore/session.pyfeatures/auth/api_key.pyfeatures/backup_cron.pyfeatures/dashboard.pyfeatures/import_export.pyfeatures/secrets.pygraph/epistemic.pygraph/temporal.pyhooks/loader.pyhooks/models.pyhooks/registry.pylifecycle/compactor.pylifecycle/consolidation.pylifecycle/emotion/engine.pylifecycle/forgetting.pymcp_server/app.pymcp_server/context.pymcp_server/middlewares.pymcp_server/registry.pymcp_server/server.pymcp_server/tools/episodic.pymcp_server/tools/graph.pymcp_server/tools/memory.pymcp_server/tools/ops.pymcp_server/tools/session.pymcp_server/tools/wiki.pymcp_server/tools_layer.pymcp_server/utils/circuit_breaker.pypyproject.tomlrag/conflict.pyrag/engine.pyrag/ingestor.pyrag/quantize.pyrag/router.pyrag/schema.pyrag/search.pyrag/searcher.pyshared/archived_memories.pyshared/dream_buffer.pyshared/embeddings.pyshared/importance/scorer.pyshared/importance/signals/emotion_signal.pyshared/memory_types.pyshared/middleware.pyshared/saga/__init__.pyshared/saga/impl/base.pyshared/saga/impl/crypto.pywiki/index.pywiki/manager.pywiki/shared.py
💤 Files with no reviewable changes (1)
- .pytest_cache/v/cache/lastfailed
| async def recall(self, query: str, limit: int = 10) -> list[dict[str, Any]]: | ||
| cache_key = f"recall:{self.user_id}:{query}:{limit}" | ||
| cached = self._cache.get(cache_key) if self._cache else None | ||
| cached: Any = self._cache.get(cache_key) if self._cache else None | ||
| if cached is not None: | ||
| return cached | ||
| return list(cached) if isinstance(cached, list) else [] | ||
|
|
||
| results = [] | ||
| results.extend(await self.l4.search(self.user_id, query, limit)) | ||
| results: list[dict[str, Any]] = [] | ||
| l4_hits = await self.l4.search(self.user_id, query, limit) | ||
| results.extend(l4_hits) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Treat invalid cache values as cache misses.
At Line 39, a non-list cache value returns [] and skips both L4 and episodic searches. A malformed or legacy cache entry can therefore hide valid memories. Fall through to the storage search and overwrite the invalid cache value.
Proposed fix
if cached is not None:
- return list(cached) if isinstance(cached, list) else []
+ if isinstance(cached, list):
+ return list(cached)📝 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.
| async def recall(self, query: str, limit: int = 10) -> list[dict[str, Any]]: | |
| cache_key = f"recall:{self.user_id}:{query}:{limit}" | |
| cached = self._cache.get(cache_key) if self._cache else None | |
| cached: Any = self._cache.get(cache_key) if self._cache else None | |
| if cached is not None: | |
| return cached | |
| return list(cached) if isinstance(cached, list) else [] | |
| results = [] | |
| results.extend(await self.l4.search(self.user_id, query, limit)) | |
| results: list[dict[str, Any]] = [] | |
| l4_hits = await self.l4.search(self.user_id, query, limit) | |
| results.extend(l4_hits) | |
| async def recall(self, query: str, limit: int = 10) -> list[dict[str, Any]]: | |
| cache_key = f"recall:{self.user_id}:{query}:{limit}" | |
| cached: Any = self._cache.get(cache_key) if self._cache else None | |
| if cached is not None: | |
| if isinstance(cached, list): | |
| return list(cached) | |
| results: list[dict[str, Any]] = [] | |
| l4_hits = await self.l4.search(self.user_id, query, limit) | |
| results.extend(l4_hits) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/__init__.py` around lines 35 - 43, Update recall so cached values are
returned only when cached is a list; treat any non-list value as a cache miss,
continue through the existing l4.search and episodic search flow, and overwrite
the invalid cache entry with the resulting memories.
| (user_id, summary, emotional_weight, json.dumps(tags or [])), | ||
| ) | ||
| await conn.commit() | ||
| return cursor.lastrowid | ||
| return int(cursor.lastrowid or 0) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Restore the created_at bind value.
The SQL statement has five placeholders, but the tuple has four values. Every call to save fails with a binding-count error. Add the timestamp as the fifth value.
Proposed fix
- (user_id, summary, emotional_weight, json.dumps(tags or [])),
+ (user_id, summary, emotional_weight, json.dumps(tags or []), time.time()),📝 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.
| (user_id, summary, emotional_weight, json.dumps(tags or [])), | |
| ) | |
| await conn.commit() | |
| return cursor.lastrowid | |
| return int(cursor.lastrowid or 0) | |
| (user_id, summary, emotional_weight, json.dumps(tags or []), time.time()), | |
| ) | |
| await conn.commit() | |
| return int(cursor.lastrowid or 0) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/episodic.py` around lines 51 - 54, Add the missing created_at timestamp
bind value to the parameter tuple in save, matching the five placeholders in its
SQL statement; preserve the existing user_id, summary, emotional_weight, and
serialized tags bindings and commit/return behavior.
| def add_middlewares(app: Starlette) -> None: | ||
| app.add_middleware(AuthMiddleware) | ||
| allowed_origins = config.get("cors", "allowed_origins", default=["http://localhost:*", "http://127.0.0.1:*"]) | ||
| allowed_origins: list[str] = config.get("cors", "allowed_origins", default=["http://localhost:*", "http://127.0.0.1:*"]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files 'mcp_server/**/*.py' '*requirements*' '*pyproject.toml' '*uv.lock' '*poetry.lock' | sed -n '1,120p'
printf '%s\n' '--- middleware source ---'
cat -n mcp_server/middlewares.py | sed -n '1,100p'
printf '%s\n' '--- Starlette pins and CORS usage ---'
rg -n -i 'starlette|allow_origins|allow_origin_regex|allowed_origins|cors' \
--glob '!node_modules' --glob '!dist' --glob '!build' . | sed -n '1,240p'
printf '%s\n' '--- config.get definitions and call sites ---'
rg -n 'def get\(|async def get\(|config\.get\(' mcp_server | sed -n '1,240p'Repository: Cipher208/mcp-ariel-memory
Length of output: 5650
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- configuration sources ---'
git ls-files | rg '(^|/)(config(\.py|/)|.*config.*\.py$|config\.yaml)$' | sed -n '1,160p'
rg -n -C 8 'class .*Config|def get\(|allowed_origins|cors:' --glob '*.py' --glob '*.yaml' --glob '*.yml' . | sed -n '1,260p'
printf '%s\n' '--- dependency declarations ---'
sed -n '25,45p' pyproject.toml
sed -n '2518,2535p' uv.lock
printf '%s\n' '--- Starlette 1.3.1 CORS implementation ---'
curl -fsSL https://raw.githubusercontent.com/encode/starlette/1.3.1/starlette/middleware/cors.py | nl -ba | sed -n '1,240p'Repository: Cipher208/mcp-ariel-memory
Length of output: 12843
🏁 Script executed:
#!/bin/bash
set -eu
tmp=$(mktemp)
curl -fsSL https://raw.githubusercontent.com/encode/starlette/1.3.1/starlette/middleware/cors.py > "$tmp"
printf '%s\n' '--- Starlette 1.3.1 CORS implementation ---'
sed -n '1,260p' "$tmp"
printf '%s\n' '--- config implementation and current YAML ---'
cat -n config.py | sed -n '1,90p'
cat -n config.yaml | sed -n '1,140p'
printf '%s\n' '--- all CORS configuration references ---'
rg -n -C 5 'allowed_origins|allow_origins|allow_origin_regex|cors:' . \
--glob '!uv.lock' --glob '!node_modules' --glob '!dist' --glob '!build' | sed -n '1,220p'Repository: Cipher208/mcp-ariel-memory
Length of output: 16516
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import re
# Standalone probe of the relevant Starlette 1.3.1 CORSMiddleware logic.
def is_allowed_origin(allow_origins, allow_origin_regex, origin):
allow_all_origins = "*" in allow_origins
compiled = re.compile(allow_origin_regex) if allow_origin_regex is not None else None
if allow_all_origins:
return True
if compiled is not None and compiled.fullmatch(origin):
return True
return origin in allow_origins
defaults = ["http://localhost:*", "http://127.0.0.1:*"]
for origin in [
"http://localhost:3000",
"http://127.0.0.1:5173",
"http://localhost:*",
"http://example.com",
]:
print(f"default origins; {origin!r}: {is_allowed_origin(defaults, None, origin)}")
regex = r"^https?://(?:localhost|127\.0\.0\.1):\d+$"
for origin in ["http://localhost:3000", "http://127.0.0.1:5173", "http://example.com"]:
print(f"regex origins; {origin!r}: {is_allowed_origin([], regex, origin)}")
for configured in [None, "http://localhost:3000", ["http://localhost:3000", 3000]]:
try:
result = "*" in configured
print(f"configured={configured!r}: init membership check -> {result!r}; annotation conversion -> unchanged")
except Exception as exc:
print(f"configured={configured!r}: init membership check -> {type(exc).__name__}: {exc}")
PYRepository: Cipher208/mcp-ariel-memory
Length of output: 793
Use a valid CORS origin matcher and validate the configuration.
CORSMiddleware performs exact matching for allow_origins; http://localhost:* does not allow http://localhost:3000. Use exact origins or allow_origin_regex. Validate config.get as list[str]; its annotation does not convert None, strings, or non-string list items. A None value causes Starlette initialization to raise TypeError.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp_server/middlewares.py` at line 26, Update the allowed_origins
configuration used by CORSMiddleware to validate that config.get("cors",
"allowed_origins") returns a list containing only strings, falling back safely
when absent or invalid. Replace wildcard-port entries such as http://localhost:*
with exact origins or configure an equivalent allow_origin_regex so localhost
and 127.0.0.1 ports match correctly.
Source: Path instructions
| ctx: Context[Any, Any] | None = None, | ||
| ) -> dict[str, Any]: |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Move synchronous storage work off the event loop.
memory_api_key calls synchronous EncryptedStore.save through APIKeyAuth.create_key and APIKeyAuth.revoke (features/auth/store.py Lines 19-70). memory_backup performs synchronous backup, restore, manifest, and directory operations (features/backup_cron.py Lines 112-141 and 174-212).
These calls run inside async MCP handlers and can stall other requests. Use async APIs or a serialized asyncio.to_thread path.
As per path instructions, mcp_server/**/*.py must avoid blocking calls in async functions.
Also applies to: 207-208
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp_server/tools/ops.py` around lines 187 - 188, Update the async
memory_api_key and memory_backup handlers to avoid executing synchronous storage
operations directly on the event loop. Use the available async APIs, or route
EncryptedStore save/revoke and backup, restore, manifest, and directory
operations through a serialized asyncio.to_thread path while preserving existing
behavior and responses.
Source: Path instructions
| async def _delete_core() -> int: | ||
| conn = await app.mm.user_memory(user_id).l4._cm.get(DB_NAME) | ||
| try: | ||
| cursor = await conn.execute("DELETE FROM core_memory WHERE user_id=? AND created_at > ?", (user_id, cutoff)) | ||
| result = cursor.rowcount | ||
| result = int(cursor.rowcount) | ||
| await conn.commit() | ||
| return result | ||
| finally: | ||
| await conn.close() | ||
|
|
||
| async def _delete_episodes(): | ||
| async def _delete_episodes() -> int: | ||
| conn = await app.mm.user_memory(user_id).l3._cm.get(DB_NAME) | ||
| try: | ||
| cursor = await conn.execute("DELETE FROM episodes WHERE user_id=? AND created_at > ?", (user_id, cutoff)) | ||
| result = cursor.rowcount | ||
| result = int(cursor.rowcount) | ||
| await conn.commit() | ||
| return result | ||
| finally: | ||
| await conn.close() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not close cached connections from concurrent purge tasks.
AsyncConnectionManager.get caches one connection per database (shared/connection.py Lines 142-162). _delete_core and _delete_episodes run concurrently through asyncio.gather and can share the same connection. Their finally blocks can close it while another helper is executing or committing.
Let the connection manager own the shared connection lifecycle, or allocate dedicated connections and coordinate the purge.
As per path instructions, mcp_server/**/*.py requires correct async connection lifecycle handling.
Also applies to: 361-385
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp_server/tools/ops.py` around lines 335 - 353, Remove the finally-block
connection closures from _delete_core and _delete_episodes, since
app.mm.user_memory(...)._cm.get(DB_NAME) may return the same cached connection
to concurrent purge tasks. Keep commit and result handling intact, and rely on
the connection manager for the shared connection lifecycle (or otherwise use
explicitly dedicated, coordinated connections).
Source: Path instructions
| cursor = await conn.execute("SELECT last_insert_rowid()") | ||
| row = await cursor.fetchone() | ||
| page_id = row[0] | ||
| page_id = int(row[0]) if row and row[0] is not None else 0 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail when SQLite does not return an inserted page ID.
If SELECT last_insert_rowid() returns no row or NULL, Line 84 sets page_id to 0. The following chunk inserts then use page_id=0, and RAGEngine.ingest_text treats 0 as no page ID. This silently creates orphaned chunks. Raise before chunk insertion so the transaction rolls back.
Proposed fix
cursor = await conn.execute("SELECT last_insert_rowid()")
row = await cursor.fetchone()
- page_id = int(row[0]) if row and row[0] is not None else 0
+ if row is None or row[0] is None:
+ raise RuntimeError("SQLite did not return the inserted page ID")
+ page_id = int(row[0])📝 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.
| cursor = await conn.execute("SELECT last_insert_rowid()") | |
| row = await cursor.fetchone() | |
| page_id = row[0] | |
| page_id = int(row[0]) if row and row[0] is not None else 0 | |
| cursor = await conn.execute("SELECT last_insert_rowid()") | |
| row = await cursor.fetchone() | |
| if row is None or row[0] is None: | |
| raise RuntimeError("SQLite did not return the inserted page ID") | |
| page_id = int(row[0]) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rag/ingestor.py` around lines 82 - 84, Update the page ID retrieval in the
ingestion flow to raise an error when SELECT last_insert_rowid() returns no row
or NULL, instead of assigning 0. Ensure the failure occurs before chunk
insertion so the transaction rolls back, while preserving normal insertion when
a valid page ID is returned.
| limit: int, | ||
| k: int = 60, | ||
| binary_for_fn=None, | ||
| binary_for_fn: Callable[[list[float]], bytes] | None = None, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass the binary encoder on the RRF path.
search_rrf skips binary retrieval when binary_for_fn is None. The provided call in rag/searcher.py Line 81 omits this callback, so hybrid queries always produce FTS-only ranks. Pass the same default_bin_for used by rag/searcher.py Line 66, or keep a default encoder inside search_rrf.
Proposed caller fix
- search_rrf(..., fts_available=fts_ready, binary_dim=self.binary_dim)
+ search_rrf(..., binary_for_fn=default_bin_for, fts_available=fts_ready, binary_dim=self.binary_dim)As per path instructions, RAG search must preserve search result ranking consistency.
Also applies to: 135-138
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rag/search.py` at line 126, Update the search_rrf call in searcher.py to pass
the existing default_bin_for callback as binary_for_fn, matching the callback
used by the other retrieval path. Ensure the RRF flow performs binary retrieval
and preserves hybrid result ranking; alternatively, provide an equivalent
default encoder within search_rrf.
Source: Path instructions
| if binary_for_fn: | ||
| with suppress(Exception): | ||
| bin_results = await search_binary(cm, query, user_id, limit * 3, binary_for_fn, binary_dim) | ||
| bin_ranks = {r["id"]: rank for rank, r in enumerate(bin_results)} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Key binary ranks by page ID.
search_binary returns the chunk ID in id and the page ID in page_id. FTS ranks use page IDs. Line 138 keys bin_ranks with chunk IDs, but Lines 159-160 fetch pages by those keys. Binary-only results can disappear, and ID collisions can attach ranks to the wrong pages. Deduplicate by page_id and retain the best binary rank.
Proposed fix
- bin_ranks = {r["id"]: rank for rank, r in enumerate(bin_results)}
+ bin_ranks = {}
+ for rank, r in enumerate(bin_results):
+ bin_ranks.setdefault(r["page_id"], rank)As per path instructions, RAG search must preserve search result ranking consistency.
📝 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.
| bin_ranks = {r["id"]: rank for rank, r in enumerate(bin_results)} | |
| bin_ranks = {} | |
| for rank, r in enumerate(bin_results): | |
| bin_ranks.setdefault(r["page_id"], rank) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rag/search.py` at line 138, Update the bin_ranks construction in
search_binary to key results by page_id rather than chunk id, deduplicating
multiple chunks from the same page by retaining the best (lowest) binary rank.
Keep the resulting keys consistent with the page lookups at Lines 159-160 so
binary-only pages remain included and ranks cannot collide across chunk IDs.
Source: Path instructions
| "wiki_type": c.wiki_type, | ||
| "score": c.final_score or c.rrf_score, | ||
| "source": c.source, | ||
| "wiki_type": str(c.wiki_type), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve nullable wiki_type values.
The RAG schema allows wiki_type to be NULL, and RAGEngine.search passes the value from metadata.get("wiki_type"). str(c.wiki_type) converts a missing value into the literal string "None". Preserve None or apply the API's defined empty-value contract.
Proposed fix
- "wiki_type": str(c.wiki_type),
+ "wiki_type": None if c.wiki_type is None else str(c.wiki_type),📝 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.
| "wiki_type": str(c.wiki_type), | |
| "wiki_type": None if c.wiki_type is None else str(c.wiki_type), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rag/search.py` at line 229, Update the result mapping in RAGEngine.search so
the wiki_type value from metadata.get("wiki_type") remains None when absent
instead of being converted to the string "None"; otherwise apply the API’s
established empty-value contract while preserving non-null values.
| def get_enabled_types(layer: str, all_types: list[str]) -> list[str]: | ||
| """Return wiki types enabled in config for the given layer.""" | ||
| cfg = load_config() | ||
| layer_cfg = cfg.get("wiki", {}).get(layer, {}) | ||
| wiki_cfg: dict[str, Any] = cfg.get("wiki", {}) | ||
| layer_cfg: dict[str, Any] = wiki_cfg.get(layer, {}) | ||
| if not layer_cfg: | ||
| return all_types | ||
| return [t for t in all_types if layer_cfg.get(t, True)] | ||
| return [t for t in all_types if bool(layer_cfg.get(t, True))] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate nested configuration mappings before access.
load_config() validates only the top-level result. If cfg["wiki"] or cfg["wiki"][layer] is None, a list, or a scalar, Line 29/Line 30 and Line 39/Line 40 call .get() on a non-mapping and raise AttributeError. This breaks wiki operations instead of using the documented fallback. Check each nested value with isinstance(..., dict) before accessing it.
Proposed fix
def get_enabled_types(layer: str, all_types: list[str]) -> list[str]:
cfg = load_config()
- wiki_cfg: dict[str, Any] = cfg.get("wiki", {})
- layer_cfg: dict[str, Any] = wiki_cfg.get(layer, {})
+ wiki_cfg = cfg.get("wiki")
+ if not isinstance(wiki_cfg, dict):
+ return all_types
+ layer_cfg = wiki_cfg.get(layer)
+ if not isinstance(layer_cfg, dict):
+ return all_types
if not layer_cfg:
return all_typesAlso applies to: 36-42
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wiki/shared.py` around lines 26 - 33, Update get_enabled_types to validate
wiki_cfg and layer_cfg with isinstance(..., dict) before calling .get(); treat
any non-dict wiki or layer configuration as empty and return all_types. Apply
the same nested-mapping validation to the corresponding configuration access in
the additional affected code path.
1 participant
Achieved absolute engineering excellence: 100% Mypy strict coverage, zero Ruff issues, and modularized high-complexity components.
Summary by CodeRabbit
Bug Fixes
Refactor