Add create_entity and delete_entity MCP tools#35
Conversation
📝 WalkthroughWalkthroughAdds two MCP tools— Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant MCP as MCP_Server
participant NS as NamespaceManager
participant KC as KaizenClient
participant MB as MilvusBackend
Client->>MCP: create_entity(content, type, metadata?, enable_conflict_resolution)
MCP->>NS: ensure_namespace()
NS-->>MCP: namespace_ok
MCP->>MCP: parse metadata JSON or return error
MCP->>KC: update_entities([Entity], enable_conflict_resolution)
KC->>MB: insert/update vectors & metadata
MB-->>KC: updates_result or error
KC-->>MCP: updates_result
MCP-->>Client: JSON response with event/id/type/content/metadata or error
Client->>MCP: delete_entity(entity_id)
MCP->>NS: ensure_namespace()
NS-->>MCP: namespace_ok
MCP->>KC: delete_entity_by_id(entity_id)
KC->>MB: check existence -> delete by id
MB-->>KC: success or error
KC-->>MCP: success or KaizenException
MCP-->>Client: JSON success/error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@kaizen/frontend/mcp/mcp_server.py`:
- Around line 141-149: The code currently calls json.loads(metadata) and will
crash on malformed JSON; wrap the parse in a try/except that catches
json.JSONDecodeError (or Exception) when building metadata_dict and return a
structured error response instead of proceeding to create the Entity; update the
logic around metadata_dict and Entity(...) so on parse failure you return or
raise a clear, structured error (including the invalid metadata/error message)
rather than letting the server crash.
- Around line 186-198: The except block around client.delete_entity_by_id(...)
should catch the specific KaizenException rather than Exception and should call
logger.exception(...) to preserve the traceback: replace "except Exception as
e:" with "except KaizenException as e:" (import KaizenException where needed)
and change logger.error(...) to logger.exception(...) while keeping the same
logged message about deleting entity {entity_id}; leave other behavior
(returning the JSON error response) unchanged.
🧹 Nitpick comments (1)
tests/e2e/test_mcp.py (1)
67-90: Add skipif guard to prevent test failure when LLM credentials are unavailable.The conflict resolution feature uses LLM (via litellm with gpt-4o model), which requires OPENAI_API_KEY or equivalent credentials. The test will fail without them.
Suggested guard
`@pytest.mark.e2e` +@pytest.mark.skipif( + not os.getenv("OPENAI_API_KEY"), + reason="Conflict resolution requires LLM credentials" +) async def test_create_entity_with_conflict_resolution(mcp):
visahak
force-pushed
the
feature/add-create-delete-entity-mcp-tools
branch
from
8902fec to
0fb2968
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/e2e/test_mcp.py`:
- Around line 200-212: The test test_delete_nonexistent_entity is wrong to
expect an error because delete_entity_by_id calls milvus.delete (which is
idempotent) and the MCP handler therefore returns success: True; either change
the test to assert result['success'] is True and remove the error expectation,
or change the implementation in delete_entity_by_id to first check existence
(e.g., via a get/query method) and raise/return an error when the entity_id does
not exist so the MCP handler returns success: False—update the test accordingly
to match the chosen behavior; reference test_delete_nonexistent_entity,
delete_entity_by_id, and milvus.delete when making the change.
🧹 Nitpick comments (2)
kaizen/backend/milvus.py (1)
200-206: Consider settingself.milvus = Noneafter closing to prevent potential double-close.The
SQLiteManager.close()pattern (fromkaizen/db/sqlite_manager.pylines 125-128) sets the resource toNoneafter closing, which guards against double-close issues and makes the closed state inspectable. Consider applying the same pattern here.♻️ Suggested improvement
def close(self): """Close Milvus connection.""" try: if hasattr(self, 'milvus'): self.milvus.close() + self.milvus = None except Exception as e: logger.warning(f"Error closing Milvus client: {e}")Regarding the static analysis hint about catching broad
Exception(BLE001): this is acceptable in cleanup code where we want best-effort teardown without propagating errors.tests/e2e/test_mcp.py (1)
14-82: Good test isolation pattern with per-test DB files.The fixture properly:
- Creates unique DB files to avoid socket/locking conflicts between tests
- Resets the MCP server's singleton client
- Performs thorough multi-stage cleanup
Regarding the static analysis hints about
try-except-pass(S110, BLE001): in test fixture cleanup, broad exception handling with pass is generally acceptable since cleanup should be best-effort and not mask test failures. However, consider adding minimal logging for easier debugging when cleanup fails:♻️ Optional: Add logging for cleanup debugging
+import logging +logger = logging.getLogger(__name__) + # In cleanup blocks, e.g.: try: kaizen_client.backend.close() - except Exception: - pass + except Exception as e: + logger.debug(f"Cleanup: backend.close() failed: {e}")
visahak
force-pushed
the
feature/add-create-delete-entity-mcp-tools
branch
from
00ad2fd to
40c4a4b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@kaizen/backend/milvus.py`:
- Around line 196-201: The except block that converts entity_id to int should
chain the original ValueError when raising KaizenException to preserve the
traceback; modify the except ValueError handler in the method that calls
int(entity_id) so it captures the original exception (e.g., except ValueError as
e) and raises KaizenException(f"Invalid entity ID: {entity_id}. Entity IDs must
be numeric.") from e; keep the subsequent calls to
self.validate_namespace(namespace_id) and self.milvus.delete(...) unchanged.
🧹 Nitpick comments (1)
tests/e2e/test_mcp.py (1)
14-82: Good isolation strategy; bare exceptions acceptable for test teardown.The per-test unique DB file approach (
test_{uuid}.db) effectively addresses Milvus Lite socket/locking issues. The comprehensive cleanup sequence (close backend → disconnect pymilvus → release servers → reset client → restore URI → remove files) is thorough.While Ruff flags the
try-except-passblocks (S110, BLE001), these are acceptable in test teardown code where resilience takes priority—failures during cleanup should not mask test results. Consider adding minimal logging if debugging becomes necessary in the future.One concern: line 36-37 silently swallows namespace creation failures. If
create_namespacefails for reasons other than "already exists" (e.g., connection issues), tests may proceed with an invalid state.♻️ Optional: handle specific exception for namespace creation
- try: - kaizen_client.create_namespace('test') - except Exception: - pass + from kaizen.schema.exceptions import NamespaceAlreadyExistsException + try: + kaizen_client.create_namespace('test') + except NamespaceAlreadyExistsException: + pass # Expected if namespace already exists
| try: | ||
| entity_id_int = int(entity_id) | ||
| except ValueError: | ||
| raise KaizenException(f"Invalid entity ID: {entity_id}. Entity IDs must be numeric.") | ||
| self.validate_namespace(namespace_id) | ||
| self.milvus.delete(collection_name=namespace_id, ids=[entity_id]) | ||
| self.milvus.delete(collection_name=namespace_id, ids=[entity_id_int]) |
There was a problem hiding this comment.
Chain the exception for better traceability.
The validation logic is correct, but per Ruff B904, exceptions raised within except blocks should use raise ... from to preserve the original traceback.
🔧 Proposed fix
def delete_entity_by_id(self, namespace_id: str, entity_id: str):
try:
entity_id_int = int(entity_id)
except ValueError:
- raise KaizenException(f"Invalid entity ID: {entity_id}. Entity IDs must be numeric.")
+ raise KaizenException(f"Invalid entity ID: {entity_id}. Entity IDs must be numeric.") from None
self.validate_namespace(namespace_id)
self.milvus.delete(collection_name=namespace_id, ids=[entity_id_int])🧰 Tools
🪛 Ruff (0.14.14)
199-199: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
199-199: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@kaizen/backend/milvus.py` around lines 196 - 201, The except block that
converts entity_id to int should chain the original ValueError when raising
KaizenException to preserve the traceback; modify the except ValueError handler
in the method that calls int(entity_id) so it captures the original exception
(e.g., except ValueError as e) and raises KaizenException(f"Invalid entity ID:
{entity_id}. Entity IDs must be numeric.") from e; keep the subsequent calls to
self.validate_namespace(namespace_id) and self.milvus.delete(...) unchanged.
There was a problem hiding this comment.
@visahak did you run ruff check before that? we have the ruff pre-commit hook setup
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@gaodan-fang not really but the tests passed. did I miss something?
There was a problem hiding this comment.
I just ran it, no issues in the committed files.
- Add create_entity tool to create single entities with optional conflict resolution - Add delete_entity tool to delete entities by ID - Add 7 comprehensive test cases for new tools - Update README.md to document new MCP tools - All 172 tests passing Fixes AgentToolkit#34
visahak
force-pushed
the
feature/add-create-delete-entity-mcp-tools
branch
from
8bb6594 to
e37cd6b
Compare
|
@visahak you need to update the branch with the base branch before merge |
2 participants
Description
Implements create and delete entity MCP tools as requested in #34.
Changes
New MCP Tools (
kaizen/frontend/mcp/mcp_server.py):create_entity: Creates single entities using existingclient.update_entities()methoddelete_entity: Deletes entities by ID using existingclient.delete_entity_by_id()methodTests (
tests/e2e/test_mcp.py):Documentation (
README.md):Features
✅ Uses existing KaizenClient API methods
✅ Supports optional conflict resolution
✅ Proper error handling
✅ Metadata support
✅ Comprehensive test coverage
✅ All tests passing (172/172)
Testing
Fixes #34
Files Changed
kaizen/frontend/mcp/mcp_server.py- Added 2 new MCP tools (80 lines)tests/e2e/test_mcp.py- Added 7 test cases (157 lines)README.md- Updated documentation (2 lines)Test Results
✅ All 172 tests passing (including Phoenix tests)
✅ All new MCP tools fully tested
✅ No breaking changes to existing functionality
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.