ci: Add Python CI workflow (format, lint, test)#15
Conversation
Matches claude-event-bus CI configuration. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Code ReviewThanks for adding CI to the project! This is an important step for code quality. I've identified several issues that need to be addressed before this workflow can run successfully. Critical Issues ❌1. Missing pyproject.toml The test job attempts to install dependencies with Action Required: Add a
2. No Tests to Run The Action Required: Either:
Recommendations 💡3. Unnecessary Matrix Strategy Lines 30-32 define a matrix with only one Python version. This adds complexity without benefit. strategy:
matrix:
python-version: ["3.12"]Suggestion: Simplify to a single version: - uses: actions/setup-python@v5
with:
python-version: "3.12"If you plan to test multiple versions in the future, the matrix makes sense, but for now it's premature. 4. Missing Ruff Configuration While ruff can run with defaults, having a 5. Workflow Dependencies Consider making the test job depend on the check job passing first: test:
name: Test Suite
needs: check
runs-on: ubuntu-latestThis saves CI minutes by not running tests if linting fails. Minor Issues6. Inconsistent Formatting The workflow follows the pattern from claude-event-bus (as mentioned in the PR description), which is good for consistency. No issues here. Security ✅No security concerns identified. The workflow:
Testing Coverage
|
🤖 Generated with [Claude Code](https://claude.com/claude-code)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
PR Review: CI Workflow SetupSummaryThis PR adds foundational CI infrastructure with format/lint/test jobs. Overall structure is solid and follows established patterns from ✅ Strengths
🔍 Issues & RecommendationsCritical1. Missing
|
PR Review: CI Workflow AdditionOverall AssessmentGood foundational PR that adds essential CI infrastructure. The implementation follows the reference architecture from claude-event-bus and aligns well with the project goals. However, there are several areas that need attention before merging. Critical Issues1. Missing
|
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Code ReviewThis PR establishes the foundational CI infrastructure and project skeleton. Overall the implementation is clean and follows best practices, but there are several items that need attention before merging. ✅ Strengths
🐛 Critical Issues1. Missing
|
🤖 Generated with [Claude Code](https://claude.com/claude-code)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
PR Review: CI Infrastructure SetupSummaryThis PR adds foundational CI/CD infrastructure with GitHub Actions for formatting, linting, and testing. Overall, this is a solid start that follows Python best practices. ✅ Strengths
🐛 Issues & BugsCritical1. Missing from session_analytics.server import mcp
__all__ = ["mcp"]2. Python version inconsistency (pyproject.toml:19 vs .github/workflows/python.yml:31)
High Priority3. Missing ruff installation in dependencies (pyproject.toml:32-34)
4. Incomplete test coverage
🔍 Code Qualitypyproject.tomlLine 40-41: Incorrect package configuration [tool.hatch.build.targets.wheel]
packages = ["src/session_analytics"]This should be: packages = ["src"]The current config will look for Line 53: Consider stricter linting select = ["E", "F", "I", "N", "W", "UP"]Consider adding:
server.pyLine 7-12: Redundant function @mcp.tool()
def get_status() -> dict:
"""Get ingestion status and database stats."""
return {
"status": "ok",
"message": "Session analytics server is running",
}Missing type hints README.mdLine 32-34: Incomplete tool list
⚡ PerformanceNo performance concerns at this stage. FastMCP is appropriately lightweight for an MCP server. 🔒 SecurityLow: Hardcoded paths in CLAUDE.md 🧪 Test Coverage RecommendationsAdd these test cases to import pytest
from session_analytics.server import mcp
def test_mcp_server_has_tools():
"""Verify that tools are registered with FastMCP."""
tools = mcp.list_tools()
assert "get_status" in [tool.name for tool in tools]
def test_get_status_structure():
"""Test that get_status returns required fields."""
from session_analytics.server import _get_status_impl
result = _get_status_impl()
assert isinstance(result, dict)
assert result["status"] == "ok"
assert isinstance(result["message"], str)
@pytest.mark.asyncio
async def test_mcp_server_starts():
"""Test that the MCP server can be instantiated."""
# This ensures the server doesn't crash on import
assert mcp is not None
assert mcp.name == "session-analytics"📋 Action ItemsMust fix before merge:
Should fix:
Nice to have:
🎯 VerdictRequest Changes - The Python version mismatch and package configuration issues need resolution before merge. The foundation is solid, but these issues will cause problems in production. Great work on getting CI set up! This is exactly the right infrastructure to build on. 🤖 Generated with Claude Code |
PR Review: Python CI WorkflowThanks for setting up the CI infrastructure! This is a solid foundation. Here's my detailed review: ✅ Strengths
🔍 Code Quality & Best Practicespyproject.toml (pyproject.toml:1-54)
CI Workflow (.github/workflows/python.yml:1-43)
Server Implementation (src/session_analytics/server.py:1-28)
Tests (tests/test_server.py:1-10)
README (README.md:1-34)
🐛 Potential Issues
🔒 Security Concerns
⚡ Performance Considerations
🧪 Test Coverage
📝 RecommendationsHigh Priority:
Medium Priority: Nice to Have: ✅ VerdictThis is a solid first PR that establishes the CI foundation correctly. The code is clean, follows best practices, and aligns with the FastMCP patterns. The issues identified are minor and mostly about consistency and future-proofing. Once the Python version matrix issue is resolved, this is ready to merge. Would you like me to suggest specific fixes for any of these issues? |
Summary
Test plan
🤖 Generated with Claude Code