fix(knowledge-graph): comprehensive Neo4j backend and tool improvements #269
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
|
@codex review this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
942
to
949
| for (const [key, filter] of Object.entries(filters)) { | ||
| const paramKey = `${prefix}_${Object.keys(params).length}`; | ||
|
|
||
| if (typeof filter === 'object' && filter !== null && !Array.isArray(filter)) { | ||
| // Range filters | ||
| if ('gte' in filter) { | ||
| constraints.push(`${prefix}.${Object.keys(params).length} >= $${paramKey}_gte`); | ||
| constraints.push(`${prefix}.${key} >= $${paramKey}_gte`); | ||
| params[`${paramKey}_gte`] = filter.gte; |
There was a problem hiding this comment.
User‑supplied filter keys are interpolated directly into the Cypher snippets (${prefix}.${key} …). If a caller provides a property name containing characters like spaces, ), or Cypher keywords, the generated query becomes syntactically invalid or can be used to smuggle additional clauses, because property identifiers in Cypher aren’t parameterised. The previous implementation produced incorrect property names but at least didn’t expose an injection surface. Please escape or validate key (e.g. with backticks and a whitelist such as /^[A-Za-z_][A-Za-z0-9_]*$/) before interpolating it into the query string.
Useful? React with 👍 / 👎.
Collaborator
|
@PROJECT-OCULUS I've just resolved an issue with Neo4j filter property keys by adding proper escaping. Please rebase from main to resolve conflicts |
…nager - Adds public getBackendType() method to properly expose backend type - Fixes Cypher query rejection despite successful Neo4j connection - Updates query-graph test mocks to include getBackendType() - Adds test coverage for backend type validation - Fixes pre-existing test missing mock method Problem: Neo4j connection succeeded but Cypher queries were rejected because the query-graph tool could not detect the backend type. The tool checked kgManager.getBackendType() which didn't exist, causing it to default to 'memory' and incorrectly reject Cypher queries. Solution: Added getBackendType() method to KnowledgeGraphManager that: - Delegates to backend instance when connected (source of truth) - Returns cached metadata when disconnected - Handles all lifecycle states (pre-connection, connected, disconnected) Changes: - manager.ts: Added getBackendType() method with comprehensive JSDoc - query-graph.test.ts: Updated mocks and added validation tests - All 16 tests passing Impact: - Cypher queries now work correctly with Neo4j - Backend type detection accurate in all scenarios - No breaking changes (purely additive) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Validates backend type before executing Cypher queries - Provides clear error messages for in-memory backend - Suggests alternative query types (node, edge, path) - Updated tool description to document backend requirements - Part of Neo4j backend detection fix (complements getBackendType() method) This ensures users get helpful feedback when attempting Cypher queries on backends that don't support them, improving the developer experience. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…attern matching - Enhanced JSON extraction with multiple fallback patterns - Added extractJSON() helper function for robust LLM response parsing - Improved fallback pattern matching with more comprehensive regex patterns - Better handling of LLM responses that use code blocks or varying formats - Enhanced error recovery when LLM returns malformed JSON These improvements make the relationship manager more resilient to variations in LLM output formatting, reducing parse failures and improving the overall reliability of relationship operations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…rror logging - Fixed filter constraint building to use actual property keys instead of counter - Previously used Object.keys(params).length as placeholder, causing incorrect queries - Now correctly references the key from the filter entry (e.g., 'age', 'name') - Applies to range filters (gte, gt, lte, lt) and array filters (any, all) - Improved error logging: changed console.error to this.logger.error for consistency This fix ensures that Neo4j queries with property filters work correctly, resolving issues where filter conditions weren't applied to the intended fields. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Added escapeRegex() helper function to properly escape special characters - Fixes "Invalid regular expression: /\bC++\b/: Nothing to repeat" error - Enables correct matching of technologies like C++, .NET, C#, etc. - Prevents regex quantifiers (+, *, ?, etc.) from breaking pattern matching - Applies to all technology/tool name extraction patterns Without this fix, entity extraction would crash when encountering technology names containing regex special characters, preventing proper knowledge graph population for many common programming languages and tools. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…l definition - Made metadata field optional in ReasoningTraceSchema (line 57) - Aligns Zod schema with tool parameter definition (which already marks it as optional) - Resolves validation failures when reasoning evaluation called without metadata - Allows reasoning evaluation to work with minimal trace information Previously, the schema required metadata even though the tool definition marked it as not required, causing validation errors. This fix ensures consistency between the schema validation and the tool's actual requirements. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Removed [DIAGNOSTIC-FACTORY] console logging statements - Removed verbose debug logging around manager creation - Cleaned up excessive debug statements for cleaner production code - Kept essential logging for actual errors and info messages This removes temporary debugging code that was added during development, keeping only the necessary logging for production operation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Author
Update on Security FeedbackThe property key escaping concern raised in the review has been addressed. The escapePropertyKey() method is implemented at lines 1103-1114 in neo4j.ts: This implementation:
The regex escape fix for entity extraction (C++ issue) is also in place at lines 447-455 in extract-entities.ts using an escapeRegex() helper. Regarding Rebase@RyanNg1403 - I can rebase this PR from main to resolve any conflicts. Would you like me to proceed with that, or would you prefer to handle the rebase yourself? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes several critical bugs in the knowledge graph system and improves the robustness of knowledge graph tools. All fixes are backward compatible with zero breaking changes.
Critical Fixes
1. Neo4j Cypher Query Support 🔧
getBackendType()method inKnowledgeGraphManagersrc/core/knowledge_graph/manager.ts2. Neo4j Filter Constraint Bug 🐛
Object.keys(params).lengthcounter instead of actual property names)Object.entries()src/core/knowledge_graph/backend/neo4j.ts3. Entity Extraction Crash 💥
escapeRegex()helper to properly escape special characterssrc/core/brain/tools/definitions/knowledge_graph/extract-entities.ts4. Schema Validation Error 📋
metadatarequired in reasoning trace but tool definition allows optionalmetadataoptional inReasoningTraceSchemasrc/core/brain/tools/def_reflective_memory_tools.tsImprovements
5. Enhanced JSON Parsing 🔄
src/core/brain/tools/definitions/knowledge_graph/relationship-manager.ts6. Backend Validation ✅
src/core/brain/tools/definitions/knowledge_graph/query-graph.ts7. Code Cleanup 🧹
src/core/knowledge_graph/factory.tsTesting
Unit Tests
query-graph.test.tsgetBackendType()methodTest Coverage
Modules Tested
MCP Tool Integration Tests
TypeScript Compilation
Breaking Changes
None - All changes are additive or fix existing bugs without altering public APIs.
Files Changed
8 files modified (100% knowledge graph system):
src/core/knowledge_graph/manager.ts(+57 lines)src/core/knowledge_graph/backend/neo4j.ts(+10, -9 lines)src/core/knowledge_graph/factory.ts(+1, -6 lines)src/core/brain/tools/definitions/knowledge_graph/query-graph.ts(+23 lines)src/core/brain/tools/definitions/knowledge_graph/relationship-manager.ts(+104, -18 lines)src/core/brain/tools/definitions/knowledge_graph/extract-entities.ts(+11 lines)src/core/brain/tools/definitions/knowledge_graph/__test__/query-graph.test.ts(+67 lines)src/core/brain/tools/def_reflective_memory_tools.ts(+5 lines)Checklist
Related Issues
Fixes backend detection issue where Neo4j connection succeeded but Cypher queries were incorrectly rejected.
Additional Context
This PR integrates cleanly with recent upstream changes:
The changes enable proper Neo4j backend utilization while maintaining full compatibility with in-memory backend operations.
🤖 Generated with assistance from Claude Code