Remove legacy local mode and standardize on Agent API - #216
Conversation
GitHub Actions
Error
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request simplifies the Data Commons MCP server by removing the local processing mode and migrating entirely to the Agent API client, which includes removing the datacommons-client dependency and refactoring tools, services, and settings. The review identified two critical issues: a potential security vulnerability where agent_api_root is not validated or normalized, and a runtime NameError in utils.py due to storage being used in _get_gcs_client() while only imported under TYPE_CHECKING.
GitHub Actions
Error
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Data Commons MCP server to migrate fully to the Agent API, removing local processing, custom Data Commons configurations, and the datacommons-client dependency. The initialization process, settings, services, and tools are simplified to directly call the Agent API endpoints, and a context-based API key override mechanism is introduced. The review feedback recommends two improvements: refactoring APIKeyMiddleware to a pure ASGI middleware to prevent contextvars propagation issues associated with BaseHTTPMiddleware, and defaulting the date parameter in get_observations to a safe, defensive value ('latest') to handle cases where the caller fails to provide a value.
| child_place_type: str | None = None, | ||
| source_override: str | None = None, | ||
| date: str = ObservationDateType.LATEST.value, | ||
| date: str | None = None, |
There was a problem hiding this comment.
To align with the general rule of defaulting tool parameters to safe, defensive values (like 'latest') rather than None, and to maintain consistency with get_multi_entity_observations (which defaults date to ObservationDateType.LATEST.value), we should default date to ObservationDateType.LATEST.value here as well. This protects against cases where the caller or agent fails to provide a value.
| date: str | None = None, | |
| date: str | None = ObservationDateType.LATEST.value, |
References
- When defining tool parameters (such as date filters) for LLM agents, default to a safe, defensive value (e.g., 'latest') rather than None to protect against the agent failing to provide a value.
There was a problem hiding this comment.
No we want latest to be the default.
There was a problem hiding this comment.
why not set default to latest here?
There was a problem hiding this comment.
Sorry, I had read this backward. Updated now so latest is default. Thanks for the catch!
GitHub Actions
Error
GitHub Actions
Error
2 participants
datacommons-clientdependency.DCSettingsmodel configured viaDC_AGENT_API_ROOT.APIKeyMiddleware.DC_USE_AGENT_API,DC_API_ROOT).