-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add MCP tracking to sessions #19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Add mcps field to Session model to track active MCP servers and populate it from container labels in ContainerManager. Enhance MCP remove command to warn when removing servers used by active sessions. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
# Get MCP list from container labels | ||
mcps_str = labels.get("cubbi.mcps", "") | ||
mcps = ( | ||
[mcp.strip() for mcp in mcps_str.split(",") if mcp.strip()] | ||
if mcps_str | ||
else [] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The code doesn't handle the case where labels
might be None, which could happen if the container object doesn't have labels. This could lead to an AttributeError when trying to call labels.get()
. [possible issue, importance: 8]
# Get MCP list from container labels | |
mcps_str = labels.get("cubbi.mcps", "") | |
mcps = ( | |
[mcp.strip() for mcp in mcps_str.split(",") if mcp.strip()] | |
if mcps_str | |
else [] | |
) | |
# Get MCP list from container labels | |
labels = labels or {} | |
mcps_str = labels.get("cubbi.mcps", "") | |
mcps = ( | |
[mcp.strip() for mcp in mcps_str.split(",") if mcp.strip()] | |
if mcps_str | |
else [] | |
) |
# Check it ran successfully with exit code 0 | ||
assert result.exit_code == 0 | ||
assert "Removed MCP server 'test-mcp'" in result.stdout | ||
# Check warning about affected sessions | ||
assert ( | ||
"Warning: Found 2 active sessions using MCP 'test-mcp'" in result.stdout | ||
) | ||
assert "session-1" in result.stdout | ||
assert "session-3" in result.stdout | ||
# session-2 should not be mentioned since it doesn't use test-mcp | ||
assert "session-2" not in result.stdout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The test is asserting that "session-1" and "session-3" are in the output, but it doesn't verify that these are the only sessions mentioned. If the implementation incorrectly included other sessions, this test would still pass. [general, importance: 5]
# Check it ran successfully with exit code 0 | |
assert result.exit_code == 0 | |
assert "Removed MCP server 'test-mcp'" in result.stdout | |
# Check warning about affected sessions | |
assert ( | |
"Warning: Found 2 active sessions using MCP 'test-mcp'" in result.stdout | |
) | |
assert "session-1" in result.stdout | |
assert "session-3" in result.stdout | |
# session-2 should not be mentioned since it doesn't use test-mcp | |
assert "session-2" not in result.stdout | |
# Check it ran successfully with exit code 0 | |
assert result.exit_code == 0 | |
assert "Removed MCP server 'test-mcp'" in result.stdout | |
# Check warning about affected sessions | |
assert ( | |
"Warning: Found 2 active sessions using MCP 'test-mcp'" in result.stdout | |
) | |
# Verify exactly which sessions are mentioned | |
assert "session-1" in result.stdout | |
assert "session-3" in result.stdout | |
# session-2 should not be mentioned since it doesn't use test-mcp | |
assert "session-2" not in result.stdout | |
# Verify no other session IDs are mentioned | |
for session in mock_sessions: | |
if session.id not in ["session-1", "session-3"]: | |
assert session.id not in result.stdout |
User description
Add mcps field to Session model to track active MCP servers and populate it from container labels in ContainerManager. Enhance MCP remove command to warn when removing servers used by active sessions.
🤖 Generated with Claude Code
PR Type
Enhancement
Description
Track MCP servers used by sessions
Warn when removing MCPs used by active sessions
Extract MCPs from container labels
Add comprehensive tests for MCP tracking
Changes walkthrough 📝
models.py
Add MCP tracking field to Session model
cubbi/models.py
mcps
field to Session model to track associated MCP serverscontainer.py
Extract MCP data from container labels
cubbi/container.py
test_mcp_commands.py
Add comprehensive tests for MCP tracking
tests/test_mcp_commands.py