-
Notifications
You must be signed in to change notification settings - Fork 0
Gl/demo #6
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
Gl/demo #6
Conversation
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.
Pull request overview
This PR improves code quality through linting/formatting enhancements, consolidates the architecture to use a single port (8080) with different endpoints, and adds test skipping when the OPENAI_API_KEY is not set. The changes modernize type annotations (Optional[T] → T | None), introduce new search and PDF conversion tools, and streamline the Docker deployment configuration.
Key Changes
- Consolidates FastAPI and MCP server onto one port (8080) with FastAPI on
/and MCP on/mcp - Adds
@pytest.mark.skipifdecorator to skip integration tests when OPENAI_API_KEY is missing - Modernizes type annotations throughout the codebase to use Python 3.10+ union syntax (T | None)
- Adds new tools: Tavily web search and PDF-to-Markdown conversion
- Improves code formatting consistency across test files and source code
Reviewed changes
Copilot reviewed 26 out of 30 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests_integration/graphite/test_simple_function_call_assistant.py | Adds skipif decorator for OPENAI_API_KEY and improves formatting |
| tests_integration/graphite/simple_function_call_assistant.py | Updates type hints to use union syntax |
| tests_integration/conftest.py | Formatting improvements to error messages |
| tests/tools/local/test_shell.py | Formatting improvements to function calls |
| tests/tools/local/test_local_file_system.py | Formatting improvements and removes blank line |
| tests/tools/data/test_pandas.py | Formatting improvements and trailing comma consistency |
| tests/tools/data/test_csv.py | Removes blank line at start of file |
| tests/adapter/test_routes.py | Formatting improvements to assertions |
| tests/adapter/test_adapter.py | Updates test to check for renamed parameter mcp_transport |
| src/tools/search/tavily_tool.py | Adds new Tavily web search integration tool |
| src/tools/local/shell.py | Modernizes type hints and formatting improvements |
| src/tools/local/local_file_system.py | Formatting improvements across all functions |
| src/tools/local/calculator.py | Formatting improvements to return dictionaries |
| src/tools/files/pdf_to_markdown.py | Adds new PDF to Markdown conversion tool |
| src/tools/data/pandas.py | Modernizes type hints and formatting improvements |
| src/tools/data/csv.py | Formatting improvements and adds strict parameter to zip |
| src/server.py | Comments out some tools and adds new search/PDF tools |
| src/main.py | Rewrites to mount MCP on /mcp endpoint of main FastAPI app |
| src/adapter/routes.py | Modernizes type hints and improves formatting |
| src/adapter/models.py | Modernizes type hints and improves formatting |
| src/adapter/adapter.py | Renames mcp_url to mcp_transport, supports in-memory transport |
| pyproject.toml | Updates dependencies, dev tools, ruff config, and adds mypy |
| docker/Dockerfile.mcp | Removes separate MCP Dockerfile |
| docker/Dockerfile | Creates unified Dockerfile for single-port deployment |
| docker-compose.yml | Simplifies to single service on port 8080 |
| README.md | Updates documentation to reflect single-port architecture |
| .pre-commit-config.yaml | Adds pre-commit hooks for ruff, mypy, and standard checks |
| .gitignore | Formatting improvements to comments |
| .env.example | Formatting improvement |
Comments suppressed due to low confidence (1)
src/adapter/adapter.py:48
- The condition
transport == "http"is checked twice - once on line 44 and again on line 47. The second check on line 47 can never be reached because iftransport == "http", the function returns at line 45. Consider removing the redundant check on line 47 or restructuring the logic.
if transport == "http" or base_url.endswith(f"/{transport}"):
return base_url
if transport == "http":
return f"{base_url}/mcp"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.
Pull request overview
Copilot reviewed 26 out of 30 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (2)
src/adapter/adapter.py:48
- Unreachable code detected. The condition on line 44 already checks if 'transport == "http"', so the subsequent check on line 47 will never be true. This is dead code that should be removed.
if transport == "http":
return f"{base_url}/mcp"
src/server.py:16
- This comment appears to contain commented-out code.
# try:
# from src.tools.data import pandas
# pandas.register_tools(mcp)
# except ImportError as e:
# print(f"Pandas tools not available: {e}")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
improve lint
one port, different endpoint
skip integration tests if no openai api key set