fix: use default SQLite path in tool registry#96
Conversation
- Update build_tool_config_from_env to use default SQLite path when SQLITE_PATH env var is not set - Match behavior of server.py lifespan which already uses default - Update test to expect default path instead of None
There was a problem hiding this comment.
Code Review
This pull request introduces a default path for the SQLite database, ensuring it defaults to a .adk/tools.db file within the agent directory if no environment variable is provided. The tests have been updated to reflect this change. Review feedback suggests that the default path calculation for the agent directory might be off by one level, pointing to 'src/' instead of the project root, and recommends simplifying the logic for fetching the SQLite path environment variable.
| import os | ||
|
|
||
| skills_dir = Path(__file__).parent / "skills" | ||
| agent_dir = os.getenv("AGENT_DIR", str(Path(__file__).resolve().parent.parent)) |
There was a problem hiding this comment.
The default path calculation for agent_dir appears to point to the src/ directory instead of the project root. Given the file is located at src/blacki/registry.py, Path(__file__).resolve().parent.parent resolves to src/. Typically, configuration and data directories like .adk should reside in the project root. Consider going up one more level to reach the root.
| agent_dir = os.getenv("AGENT_DIR", str(Path(__file__).resolve().parent.parent)) | |
| agent_dir = os.getenv("AGENT_DIR", str(Path(__file__).resolve().parent.parent.parent)) |
| sqlite_path_env = os.getenv("SQLITE_PATH", "").strip() or None | ||
| sqlite_path = sqlite_path_env or default_sqlite_path |
There was a problem hiding this comment.
The logic for determining sqlite_path can be simplified by removing the intermediate sqlite_path_env variable and using the fallback directly.
| sqlite_path_env = os.getenv("SQLITE_PATH", "").strip() or None | |
| sqlite_path = sqlite_path_env or default_sqlite_path | |
| sqlite_path = os.getenv("SQLITE_PATH", "").strip() or default_sqlite_path |
- Collapse sqlite_path assignment to single line - Add comment explaining path calculation matches server.py
QueryPlanner
left a comment
There was a problem hiding this comment.
Response to Review Comments
Comment 1: Path Calculation (Not Applied)
The suggestion to use parent.parent.parent to reach project root is incorrect.
This implementation deliberately matches server.py:128:
AGENT_DIR = os.getenv("AGENT_DIR", str(Path(__file__).resolve().parent.parent))Evidence from user's logs confirms .adk lives in src/, not project root:
SQLite connection opened: /app/src/.adk/tools.db (WAL mode)
The AGENT_DIR points to src/ because that's where ADK expects agent modules to live, and the .adk directory is created there.
I've added a clarifying comment to make this explicit.
Comment 2: Simplify sqlite_path Logic (Applied)
Valid suggestion. Simplified to single-line assignment.
What
Fixes tool registry to use the default SQLite path when
SQLITE_PATHenv var is not set.Why
After migrating from Postgres to SQLite, the storage layer correctly uses a default path (
{AGENT_DIR}/.adk/tools.db), but the tool registry was returningNoneforsqlite_pathwhen the env var wasn't set. This caused reminder/calorie/workout tools to never be registered with the agent.The logs showed:
SQLITE_PATH: Noneschedule_reminder,log_meal, etc. were missing fromAvailable ToolsHow
build_tool_config_from_env()inregistry.pyto compute and use the same default SQLite path asserver.pyTests
ruff formatpassesruff checkpassesmypytypecheck passes