Repository navigation
Conversation
…and MCP stability fixes
lyonzin
left a comment
There was a problem hiding this comment.
Hey @Hohlas — first of all, thanks for taking the time to fork, dig into the code, and put this together. It's clear you've been using knowledge-rag in production (MetaTrader workflow?) and hit real pain points. The problems you identified are legitimate:
- stdout corruption on MCP stdio — this is a real issue, and we've seen "Failed to connect" reports
- Model cache in
/tmp— absolutely a problem on Linux (reboot = 250MB re-download) - inotify limits — valid crash scenario on Linux with many watched directories
- Exclude patterns — frequently requested feature, especially for large project trees
- Jupyter/ipynb support — good addition to have
That said, after a thorough code review, there are several implementation issues that prevent us from merging this as-is:
1. builtins.print monkey-patch in __init__.py
Placing this in __init__.py means it fires on any import of the package — not just when running as an MCP server. If someone does from mcp_server import Config in their own script, their entire program's print() behavior silently changes. This violates the principle of least surprise. The correct approach is to redirect stdout only inside main() where the MCP server actually starts.
2. _get_top type-check regression
Changing isinstance(val, dict) → isinstance(val, type(default)) weakens the validation and removes the descriptive error message (expected type vs. got type). This was done to support exclude_patterns as a list, but the proper fix is a dedicated function or adding a type parameter — not weakening the existing guard.
3. .ipynb → _parse_json is incorrect
_parse_json does json.dumps(data, indent=2) which for a Jupyter notebook means indexing everything: execution counts, cell IDs, kernel metadata, and worst of all, base64-encoded image outputs (potentially megabytes of garbage). A proper .ipynb parser needs to extract only source content from markdown and code cells.
4. Exclusion logic — duplicated and fragile
The directory exclusion (~30 lines) and file exclusion (~30 lines) are nearly identical — this should be a single _should_exclude() function. The pattern transformation p.replace("**", "").strip("/") strips glob semantics, and the dotfile check (clean_p == ".") works by accident rather than by design. No tests cover any of this.
5. exclude_patterns as top-level YAML key
The config is organized in sections (paths:, documents:, search:, etc.). A bare top-level exclude_patterns: breaks this convention. Should live under documents: or paths:.
6. Documentation in Russian
PATCH_AND_OPTIMIZATION.md is entirely in Russian — the project documentation is in English.
What we're going to do
We're closing this PR, but we're implementing all the good ideas you raised in a proper release (v3.4.0) with:
models_cache_dir— persistent embedding cache ✅exclude_patterns— underdocuments:section, single helper function, with tests ✅.ipynbparser — proper cell extraction (markdown + code source only) ✅- inotify resilience — try-except in
main()✅ - stdout protection — scoped to server startup only ✅
.mqh/.mq4support ✅- Full test coverage for all new features
You'll see your ideas credited in the changelog. Thanks again for the contribution — this kind of feedback from real users is exactly what makes open-source better. 🤝
|
Closing as discussed in the review above. The good ideas from this PR will be implemented in v3.4.0 with proper tests and architecture. Thanks again @Hohlas! |
|
Thanks for the detailed review and for maintaining knowledge-rag. I agree with your implementation concerns, especially around the global builtins.print monkey-patch and the .ipynb parsing. I have switched my setup to the official v3.5.2 release and migrated my config to the upstream format with documents.exclude_patterns. The release covers the main production issues I originally hit in my Codex/MetaTrader workflow: persistent model cache, MCP stdio stability, exclude patterns, .mqh/.mq4 support, .ipynb handling, and inotify resilience. I’ll keep using the upstream package and will report any future issues as small reproducible cases. Thanks again — knowledge-rag is very useful in my day-to-day project workflow. |
This PR introduces critical stability and performance improvements to make the Knowledge RAG server more production-ready and resilient.
🚀 Key Features & Fixes
MCP Protocol Protection (Stdout Redirection): Implemented a global redirect of stdout to stderr. This prevents random library logs or print() calls from corrupting the JSON-RPC stream, fixing the common "Failed to connect" error in VS Code/Claude.
Persistent Model Storage: Added models_cache_dir configuration. Embedding models are now stored in a persistent directory instead of /tmp, preventing repeated 250MB+ downloads after system reboots.
Advanced Path Exclusion Engine: Upgraded the directory traversal logic to support robust, recursive sub-path matching. Now exclude_patterns (like /node_modules/ or .venv) are strictly enforced regardless of nesting depth.
Linux Inotify Resilience: Wrapped the file watcher initialization in a try-except block. The server no longer crashes when system inotify limits are reached, falling back gracefully to manual indexing.
Extended Format Support: Added native support for Jupyter Notebooks (.ipynb) and other text-based extensions.
🛠 Technical Changes
Updated mcp_server/init.py with global print interception.
Modified config.py to support list-based top-level configurations and new path settings.
Enhanced ingestion.py with fnmatch and component-based path verification.
Refactored server.py for better error handling and model initialization.