Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Improve memory
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds stricter prompt constraints and personalization non‑translatability rules in the analysis agent; refactors MemoryTool to be history-aware with new/changed methods and direct Cypher node creation; routes and frontend gate demo graphs, pass history into memory persistence, and centralize chat input state. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant FE as Frontend
participant API as /graphs.generate
participant AA as AnalysisAgent
participant DB as SQL DB
participant MT as MemoryTool
U->>FE: enter question
FE->>API: stream question + selected graph
API->>AA: analyze(question, memory_context)
AA-->>API: {is_sql_translatable, sql_query, missing_information}
alt is_sql_translatable == true
API->>DB: execute sql_query
DB-->>API: query_results
alt results non-empty
API-->>FE: emit event: query_result
end
par Save memory concurrently
API->>MT: add_new_memory(full_response, [queries_history, result_history])
MT->>MT: summarize_conversation(..., history)
MT->>MT: update_user_information(..., history)
MT-->>API: memory saved
and Stream final response
API-->>FE: final response stream
end
else is_sql_translatable == false
API-->>FE: explanation (missing_information) — no fabricated placeholders
note right of FE: frontend no longer emits fallback follow-up on invalid final results
end
sequenceDiagram
autonumber
participant MT as MemoryTool
participant G as GraphDB
participant LLM as LLM
MT->>G: ensure entity nodes via Cypher (user node named by user_id)
par Concurrent retrieval
MT->>G: search_user_summary (direct Cypher)
MT->>G: search_database_facts(query, episode_limit)
MT->>G: search_similar_queries(query)
end
G-->>MT: user summary, facts, episodes, similar queries
MT->>MT: assemble composite memory_context
MT->>LLM: summarize_conversation(conversation, history)
LLM-->>MT: {database_summary,...}
MT->>LLM: update_user_information(conversation, history)
LLM-->>MT: user summary update
MT->>G: persist summaries, episodes, facts
MT-->>Caller: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
✨ Finishing Touches
🧪 Generate unit tests
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 |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Pull Request Overview
This PR implements memory-aware conversation handling in QueryWeaver, adding personalized context and user information management to improve query analysis and response generation.
Key changes:
- Enhances memory management with user information updates and conversation history integration
- Improves chat handling by removing error fallback messages for invalid SQL queries
- Strengthens query analysis with better personal context validation and placeholder prevention
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
app/ts/modules/chat.ts |
Removes error message fallback when SQL query generation fails |
api/routes/graphs.py |
Adds conversation history to memory save operations and conditional query result streaming |
api/memory/graphiti_tool.py |
Major refactoring: adds user information updates, conversation history integration, and improved memory search |
api/agents/analysis_agent.py |
Enhances personal query validation and prevents placeholder generation in SQL queries |
Comments suppressed due to low confidence (1)
app/ts/modules/chat.ts:1
- Removing the error message fallback when
step.is_validis false will leave users without feedback when SQL generation fails. This creates a poor user experience as users won't understand why their query didn't produce results. Consider replacing this with a more appropriate error handling mechanism or keeping some form of user feedback.
/**
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
api/agents/analysis_agent.py (1)
244-244: Consider reformatting for consistency.While the logic is correct, the line formatting could be more consistent with the surrounding bullet points.
- - When there several tables that can be used to answer the question, you can combine them in a single SQL query. + - When there are several tables that can be used to answer the question, you can combine them in a single SQL query.api/memory/graphiti_tool.py (2)
518-518: Consider suppressing or renaming unused loop variable.The loop variable
iis not used within the loop body.- for i, result in enumerate(reranked_results, 1): + for _i, result in enumerate(reranked_results, 1):
586-586: Consider improving readability of personal context label.The current formatting might be clearer with standard parentheses formatting.
- memory_context += f"(Personal preferences and information):\n{user_summary}\n\n" + memory_context += f"Personal preferences and information:\n{user_summary}\n\n"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/agents/analysis_agent.py(3 hunks)api/memory/graphiti_tool.py(11 hunks)api/routes/graphs.py(2 hunks)app/ts/modules/chat.ts(0 hunks)
💤 Files with no reviewable changes (1)
- app/ts/modules/chat.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All Python code should pass pylint (use make lint)
Files:
api/agents/analysis_agent.pyapi/routes/graphs.pyapi/memory/graphiti_tool.py
🧬 Code graph analysis (2)
api/routes/graphs.py (2)
app/ts/modules/config.ts (1)
MESSAGE_DELIMITER(5-5)api/memory/graphiti_tool.py (1)
add_new_memory(251-279)
api/memory/graphiti_tool.py (1)
api/config.py (1)
Config(49-135)
🪛 Ruff (0.12.2)
api/memory/graphiti_tool.py
102-102: f-string without any placeholders
Remove extraneous f prefix
(F541)
214-214: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
247-247: Consider moving this statement to an else block
(TRY300)
248-248: Do not catch blind exception: Exception
(BLE001)
248-248: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
456-456: Consider moving this statement to an else block
(TRY300)
458-458: Do not catch blind exception: Exception
(BLE001)
518-518: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
541-541: Consider moving this statement to an else block
(TRY300)
681-681: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (12)
api/agents/analysis_agent.py (3)
240-240: LGTM! Good security practice for email privacy.The addition of the constraint to never use email-based identifiers is a solid security improvement that helps protect user privacy.
257-262: Good implementation of personal context handling.The logic correctly prevents placeholder fabrication when personal context is missing, ensuring queries fail gracefully rather than generating incorrect SQL with made-up placeholders.
290-300: LGTM! Well-structured evaluation steps.The renumbered evaluation guidelines provide clear progression through the analysis process, particularly with the addition of memory context utilization and the critical personalization check.
api/memory/graphiti_tool.py (7)
85-85: Simplified naming convention looks good.The change from
User {user_id}to just{user_id}simplifies the node naming convention while maintaining uniqueness.
184-249: Well-designed user information update method.The implementation properly handles existing summaries, integrates new insights, and maintains a factual third-person style. The error handling with the return of boolean status is appropriate.
251-279: Excellent concurrent processing implementation.The refactored
add_new_memorymethod efficiently runs database episode addition and user information update concurrently usingasyncio.gather, improving performance while maintaining proper error handling.
445-460: Efficient direct database query for user summary.Good refactoring to use direct Cypher queries instead of going through Graphiti search, which should improve performance for fetching user summaries.
462-486: Well-implemented episode extraction method.The method properly extracts episode contents from relationship results, handling multiple episodes efficiently with proper null checking.
488-545: Good combination of episodes and facts in database context.The method effectively combines recent episode contents with database facts to provide comprehensive context. The episode limit prevents overwhelming the context while still providing relevant history.
647-718: Well-structured conversation summarization with history support.The method effectively uses conversation history to provide context for summarization, with proper handling of both cases (with and without history). The database-oriented prompting ensures relevant summaries.
api/routes/graphs.py (2)
496-503: Good conditional emission of query results.The change to only emit query results when they're non-empty prevents unnecessary empty payloads in the response stream.
643-646: History parameters correctly passedqueries_historyandresult_historyare defined earlier (lines 286–303, 690) and correctly passed tomemory_tool.add_new_memory.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/memory/graphiti_tool.py (1)
319-355: Use parameterized Cypher to avoid injection and escaping pitfalls.Building Cypher with string interpolation (escaped_query, escaped_sql, escaped_error) risks injection and brittle escaping. Prefer parameters for all values (relationship type can remain interpolated).
Apply:
- check_query = f""" - MATCH (db:Entity {{uuid: "{database_node_uuid}"}}) - MATCH (db)-[r]->(q:Query) - WHERE q.user_query = "{escaped_query}" AND q.sql_query = "{escaped_sql}" - RETURN q.uuid as existing_query_uuid - LIMIT 1 - """ - check_result = await graph_driver.execute_query(check_query) + check_query = """ + MATCH (db:Entity {uuid: $db_uuid})-[:SUCCESS|:FAILED]->(q:Query) + WHERE q.user_query = $user_query AND q.sql_query = $sql_query + RETURN q.uuid as existing_query_uuid + LIMIT 1 + """ + check_result, _, _ = await graph_driver.execute_query( + check_query, + db_uuid=database_node_uuid, + user_query=query, + sql_query=sql_query, + ) @@ - cypher_query = f""" - MATCH (db:Entity {{uuid: "{database_node_uuid}"}}) - MERGE (q:Query {{ - user_query: "{escaped_query}", - sql_query: "{escaped_sql}", - success: {str(success).lower()}, - error: "{escaped_error}", - timestamp: timestamp(), - embeddings: vecf32($embedding) - }}) - CREATE (db)-[:{relationship_type} {{timestamp: timestamp()}}]->(q) - RETURN q.uuid as query_uuid - """ + cypher_query = f""" + MATCH (db:Entity {{uuid: $db_uuid}}) + MERGE (q:Query {{ user_query: $user_query, sql_query: $sql_query }}) + ON CREATE SET q.success = $success, + q.error = $error, + q.timestamp = timestamp(), + q.embeddings = vecf32($embedding) + ON MATCH SET q.success = $success, + q.error = $error, + q.timestamp = timestamp() + MERGE (db)-[:{relationship_type} {{timestamp: timestamp()}}]->(q) + RETURN q.uuid as query_uuid + """ @@ - result = await graph_driver.execute_query(cypher_query, embedding=embeddings) + _ = await graph_driver.execute_query( + cypher_query, + db_uuid=database_node_uuid, + user_query=query, + sql_query=sql_query, + success=success, + error=error or "", + embedding=embeddings, + )api/agents/analysis_agent.py (1)
266-283: f-string contains literal JSON braces → runtime formatting error.The JSON template inside the f-string needs doubled braces to render literally. As written, Python will try to format them and crash.
- ```json - {{ - "is_sql_translatable": true or false, - "instructions_comments": ("Comments about any part of the instructions, " - "especially if they are unclear, impossible, " - "or partially met"), - "explanation": ("Detailed explanation why the query can or cannot be " - "translated, mentioning instructions explicitly and " - "referencing conversation history if relevant"), - "sql_query": ("High-level SQL query (you must to applying instructions " - "and use previous answers if the question is a continuation)"), - "tables_used": ["list", "of", "tables", "used", "in", "the", "query", - "with", "the", "relationships", "between", "them"], - "missing_information": ["list", "of", "missing", "information"], - "ambiguities": ["list", "of", "ambiguities"], - "confidence": integer between 0 and 100 - }} - ``` + ```json + {{ + "is_sql_translatable": true or false, + "instructions_comments": ("Comments about any part of the instructions, " + "especially if they are unclear, impossible, " + "or partially met"), + "explanation": ("Detailed explanation why the query can or cannot be " + "translated, mentioning instructions explicitly and " + "referencing conversation history if relevant"), + "sql_query": ("High-level SQL query (you must to applying instructions " + "and use previous answers if the question is a continuation)"), + "tables_used": ["list", "of", "tables", "used", "in", "the", "query", + "with", "the", "relationships", "between", "them"], + "missing_information": ["list", "of", "missing", "information"], + "ambiguities": ["list", "of", "ambiguities"], + "confidence": integer between 0 and 100 + }} + ```Alternatively, build the JSON example as a separate non-f string and concatenate.
♻️ Duplicate comments (2)
api/memory/graphiti_tool.py (1)
85-85: Inconsistent user node naming ("User {id}" vs "{id}") will break lookups.Entity nodes use name=f"{user_id}" (Line 85), but clean_memory pins "User {self.user_id}" (Lines 638–639). This inconsistency can cause accidental deletion of the user node.
Apply:
- pinned_user=f"User {self.user_id}", + pinned_user=self.user_id,api/routes/graphs.py (1)
496-504: Prefer truthiness check for results.Use if query_results: instead of len(...) != 0. More Pythonic and handles None safely. This mirrors a prior suggestion.
- if len(query_results) != 0: + if query_results:
🧹 Nitpick comments (8)
api/memory/graphiti_tool.py (7)
404-417: Make vector top-k configurable and consistent with limit.Hardcoding 10 in db.idx.vector.queryNodes may return fewer-than-ideal candidates when limit > 10. Use top_k tied to limit.
Apply:
- CALL db.idx.vector.queryNodes('Query', 'embeddings', 10, vecf32($embedding)) + CALL db.idx.vector.queryNodes('Query', 'embeddings', {max(10, limit*2)}, vecf32($embedding))Alternatively, compute top_k before the f-string and interpolate the integer.
472-487: Batch episode fetches to reduce N+1 queries.extract_episode_from_rel() queries each episode UUID separately. Prefer a single MATCH ... WHERE e.uuid IN $uuids.
Example:
uuids = list(rel_result.episodes or []) if not uuids: return [] q = "MATCH (e:Episodic) WHERE e.uuid IN $uuids RETURN e.content AS content" records, _, _ = await driver.execute_query(q, uuids=uuids) return [r["content"] for r in records if r.get("content")]
70-74: Index creation may fail on re-runs; make it idempotent.CREATE VECTOR INDEX with a fixed label/property can error if it already exists. Guard with try/except or check existence first.
Would you like a helper that probes index existence and creates it only if missing?
102-103: Remove unnecessary f-string.f"The User is using QueryWeaver" triggers Ruff F541. Drop the f prefix.
- 'summary': f'The User is using QueryWeaver', + 'summary': 'The User is using QueryWeaver',
117-119: Use logging instead of print for server code.print statements hinder observability and linting. Replace with logging at appropriate levels.
I can provide a small wrapper or direct diffs if you prefer.
Also applies to: 154-156, 173-176, 275-277, 361-362, 426-427, 544-545
582-616: Docstring vs return type mismatch.search_memories() docstring says it returns a Dict, but it returns a string memory_context.
Update the docstring to match the actual return type.
673-682: Normalize Unicode punctuation in prompts.“2–6 sentences” uses an en dash; prefer ASCII hyphen for linting (RUF001) and consistency.
- - Keep it concise (2–6 sentences). + - Keep it concise (2-6 sentences).api/agents/analysis_agent.py (1)
245-245: Grammar nit: “there” → “there are”.- - When there several tables that can be used to answer the question, you can combine them in a single SQL query. + - When there are several tables that can be used to answer the question, you can combine them in a single SQL query.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/agents/analysis_agent.py(3 hunks)api/memory/graphiti_tool.py(11 hunks)api/routes/graphs.py(2 hunks)app/ts/modules/chat.ts(0 hunks)
💤 Files with no reviewable changes (1)
- app/ts/modules/chat.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All Python code should pass pylint (use make lint)
Files:
api/routes/graphs.pyapi/agents/analysis_agent.pyapi/memory/graphiti_tool.py
🧬 Code graph analysis (2)
api/routes/graphs.py (2)
app/ts/modules/config.ts (1)
MESSAGE_DELIMITER(5-5)api/memory/graphiti_tool.py (1)
add_new_memory(251-279)
api/memory/graphiti_tool.py (1)
api/config.py (1)
Config(49-135)
🪛 Ruff (0.12.2)
api/memory/graphiti_tool.py
102-102: f-string without any placeholders
Remove extraneous f prefix
(F541)
214-214: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
247-247: Consider moving this statement to an else block
(TRY300)
248-248: Do not catch blind exception: Exception
(BLE001)
248-248: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
456-456: Consider moving this statement to an else block
(TRY300)
458-458: Do not catch blind exception: Exception
(BLE001)
518-518: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
541-541: Consider moving this statement to an else block
(TRY300)
681-681: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (1)
api/memory/graphiti_tool.py (1)
488-512: Verify correct Graphiti search API (search vs search_).Elsewhere you use search_ (retrieve_similar_queries), here you call search. Confirm the intended method and align usage to avoid runtime AttributeError.
Would you like me to scan the repo for Graphiti client usages to standardize the method name?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/templates/components/chat_input.j2 (1)
4-4: Remove invalid textarea attribute; consider a11y hint.
type="text"is invalid on<textarea>and should be dropped. Optional: addaria-disabled="true"when disabled for clearer AT signaling.- <textarea type="text" id="message-input" placeholder="Please select a database first..." rows="1" disabled></textarea> + <textarea id="message-input" placeholder="Please select a database first..." rows="1" disabled></textarea>Optional:
- <textarea id="message-input" placeholder="Please select a database first..." rows="1" disabled></textarea> + <textarea id="message-input" placeholder="Please select a database first..." rows="1" disabled aria-disabled="true"></textarea>app/ts/app.ts (1)
267-269: Avoidanyon window with a global type.Declare
updateInputState(and related globals) to dropanycasts.Create
app/ts/global.d.ts:export {}; declare global { interface Window { updateInputState?: () => void; isAuthenticated?: boolean; currentGraph?: string; toggleSidebar?: (containerId: string) => void; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/templates/components/chat_input.j2(1 hunks)app/ts/app.ts(3 hunks)app/ts/modules/graphs.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Frontend source changes in app/**/*.{ts,tsx} must be compiled before production runs
Files:
app/ts/modules/graphs.tsapp/ts/app.ts
🧬 Code graph analysis (1)
app/ts/app.ts (2)
app/ts/modules/config.ts (1)
DOM(39-65)app/ts/modules/graph_select.ts (1)
getSelectedGraph(7-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (4)
app/ts/app.ts (2)
97-99: LGTM on centralizing input handling.Replacing inline logic with
updateInputState()keeps behavior consistent across modules.
264-265: Confirm auth flag is set before updateInputState runs.
We didn’t find any assignment towindow.isAuthenticatedin the TS code beforeDOMContentLoaded→initializeApp→loadInitialData→updateInputState(app/ts/app.ts:264), so ensure the flag is injected or initialized (e.g. via inline script in your HTML) before this point.app/ts/modules/graphs.ts (2)
90-92: LGTM: refresh input state after selection.The guarded global call avoids runtime errors if the function isn’t present.
222-224: LGTM: re-sync input state on graph changes.Keeps the input enablement/placeholder aligned with current selection.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
app/ts/app.ts (3)
271-274: Avoid overriding unauthenticated placeholder on loadOnly run
updateInputState()after graphs load if authenticated, otherwise you’ll clobber the “Please log in…” message set elsewhere.function loadInitialData() { loadGraphs(); - updateInputState(); // Set initial input state based on database selection + if ((window as any).isAuthenticated) { + updateInputState(); // Set initial input state based on database selection + } }
90-95: Honor disabled state on Enter-to-sendEarly-return when the submit button is disabled or the input is empty, so Enter respects UI state.
// replace handler body outside this range if ((e as KeyboardEvent).key === "Enter" && !e.shiftKey) { e.preventDefault(); if (DOM.submitButton?.disabled) return; if (!DOM.messageInput?.value.trim()) return; sendMessage(); }
56-78: Gate by auth and coerce boolean; avoid placeholder regression
updateInputState()overwrites the unauthenticated placeholder and treats selection loosely. Respectwindow.isAuthenticatedfirst and coerce selection to boolean.-function updateInputState() { +function updateInputState() { const submitButton = DOM.submitButton; const messageInput = DOM.messageInput; if (!submitButton || !messageInput) return; - - const selected = getSelectedGraph(); - const isDatabaseSelected = selected && selected !== "Select Database"; - - if (isDatabaseSelected) { + + const isAuthenticated = + (window as any).isAuthenticated !== undefined + ? !!(window as any).isAuthenticated + : false; + if (!isAuthenticated) { + messageInput.disabled = true; + messageInput.placeholder = "Please log in to start chatting"; + submitButton.disabled = true; + return; + } + + const selected = getSelectedGraph(); + const isDatabaseSelected = !!selected && selected !== "Select Database"; + + if (isDatabaseSelected) { messageInput.disabled = false; messageInput.placeholder = "Describe the SQL query you want..."; // Enable submit button only if there's also text in the input - if (messageInput.value.trim()) { - submitButton.disabled = false; - } else { - submitButton.disabled = true; - } + submitButton.disabled = messageInput.value.trim().length === 0; } else { messageInput.disabled = true; messageInput.placeholder = "Please select a database first..."; submitButton.disabled = true; } }
🧹 Nitpick comments (5)
app/public/css/menu.css (1)
526-534: Keep disabled delete controls visually disabled on hoverCurrently disabled delete buttons become fully visible on hover, which can mislead users into thinking they’re actionable. Keep a reduced opacity to signal disabled state while preserving the tooltip/title behavior.
-.dropdown-option:hover .delete-btn:disabled { - opacity: 1; -} +.dropdown-option:hover .delete-btn:disabled { + opacity: 0.5; /* remain visibly disabled on hover */ +}api/routes/auth.py (1)
26-27: Clarify config source and avoid future driftThe comment suggests importing from “graphs route”, but the value is read from env here. Also, to avoid None propagation and ease template handling, default to an empty string. Consider centralizing all runtime config (e.g., api/config.py) so both auth and graphs import the same source.
-# Import GENERAL_PREFIX from graphs route -GENERAL_PREFIX = os.getenv("GENERAL_PREFIX") +# Demo graphs prefix (exposed to client) +GENERAL_PREFIX = os.getenv("GENERAL_PREFIX") or ""app/ts/modules/graph_select.ts (2)
42-44: Avoid innerHTML for SVG icon creationEven though the string is static, linters flag innerHTML. Build the SVG with DOM APIs to eliminate the warning and reduce XSS surface.
- delBtn.innerHTML = `<svg viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"><polyline points="3 6 5 6 21 6"></polyline><path d="M19 6l-1 14a2 2 0 0 1-2 2H8a2 2 0 0 1-2-2L5 6"></path><path d="M10 11v6"></path><path d="M14 11v6"></path><path d="M9 6V4a2 2 0 0 1 2-2h2a2 2 0 0 1 2 2v2"></path></svg>`; + delBtn.appendChild(createTrashIcon());Outside this range, add:
function createTrashIcon(): SVGElement { const ns = "http://www.w3.org/2000/svg"; const svg = document.createElementNS(ns, "svg"); svg.setAttribute("viewBox", "0 0 24 24"); svg.setAttribute("fill", "none"); svg.setAttribute("stroke", "currentColor"); svg.setAttribute("stroke-width", "2"); svg.setAttribute("stroke-linecap", "round"); svg.setAttribute("stroke-linejoin", "round"); const paths = [ { tag: "polyline", attrs: { points: "3 6 5 6 21 6" } }, { tag: "path", attrs: { d: "M19 6l-1 14a2 2 0 0 1-2 2H8a2 2 0 0 1-2-2L5 6" } }, { tag: "path", attrs: { d: "M10 11v6" } }, { tag: "path", attrs: { d: "M14 11v6" } }, { tag: "path", attrs: { d: "M9 6V4a2 2 0 0 1 2-2h2a2 2 0 0 1 2 2v2" } }, ]; for (const p of paths) { const el = document.createElementNS(ns, p.tag); for (const [k, v] of Object.entries(p.attrs)) el.setAttribute(k, v as string); svg.appendChild(el); } return svg; }
52-57: Don’t force-enable submit here; defer to centralized state managementDirectly setting
submitButton.disabled = falseon option click can conflict with the app-levelupdateInputState()(auth, text presence). Let the central logic decide.- if (DOM.graphSelectRefresh && DOM.submitButton) { - // Disable refresh button for demo databases - DOM.graphSelectRefresh.disabled = isDemo; - DOM.submitButton.disabled = false - }; + if (DOM.graphSelectRefresh) { + // Disable refresh button for demo databases + DOM.graphSelectRefresh.disabled = isDemo; + }Optionally trigger the global handler (outside changed range):
(window as any).updateInputState?.();app/ts/app.ts (1)
123-131: Coerce isDemo to boolean for clarity
const isDemo = generalPrefix && selected.startsWith(generalPrefix);yields string|false. Coerce to boolean to avoid accidental truthy string usage.- const generalPrefix = (window as any).generalPrefix; - const isDemo = generalPrefix && selected.startsWith(generalPrefix); + const generalPrefix = (window as any).generalPrefix as string | null; + const isDemo = !!generalPrefix && selected.startsWith(generalPrefix);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
api/routes/auth.py(2 hunks)api/routes/graphs.py(3 hunks)app/public/css/menu.css(1 hunks)app/templates/base.j2(1 hunks)app/ts/app.ts(4 hunks)app/ts/modules/graph_select.ts(2 hunks)app/ts/modules/graphs.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/routes/graphs.py
- app/ts/modules/graphs.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All Python code should pass pylint (use make lint)
Files:
api/routes/auth.py
app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Frontend source changes in app/**/*.{ts,tsx} must be compiled before production runs
Files:
app/ts/app.tsapp/ts/modules/graph_select.ts
🧬 Code graph analysis (2)
app/ts/app.ts (2)
app/ts/modules/config.ts (1)
DOM(39-65)app/ts/modules/graph_select.ts (1)
getSelectedGraph(7-12)
app/ts/modules/graph_select.ts (1)
app/ts/modules/config.ts (1)
DOM(39-65)
🪛 ast-grep (0.38.6)
app/ts/modules/graph_select.ts
[warning] 42-42: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: delBtn.innerHTML = <svg viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"><polyline points="3 6 5 6 21 6"></polyline><path d="M19 6l-1 14a2 2 0 0 1-2 2H8a2 2 0 0 1-2-2L5 6"></path><path d="M10 11v6"></path><path d="M14 11v6"></path><path d="M9 6V4a2 2 0 0 1 2-2h2a2 2 0 0 1 2 2v2"></path></svg>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 42-42: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: delBtn.innerHTML = <svg viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"><polyline points="3 6 5 6 21 6"></polyline><path d="M19 6l-1 14a2 2 0 0 1-2 2H8a2 2 0 0 1-2-2L5 6"></path><path d="M10 11v6"></path><path d="M14 11v6"></path><path d="M9 6V4a2 2 0 0 1 2-2h2a2 2 0 0 1 2 2v2"></path></svg>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (5)
api/routes/auth.py (1)
409-411: LGTM: Exposes general_prefix to templatesPassing GENERAL_PREFIX through the template context is correct and matches the frontend expectation (window.generalPrefix).
app/templates/base.j2 (1)
44-44: LGTM: Safe serialization for globalUsing
tojsonto emitwindow.generalPrefixis correct and safe; falls back to null when unset.app/ts/modules/graph_select.ts (1)
25-32: Backwards-compatible API extensionAdding
isDemo: boolean = falseand reflecting it viadata-is-demokeeps existing callers working and enables per-row behavior.app/ts/app.ts (2)
97-99: LGTM: Centralized input-state refresh on input eventsReplacing ad-hoc logic with
updateInputState()keeps behavior consistent.
276-278: LGTM: Exposing updateInputState globallyMakes it easy for modules like graphs.ts to sync UI on selection changes.
Remove litellm logs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
api/config.py (2)
12-20: Avoid globally disabling the logger; prefer level + no-propagation (and cover both logger names).Disabling a library logger at import-time can mask important errors and surprise downstream apps. Setting ERROR level and stopping propagation keeps noise and PII down without silencing errors. Also, some versions may register the logger as "litellm" instead of "LiteLLM".
Apply:
- # Disable LiteLLM logger that outputs - litellm_logger = logging.getLogger("LiteLLM") - litellm_logger.setLevel(logging.ERROR) - litellm_logger.disabled = True + # Reduce LiteLLM logs without silencing errors; support both logger names + for _name in ("LiteLLM", "litellm"): + _logger = logging.getLogger(_name) + _logger.setLevel(logging.ERROR) + _logger.propagate = False + # Optionally add a NullHandler to avoid "No handler" warnings + if not _logger.handlers: + _logger.addHandler(logging.NullHandler())
22-24: Avoid side effects on import; gate configuration behind an env flag.Running configuration at import can surprise consumers and complicate tests. Gate it so apps can opt out easily.
-# Initialize litellm configuration -configure_litellm_logging() +# Initialize litellm configuration (opt-out via env) +if os.getenv("SUPPRESS_LITELLM_LOGS", "1") == "1": + configure_litellm_logging()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/config.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All Python code should pass pylint (use make lint)
Files:
api/config.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
- GitHub Check: e2e-tests
- GitHub Check: Analyze (python)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
🔇 Additional comments (1)
api/config.py (1)
7-7: LGTM: import logging is appropriate for the new logger config.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
api/memory/graphiti_tool.py (3)
306-359: Cypher injection and brittle string escaping in save_query_memory.Build queries with parameters instead of interpolating escaped strings. Also destructure execute_query results consistently.
- database_result = await graph_driver.execute_query(find_database_query, name=database_node_name) + records, __, _ = await graph_driver.execute_query(find_database_query, name=database_node_name) @@ - if not database_result[0]: # If no records found + if not records: # If no records found print(f"Database entity node {database_node_name} not found") return False - database_node_uuid = database_result[0][0]['uuid'] + database_node_uuid = records[0].get('uuid', '') @@ - escaped_query = query.replace("'", "\\'").replace('"', '\\"') - escaped_sql = sql_query.replace("'", "\\'").replace('"', '\\"') - escaped_error = error.replace("'", "\\'").replace('"', '\\"') if error else "" - embeddings = Config.EMBEDDING_MODEL.embed(escaped_query)[0] + embeddings = Config.EMBEDDING_MODEL.embed(query)[0] @@ - check_query = f""" - MATCH (db:Entity {{uuid: "{database_node_uuid}"}}) - MATCH (db)-[r]->(q:Query) - WHERE q.user_query = "{escaped_query}" AND q.sql_query = "{escaped_sql}" - RETURN q.uuid as existing_query_uuid - LIMIT 1 - """ + check_query = """ + MATCH (db:Entity {uuid: $db_uuid}) + MATCH (db)-[r]->(q:Query) + WHERE q.user_query = $user_query AND q.sql_query = $sql_query + RETURN q.uuid as existing_query_uuid + LIMIT 1 + """ @@ - check_result = await graph_driver.execute_query(check_query) + check_result, __, _ = await graph_driver.execute_query( + check_query, + db_uuid=database_node_uuid, + user_query=query, + sql_query=sql_query, + ) @@ - cypher_query = f""" - MATCH (db:Entity {{uuid: "{database_node_uuid}"}}) - MERGE (q:Query {{ - user_query: "{escaped_query}", - sql_query: "{escaped_sql}", - success: {str(success).lower()}, - error: "{escaped_error}", - timestamp: timestamp(), - embeddings: vecf32($embedding) - }}) - CREATE (db)-[:{relationship_type} {{timestamp: timestamp()}}]->(q) - RETURN q.uuid as query_uuid - """ + cypher_query = f""" + MATCH (db:Entity {{uuid: $db_uuid}}) + MERGE (q:Query {{ user_query: $user_query, sql_query: $sql_query }}) + SET q.success = $success, + q.error = $error, + q.timestamp = timestamp(), + q.embeddings = vecf32($embedding) + MERGE (db)-[:{relationship_type} {{timestamp: timestamp()}}]->(q) + RETURN q.uuid as query_uuid + """ @@ - result = await graph_driver.execute_query(cypher_query, embedding=embeddings) + await graph_driver.execute_query( + cypher_query, + db_uuid=database_node_uuid, + user_query=query, + sql_query=sql_query, + success=success, + error=error or "", + embedding=embeddings, + ) return True
500-511: Critical: function-arg “query” is shadowed by Cypher string in search_database_facts.You pass the Cypher into graphiti_client.search() instead of the user’s search text.
- driver = self.graphiti_client.driver - query = """ + driver = self.graphiti_client.driver + center_node_cypher = """ MATCH (e:Entity {name: $name}) RETURN e.uuid AS uuid """ - result, __, _ = await driver.execute_query(query, name=f"Database {self.graph_id}") - center_node_uuid = result[0].get("uuid", "") - reranked_results = await self.graphiti_client.search( - query=query, + result, __, _ = await driver.execute_query(center_node_cypher, name=f"Database {self.graph_id}") + if not result: + return "" + center_node_uuid = result[0].get("uuid", "") + reranked_results = await self.graphiti_client.search( + query=query, # preserve the user's input center_node_uuid=center_node_uuid, num_results=limit ) @@ - for i, result in enumerate(reranked_results, 1): - if result.source_node_uuid != center_node_uuid and result.target_node_uuid != center_node_uuid: + for rel in reranked_results: + if rel.source_node_uuid != center_node_uuid and rel.target_node_uuid != center_node_uuid: continue if len(episodes_contents) < episode_limit: - episodes_content = await self.extract_episode_from_rel(result) + episodes_content = await self.extract_episode_from_rel(rel) episodes_contents.extend(episodes_content) - fact_entry = f"{result.fact}" + fact_entry = f"{rel.fact}"Also applies to: 516-525
647-706: Summarization: align history typing, normalize, and fix unicode dash.
- Signature should accept Optional[Tuple[List[str], List[str]]] and normalize.
- Replace en dash in prompt (RUF001).
- Avoid “query” as loop var.
- async def summarize_conversation(self, conversation: Dict[str, Any], history: List[Dict[str, Any]]) -> Dict[str, Any]: + async def summarize_conversation( + self, + conversation: Dict[str, Any], + history: Optional[Tuple[List[str], List[str]]] = None, + ) -> Dict[str, Any]: @@ - - Keep it concise (2–6 sentences). + - Keep it concise (2-6 sentences). @@ - if len(history[1]) == 0: - messages = [{"role": "user", "content": prompt}] - else: - messages = [] - for query, result in zip(history[0], history[1]): - messages.append({"role": "user", "content": query}) - messages.append({"role": "assistant", "content": result}) + queries, results = (history or ([], [])) + if len(results) == 0: + messages = [{"role": "user", "content": prompt}] + else: + messages = [] + for q, r in zip(queries, results): + messages.append({"role": "user", "content": q}) + messages.append({"role": "assistant", "content": r}) messages.append({"role": "user", "content": prompt}) @@ - messages=messages, + messages=messages,Also applies to: 694-701
♻️ Duplicate comments (1)
api/memory/graphiti_tool.py (1)
638-639: Pinned user name mismatch can delete the user node.Elsewhere you set user entity name to user_id, but clean_memory excludes "User {self.user_id}". Use self.user_id to avoid accidental deletion. (Previously raised.)
- pinned_user=f"User {self.user_id}", + pinned_user=self.user_id,
🧹 Nitpick comments (7)
api/memory/graphiti_tool.py (7)
70-74: Make vector-index creation idempotent.Creating the same index on every tool initialization can fail or race under concurrency. Wrap with IF NOT EXISTS (if supported) or catch the duplicate-index error.
Apply:
- await driver.execute_query(f"CREATE VECTOR INDEX FOR (p:Query) ON (p.embeddings) OPTIONS {{dimension:{vector_size}, similarityFunction:'euclidean'}}") + try: + await driver.execute_query( + f"CREATE VECTOR INDEX FOR (p:Query) ON (p.embeddings) " + f"OPTIONS {{dimension:{vector_size}, similarityFunction:'euclidean'}}" + ) + except Exception as e: + # Ignore if index already exists + if "already exists" not in str(e).lower(): + raise
85-85: Drop unnecessary f-string.No interpolation needed.
- user_node_name = f"{user_id}" + user_node_name = user_id
445-451: Avoid “query” shadowing; name Cypher clearly.This prevents confusion with other variables named query.
- driver = self.graphiti_client.driver - query = """ + driver = self.graphiti_client.driver + user_summary_cypher = """ MATCH (e:Entity {name: $name}) RETURN e.summary AS summary """ - result, __, _ = await driver.execute_query(query, name=self.user_id) + result, __, _ = await driver.execute_query(user_summary_cypher, name=self.user_id)
537-541: Minor flow nit: move join/return into an else.Not required, but aligns with TRY300.
- facts = "\n".join(database_facts_text) if database_facts_text else "" - episodes = "\n".join(episodes_contents) if episodes_contents else "" - database_context = "Previous sessions:\n" + episodes + "\n\nFacts:\n" + facts - # Join all facts into a single string - return database_context + facts = "\n".join(database_facts_text) if database_facts_text else "" + episodes = "\n".join(episodes_contents) if episodes_contents else "" + database_context = "Previous sessions:\n" + episodes + "\n\nFacts:\n" + facts + return database_context
586-589: Text label consistency (optional).Consider dropping the parentheses around “Personal preferences and information” for consistency with other section headers.
- memory_context += f"(Personal preferences and information):\n{user_summary}\n\n" + memory_context += f"PERSONAL PREFERENCES AND INFORMATION:\n{user_summary}\n\n"
100-101: Use plain underscore for group_id.Storing the literal "_" may be unintended.
- 'group_id': '\\_', + 'group_id': '_',
518-518: Unused enumerate index.Index isn’t used; drop it to satisfy Ruff B007.
- for i, result in enumerate(reranked_results, 1): + for result in reranked_results:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/memory/graphiti_tool.py(11 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All Python code should pass pylint (use make lint)
Files:
api/memory/graphiti_tool.py
🧬 Code graph analysis (1)
api/memory/graphiti_tool.py (1)
api/config.py (1)
Config(62-148)
🪛 Ruff (0.12.2)
api/memory/graphiti_tool.py
214-214: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
247-247: Consider moving this statement to an else block
(TRY300)
248-248: Do not catch blind exception: Exception
(BLE001)
248-248: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
456-456: Consider moving this statement to an else block
(TRY300)
458-458: Do not catch blind exception: Exception
(BLE001)
518-518: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
541-541: Consider moving this statement to an else block
(TRY300)
681-681: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: Analyze (python)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (3)
api/memory/graphiti_tool.py (3)
102-102: LGTM: removed unnecessary f-string.This fixes the Ruff F541 warning.
262-274: Concurrent episode + user update: good pattern.Running add_episode and update_user_information concurrently is appropriate.
251-254: I’ve initiated a search for thesummarize_conversationdefinition.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
api/memory/graphiti_tool.py (3)
501-511: Critical variable shadowing issue in search_database_facts.The local variable
queryshadows the function parameter, causingself.graphiti_client.search()to receive Cypher text instead of the user's search query.This is consistent with the previous review comment about renaming the Cypher variable to avoid shadowing the function argument.
648-701: Critical type annotation and history handling issues.The method has the same issues as
update_user_information:
- Type mismatch: Annotated as
List[Dict[str, Any]]but used asTuple[List[str], List[str]]- History handling: Assumes
history[1]exists without null checks- Variable reuse:
queryandresultvariables shadow loop parametersThis is consistent with previous review comments about unifying history typing and handling None safely.
463-487: N+1 query performance issue in extract_episode_from_rel.The method executes a separate database query for each episode UUID, creating an N+1 query problem.
This is consistent with the previous review comment about batching episode fetches by UUIDs to reduce database round-trips.
🧹 Nitpick comments (1)
api/memory/graphiti_tool.py (1)
682-682: Fix Unicode character in prompt text.The prompt contains an en dash (–) character that should be replaced with a standard hyphen-minus (-) for consistency.
- - Keep it concise (2–6 sentences). + - Keep it concise (2-6 sentences).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/memory/graphiti_tool.py(19 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All Python code should pass pylint (use make lint)
Files:
api/memory/graphiti_tool.py
🧬 Code graph analysis (1)
api/memory/graphiti_tool.py (1)
api/config.py (1)
Config(62-148)
🪛 GitHub Check: CodeQL
api/memory/graphiti_tool.py
[failure] 155-155: Log Injection
This log entry depends on a user-provided value.
This log entry depends on a user-provided value.
[failure] 157-157: Log Injection
This log entry depends on a user-provided value.
This log entry depends on a user-provided value.
[failure] 173-173: Log Injection
This log entry depends on a user-provided value.
This log entry depends on a user-provided value.
[failure] 182-182: Log Injection
This log entry depends on a user-provided value.
This log entry depends on a user-provided value.
🪛 Ruff (0.12.2)
api/memory/graphiti_tool.py
175-175: Do not catch blind exception: Exception
(BLE001)
176-176: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
179-179: Consider moving this statement to an else block
(TRY300)
181-181: Do not catch blind exception: Exception
(BLE001)
182-182: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
215-215: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
248-248: Consider moving this statement to an else block
(TRY300)
249-249: Do not catch blind exception: Exception
(BLE001)
249-249: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
276-276: Do not catch blind exception: Exception
(BLE001)
277-277: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
362-362: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
365-365: Do not catch blind exception: Exception
(BLE001)
366-366: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
427-427: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
430-430: Do not catch blind exception: Exception
(BLE001)
431-431: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
457-457: Consider moving this statement to an else block
(TRY300)
459-459: Do not catch blind exception: Exception
(BLE001)
460-460: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
519-519: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
542-542: Consider moving this statement to an else block
(TRY300)
544-544: Do not catch blind exception: Exception
(BLE001)
545-545: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
620-620: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
645-645: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
682-682: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
716-716: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (4)
api/memory/graphiti_tool.py (4)
447-461: LGTM! Simplified user summary retrieval.The refactor to use direct Cypher queries instead of Graphiti search results improves performance and reliability for user summary retrieval.
516-542: LGTM! Enhanced database facts context structure.The refactor to build comprehensive database context with "Previous sessions" and "Facts" sections provides better structure for the analysis agent.
587-587: LGTM! Improved memory context labeling.The personal context is now clearly labeled as "(Personal preferences and information)" which helps the analysis agent understand the context type.
674-690: LGTM! Enhanced prompt structure for database-oriented summaries.The updated prompt clearly specifies requirements for database-oriented conversational summaries and includes explicit formatting requirements.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
api/memory/graphiti_tool.py (4)
488-546: Critical: Variable shadowing breaks search functionality.The local variable
queryon Line 501 shadows the function parameterquery, causingself.graphiti_client.search()to receive the Cypher string instead of the user's search query.Apply this fix:
try: driver = self.graphiti_client.driver - query = """ + center_node_query = """ MATCH (e:Entity {name: $name}) RETURN e.uuid AS uuid """ - result, __, _ = await driver.execute_query(query, name=f"Database {self.graph_id}") + result, __, _ = await driver.execute_query(center_node_query, name=f"Database {self.graph_id}") center_node_uuid = result[0].get("uuid", "") reranked_results = await self.graphiti_client.search( query=query, # This now correctly uses the function parameter center_node_uuid=center_node_uuid, num_results=limit )Also improve exception logging:
except Exception as e: - logging.error("Error searching database facts for %s: %s", self.graph_id, e) + logging.exception("Error searching database facts for %s", self.graph_id) return ""
462-486: N+1 query issue in extract_episode_from_rel.The method fetches episode content one UUID at a time, creating an N+1 query problem. This should be optimized with batch fetching.
Apply this fix to batch fetch all episodes:
async def extract_episode_from_rel(self, rel_result): """ Extracts the content of episodes associated with a given relationship result. Args: rel_result: An object containing an 'episodes' attribute, which is a list of episode UUIDs. Returns: List of episode content strings corresponding to the provided episode UUIDs. """ driver = self.graphiti_client.driver - query = """ - MATCH (e:Episodic {uuid: $uuid}) + batch_query = """ + MATCH (e:Episodic) + WHERE e.uuid IN $uuids RETURN e.content AS content """ episodes_uuid = rel_result.episodes episode_contents = [] - for episode_uuid in episodes_uuid: - episode_content, _, _ = await driver.execute_query(query, uuid=episode_uuid) - if episode_content: - content = episode_content[0].get("content") - episode_contents.append(content) + if episodes_uuid: + episode_results, _, _ = await driver.execute_query(batch_query, uuids=episodes_uuid) + episode_contents = [row.get("content") for row in episode_results if row.get("content")] return episode_contents
647-647: Critical: Type annotation inconsistency.The method signature shows
history: List[Dict[str, Any]]but the method uses it asTuple[List[str], List[str]](Lines 694-700), which will cause runtime errors.Apply this fix:
- async def summarize_conversation(self, conversation: Dict[str, Any], history: List[Dict[str, Any]]) -> Dict[str, Any]: + async def summarize_conversation( + self, + conversation: Dict[str, Any], + history: Optional[Tuple[List[str], List[str]]] = None, + ) -> Dict[str, Any]:Also fix the history handling:
- if len(history[1]) == 0: + queries, results = (history or ([], [])) + if len(results) == 0: messages = [{"role": "user", "content": prompt}] else: messages = [] - for query, result in zip(history[0], history[1]): - messages.append({"role": "user", "content": query}) + for q, result in zip(queries, results): + messages.append({"role": "user", "content": q}) messages.append({"role": "assistant", "content": result})
184-250: Critical: Multiple issues in update_user_information method.This method has several significant problems:
- Variable shadowing:
queryvariable shadows the method parameter and is reused for different Cypher queries- History type inconsistency: Method expects
Tuple[List[str], List[str]]but calls like Line 270 may pass None- Ambiguous apostrophe: Line 214 uses curly quote instead of ASCII apostrophe
- Exception handling: Catches Exception without logging details
Apply this comprehensive fix:
- async def update_user_information(self, conversation: Dict[str, Any], history: Tuple[List[str], List[str]]) -> bool: + async def update_user_information( + self, + conversation: Dict[str, Any], + history: Optional[Tuple[List[str], List[str]]] = None, + ) -> bool: driver = self.graphiti_client.driver - query = """ + get_summary_query = """ MATCH (u:Entity {name: $user_id}) RETURN u.summary AS summary """ - summary_result, __, _ = await driver.execute_query(query, user_id=self.user_id) + summary_result, __, _ = await driver.execute_query(get_summary_query, user_id=self.user_id) summary = summary_result[0].get("summary", "") if summary_result else "" # Format conversation for summarization conv_text = "" conv_text += f"User: {conversation.get('question', '')}\n" if conversation.get('generated_sql'): conv_text += f"SQL: {conversation['generated_sql']}\n" if conversation.get('error'): conv_text += f"Error: {conversation['error']}\n" if conversation.get('answer'): conv_text += f"Assistant: {conversation['answer']}\n" prompt = f""" You are updating the personal memory of user. ### Inputs 1. Existing user summary (overall + personal info): {summary} 2. Latest Q&A conversational memory: {conv_text} ### Task - - Produce a new user summary of his overall preferences and his personal information. - - *Important*: Ensure that the summary is contain any personal statements or preferences expressed by the user. + - Produce a new user summary of the user's overall preferences and personal information. + - *Important*: Ensure that the summary contains any personal statements or preferences expressed by the user. - Preserve existing personal information, preferences, and tendencies from the old summary. - Integrate any **new insights** about the user's interests, behaviors, or database usage patterns from the latest memory. - If new info refines or corrects older info, update accordingly. - Focus only on **overall and personal information** — do not include temporary query details. - Write in **factual third-person style**, suitable for storage as a user node in a graph. - Try to explicitly divide overall summary, usage preferences and personal information. ### Output An updated user summary. """ try: - if len(history[1]) == 0: + queries, results = (history or ([], [])) + if len(results) == 0: messages = [{"role": "user", "content": prompt}] else: messages = [] - for query, result in zip(history[0], history[1]): - messages.append({"role": "user", "content": query}) + for q, result in zip(queries, results): + messages.append({"role": "user", "content": q}) messages.append({"role": "assistant", "content": result}) messages.append({"role": "user", "content": prompt}) response = completion( model=Config.COMPLETION_MODEL, messages=messages, temperature=0.1 ) # Parse the direct text response (no JSON parsing needed) content = response.choices[0].message.content.strip() - query = """ + update_query = """ MATCH (u:Entity {name: $user_id}) SET u.summary = $summary RETURN u.summary AS summary """ - await driver.execute_query(query, user_id=self.user_id, summary=content) + await driver.execute_query(update_query, user_id=self.user_id, summary=content) return True except Exception as e: + logging.exception("Failed to update user information") return False
🧹 Nitpick comments (6)
api/memory/graphiti_tool.py (6)
445-461: LGTM! Simplified direct user summary retrieval.The refactoring from using
self.graphiti_client.search_to direct Cypher queries simplifies the logic and should be more performant. The error handling is appropriate.Minor: Improve exception logging.
except Exception as e: - logging.error("Error searching user node: %s", e) + logging.exception("Error searching user node") return ""
517-518: Unused loop variable.The loop variable
iis not used within the loop body. This is flagged by static analysis.- for i, result in enumerate(reranked_results, 1): + for result in reranked_results:
619-620: Improve exception logging.Use
logging.exception()for better debugging context.except Exception as e: - logging.error("Error in concurrent memory search: %s", e) + logging.exception("Error in concurrent memory search") return ""
644-645: Improve exception logging.Use
logging.exception()for better debugging context.except Exception as e: - logging.error("Error cleaning memory: %s", e) + logging.exception("Error cleaning memory") return 0
681-681: Fix ambiguous character in prompt.Line 681 uses an en-dash (–) instead of a standard hyphen (-).
- - Keep it concise (2–6 sentences). + - Keep it concise (2-6 sentences).
715-716: Improve exception logging.Use
logging.exception()for better debugging context.except Exception as e: - logging.error("Error in LLM summarization: %s", e) + logging.exception("Error in LLM summarization") return { "database_summary": "" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/memory/graphiti_tool.py(19 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All Python code should pass pylint (use make lint)
Files:
api/memory/graphiti_tool.py
🧬 Code graph analysis (1)
api/memory/graphiti_tool.py (1)
api/config.py (1)
Config(62-148)
🪛 Ruff (0.12.2)
api/memory/graphiti_tool.py
174-174: Do not catch blind exception: Exception
(BLE001)
175-175: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
178-178: Consider moving this statement to an else block
(TRY300)
180-180: Do not catch blind exception: Exception
(BLE001)
181-181: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
214-214: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
247-247: Consider moving this statement to an else block
(TRY300)
248-248: Do not catch blind exception: Exception
(BLE001)
248-248: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
275-275: Do not catch blind exception: Exception
(BLE001)
276-276: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
361-361: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
364-364: Do not catch blind exception: Exception
(BLE001)
365-365: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
426-426: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
429-429: Do not catch blind exception: Exception
(BLE001)
430-430: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
456-456: Consider moving this statement to an else block
(TRY300)
458-458: Do not catch blind exception: Exception
(BLE001)
459-459: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
518-518: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
541-541: Consider moving this statement to an else block
(TRY300)
543-543: Do not catch blind exception: Exception
(BLE001)
544-544: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
619-619: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
644-644: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
681-681: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
715-715: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🪛 GitHub Check: CodeQL
api/memory/graphiti_tool.py
[failure] 173-173: Log Injection
This log entry depends on a user-provided value.
This log entry depends on a user-provided value.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: unit-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (4)
api/memory/graphiti_tool.py (4)
103-103: LGTM! Summary text updated appropriately.The change from
f'User {user_id} is using QueryWeaver'to'The User is using QueryWeaver'simplifies the user node summary while removing the f-string prefix that wasn't needed.
118-118: LGTM! Good logging improvements.The replacement of
Also applies to: 120-120, 155-157, 173-173, 175-175, 181-182
251-274: LGTM! Concurrent execution is a good improvement.The refactoring to run episode addition and user information updates concurrently using
asyncio.gatherwill improve performance. The structured task organization is clean and maintainable.
586-587: LGTM! Memory context formatting looks good.The structured formatting of memory context with clear sections for personal preferences, interaction history, and similar queries will provide good context to the LLM.
…/typescript-5.7.2 Bump typescript from 5.5.4 to 5.7.2
Summary by CodeRabbit
Improvements
New Features
Bug Fixes
UX