Conversation
|
If the env variable is required to function, we should error if it's not set. |
There was a problem hiding this comment.
Pull Request Overview
This PR ensures the POLYGON_API_KEY must be set before startup, adds basic error handling and resource cleanup in the server’s run function, and updates documentation to reflect the failure mode when the key is missing.
- Enforce
POLYGON_API_KEYpresence and raise an error if it’s not set in bothserver.pyandentrypoint.py. - Wrap the server launch in a
try/except/finallyto log errors and close the REST client. - Update README to note that the server will exit if the API key isn’t configured.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/mcp_polygon/server.py | Enforce API key, add try/except/finally around run, clean up client |
| entrypoint.py | Enforce API key check on startup |
| README.md | Document server exit behavior when API key is missing |
Comments suppressed due to low confidence (2)
src/mcp_polygon/server.py:11
- Add a unit test for server initialization to ensure it raises an EnvironmentError when POLYGON_API_KEY is not set.
raise EnvironmentError("POLYGON_API_KEY environment variable not set.")
entrypoint.py:9
- Consider adding a test to verify that
start_server()exits with an EnvironmentError when POLYGON_API_KEY is missing.
raise EnvironmentError("POLYGON_API_KEY environment variable not set.")
| except Exception as exc: | ||
| print(f"Error running Polygon MCP server: {exc}") | ||
| raise | ||
| finally: | ||
| try: | ||
| polygon_client.close() | ||
| except Exception as close_exc: | ||
| print(f"Error closing Polygon REST client: {close_exc}") |
There was a problem hiding this comment.
Catching a broad Exception may mask unexpected errors; consider catching more specific exceptions or allowing critical errors to propagate.
| except Exception as exc: | |
| print(f"Error running Polygon MCP server: {exc}") | |
| raise | |
| finally: | |
| try: | |
| polygon_client.close() | |
| except Exception as close_exc: | |
| print(f"Error closing Polygon REST client: {close_exc}") | |
| except RuntimeError as exc: | |
| print(f"Runtime error while running Polygon MCP server: {exc}") | |
| raise | |
| except ValueError as exc: | |
| print(f"Value error while running Polygon MCP server: {exc}") | |
| raise | |
| finally: | |
| try: | |
| polygon_client.close() | |
| except OSError as close_exc: | |
| print(f"OS error while closing Polygon REST client: {close_exc}") |
| print(f"Error running Polygon MCP server: {exc}") | ||
| raise | ||
| finally: | ||
| try: | ||
| polygon_client.close() | ||
| except Exception as close_exc: | ||
| print(f"Error closing Polygon REST client: {close_exc}") |
There was a problem hiding this comment.
Prefer using a logging framework instead of print statements for error reporting to ensure consistent log management and easier debugging.
| print(f"Error running Polygon MCP server: {exc}") | |
| raise | |
| finally: | |
| try: | |
| polygon_client.close() | |
| except Exception as close_exc: | |
| print(f"Error closing Polygon REST client: {close_exc}") | |
| logging.error(f"Error running Polygon MCP server: {exc}") | |
| raise | |
| finally: | |
| try: | |
| polygon_client.close() | |
| except Exception as close_exc: | |
| logging.error(f"Error closing Polygon REST client: {close_exc}") |
| # Ensure the server process doesn't exit immediately when run as an MCP server | ||
| def start_server(): | ||
| polygon_api_key = os.environ.get("POLYGON_API_KEY", "") | ||
| polygon_api_key = os.environ.get("POLYGON_API_KEY") |
There was a problem hiding this comment.
[nitpick] Retrieval and validation of POLYGON_API_KEY is duplicated in server.py; consider centralizing this logic in a shared helper function.
Summary
POLYGON_API_KEYinserverandentrypointrunTesting
python -m py_compile entrypoint.py src/mcp_polygon/server.py