Add SQL database toolset for read-only database queries - #1568
Conversation
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📂 Previous Runs📜 Run @ f194c3b (#22300749260)✅ Results of HolmesGPT evalsAutomatically triggered by commit f194c3b on branch Results of HolmesGPT evals
📜 Run @ 0be49cd (#22299333162)✅ Results of HolmesGPT evalsAutomatically triggered by commit 0be49cd on branch Results of HolmesGPT evals
📜 Run @ 4ebf33f (#22068101706)✅ Results of HolmesGPT evalsAutomatically triggered by commit 4ebf33f on branch Results of HolmesGPT evals
📜 Run @ c9ab24e (#22039539098)✅ Results of HolmesGPT evalsAutomatically triggered by commit c9ab24e on branch Results of HolmesGPT evals
📜 Run @ 622a5b8 (#22039340532)✅ Results of HolmesGPT evalsAutomatically triggered by commit 622a5b8 on branch Results of HolmesGPT evals
✅ Results of HolmesGPT evalsAutomatically triggered by commit 1ad88b6 on branch Results of HolmesGPT evals
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" Option 3: Add PR labels to include extra evals in automatic regression runs:
Examples: 🏷️ Valid tags
Commands: CLI: |
|
✅ Docker images ready for
Use these tags to pull the images for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:089e5713
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:089e5713 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:089e5713
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:089e5713
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:089e5713
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:089e5713 me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:089e5713
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:089e5713Patch Helm values in one line (choose the chart you use): HolmesGPT chart: helm upgrade --install holmesgpt ./helm/holmes \
--set registry=me-west1-docker.pkg.dev/robusta-development/development \
--set image=holmes-dev:089e5713 \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:089e5713Robusta wrapper chart: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set holmes.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.image=holmes-dev:089e5713 \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:089e5713 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new read-only DatabaseToolset (query, list tables, describe table) with URL normalization and health checks; includes unit and integration tests. Also applies many non-functional formatting/import cleanups across docs, tests, and existing toolsets. Changes
Sequence DiagramsequenceDiagram
participant Client
participant DatabaseToolset
participant Engine as SQLAlchemy_Engine
participant Database
Client->>DatabaseToolset: Request DatabaseQuery(sql, params)
DatabaseToolset->>DatabaseToolset: Validate read-only SQL
DatabaseToolset->>Engine: Build engine from normalized URL
DatabaseToolset->>Engine: Execute query (LIMIT ≤200)
Engine->>Database: Execute SELECT
Database-->>Engine: Return rows & metadata
Engine-->>DatabaseToolset: Rows + columns
DatabaseToolset->>DatabaseToolset: Serialize values to JSON-safe types
DatabaseToolset-->>Client: {columns, rows, row_count, truncated}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔬 CLI Performance Benchmark🟡 Startup Time (no LLM)Measures
🟡 Full CLI with LLMMeasures
PR: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/utils/pydantic_utils.py (1)
144-149:⚠️ Potential issue | 🟡 MinorDocstring step numbering is out of order.
Steps are listed as 1, 3, 2, 4, 5 instead of sequential 1–5.
📝 Suggested fix
- Logic (in order): - 1) Keys are derived from the field/variable name. - 3) If available, use the first value from the field's examples array. - 2) Otherwise, use default or default_factory to generate the example value. - 4) If it's still None and the field type extends BaseModel, recursively build nested object example. - 5) Otherwise, use "your_<variable_name>" (even for lists, dicts, primitive, and any other type). + Logic (in order): + 1) Keys are derived from the field/variable name. + 2) If available, use the first value from the field's examples array. + 3) Otherwise, use default or default_factory to generate the example value. + 4) If it's still None and the field type extends BaseModel, recursively build nested object example. + 5) Otherwise, use "your_<variable_name>" (even for lists, dicts, primitive, and any other type).
🤖 Fix all issues with AI agents
In `@holmes/plugins/toolsets/database/database.py`:
- Around line 26-35: The current prefix-only regex checks (_READONLY_PATTERN and
_WRITE_PATTERN) can be bypassed by writable CTEs; update the guard in the module
(referencing _READONLY_PATTERN and _WRITE_PATTERN) to either (A) scan the entire
SQL string for write keywords (apply _WRITE_PATTERN.search over the full query
body and also check for "WITH ... AS" blocks that contain write verbs) or (B,
preferred defense-in-depth) enforce a DB-level read-only transaction when
executing queries (e.g., set the connection/transaction to read-only via the DB
API or SQLAlchemy execution_options/SET TRANSACTION READ ONLY) so even queries
with embedded write statements cannot mutate data; implement one of these fixes
where queries are executed (the function that uses these patterns) and remove
reliance on prefix-only matching.
- Around line 302-336: The error handler in DatabaseListTables' _invoke does not
include the schema (and other params) in the returned error; update the except
block to include the schema and relevant params (e.g., schema =
params.get("schema") and include_views = params.get("include_views")) and embed
them in the StructuredToolResult.error message (or an additional field)
alongside the exception string so the error explicitly states which schema and
options were used when calling _toolset.database_config.connection_url via
sqlalchemy.create_engine and inspector calls.
- Around line 128-138: Move the inline "import sqlalchemy" statements out of the
functions and to the top of the module using a module-level optional import
pattern (try: import sqlalchemy except ImportError: sqlalchemy = None). Update
the functions that currently import sqlalchemy inline (the connection-check
routine that calls _normalise_url and engine.connect(), plus the other routines
at the places flagged) to first verify sqlalchemy is available and return/raise
a clear error if it's None; keep existing behavior otherwise. Ensure you
reference the module-level sqlalchemy symbol (not local imports) in calls like
sqlalchemy.create_engine, sqlalchemy.text and engine.dialect.name.
In
`@tests/llm/fixtures/test_ask_holmes/93_events_since_specific_date/test_case.yaml`:
- Line 13: In the test case YAML entry for the fixture (the skip_reason field),
fix the typo by changing the value from "this sometimes fails to to missing mock
errors" to "this sometimes fails due to missing mock errors" so the skip_reason
reads correctly; update the skip_reason string in
test_ask_holmes/93_events_since_specific_date/test_case.yaml accordingly.
In `@tests/test_database_toolset.py`:
- Around line 151-172: The test uses deprecated tempfile.mktemp to create
db_file; replace it by creating a tempfile.NamedTemporaryFile(delete=False) and
use its .name as the SQLite path (close the NamedTemporaryFile before opening
the DB so SQLite can access it), then proceed to create the engine and run the
queries as before; ensure the finally block still unlinks the temporary file.
Update references in this test around the db_file creation and keep
DatabaseConfig(connection_url=f"sqlite:///{db_file}") and
self.toolset.execute_query usage unchanged.
- Around line 108-110: The test test_config_requires_url is catching a bare
Exception which hides unexpected failures; update it to expect
pydantic.errors.ValidationError instead of Exception by importing
ValidationError from pydantic and using pytest.raises(ValidationError) when
instantiating DatabaseConfig() so the test only passes for the specific
required-field validation failure.
🧹 Nitpick comments (8)
tests/llm/fixtures/test_ask_holmes/212_large_configmap_needle/generate_configs.py (1)
177-200: Consider adding type hints to improve maintainability.The functions
random_hex,random_version,random_connection_string,generate_service_config, andmainlack type hints. Adding them would improve code clarity and enable better static analysis. As per coding guidelines, type hints are required for Python files.📝 Example type hints
def random_hex(rng: random.Random, length: int = 6) -> str: return "".join(rng.choice("0123456789abcdef") for _ in range(length)) def random_version(rng: random.Random) -> str: return f"{rng.randint(1, 12)}.{rng.randint(0, 30)}.{rng.randint(0, 99)}" def random_connection_string(rng: random.Random, service_name: str) -> str: # ... existing implementation def generate_service_config( rng: random.Random, name: str, connection_string: str | None = None ) -> str: # ... existing implementation def main() -> None: # ... existing implementationAlso applies to: 202-308, 311-357
tests/llm/fixtures/test_ask_holmes/217_database_sql_query/test_case.yaml (1)
27-69:before_testsetup looks solid, but URL normalization is duplicated.The URL normalization logic (replacing
postgresql://→postgresql+pg8000://) is duplicated between this script and the maindatabase.pytoolset code. This is acceptable for test independence, but worth noting.One minor concern: the
before_testscript doesn't handle the case wherePOSTGRES_URLalready contains a+driversuffix (e.g.,postgresql+psycopg2://...). If a CI environment provides such a URL, the normalization would be skipped andpg8000wouldn't be available, causing a silent failure.holmes/plugins/toolsets/database/instructions.jinja2 (1)
1-9: Consider mentioning the 200-row hard cap.The
DatabaseQuerytool enforces a_MAX_ROWS = 200limit, but the instructions only say "Use LIMIT clauses" without mentioning this ceiling. If the LLM requestsLIMIT 500, it will silently get 200 rows withtruncated: true, which could confuse it. Adding a note like "Results are capped at 200 rows regardless of LIMIT" would help the LLM plan queries with appropriate aggregation.tests/test_database_toolset.py (2)
91-100: Move repeated imports to the top of the file.
datetime,Decimal,sqlalchemy,tempfile,os,_READONLY_PATTERN, and_WRITE_PATTERNare imported inline in multiple test methods. The coding guidelines require imports at the top of the file. The_READONLY_PATTERN/_WRITE_PATTERNimports in particular are duplicated four times.As per coding guidelines: "ALWAYS place Python imports at the top of the file, not inside functions or methods."
Proposed fix (top-of-file imports)
import pytest +import datetime +import os +import tempfile +from decimal import Decimal + +import sqlalchemy from holmes.plugins.toolsets.database.database import ( DatabaseConfig, DatabaseToolset, + _READONLY_PATTERN, + _WRITE_PATTERN, _normalise_url, _serialize_value, )Also applies to: 139-139, 151-152, 205-208, 214-217, 223-226, 232-235
253-275: Accessing tools by index is fragile.
toolset.tools[0],toolset.tools[1],toolset.tools[2]will break silently if the tool registration order changes. Consider looking up tools by name instead.Suggested approach
def test_query_one_liner(self): toolset = DatabaseToolset() - query_tool = toolset.tools[0] # DatabaseQuery + query_tool = next(t for t in toolset.tools if t.name == "database_query") result = query_tool.get_parameterized_one_liner( {"sql": "SELECT * FROM users WHERE active = true LIMIT 10"} )holmes/plugins/toolsets/database/database.py (3)
165-187: Engine created and disposed on every query invocation.
sqlalchemy.create_engine()is designed to be a long-lived, pooled resource. Creating and disposing it on every call toexecute_query(and similarly inDatabaseListTables._invokeandDatabaseDescribeTable._invoke) defeats connection pooling and adds unnecessary overhead per query.Consider creating the engine once (e.g., in
_perform_health_checkor lazily on first use) and reusing it across invocations, disposing only on toolset teardown.Sketch: lazy engine with reuse
class DatabaseToolset(Toolset): + _engine: Optional[Any] = None + + def _get_engine(self): + import sqlalchemy + if self._engine is None: + url = _normalise_url(self.database_config.connection_url) + self._engine = sqlalchemy.create_engine(url, pool_pre_ping=True) + return self._engine + def execute_query(self, sql: str, limit: Optional[int] = None) -> Dict[str, Any]: import sqlalchemy # ... validation ... - url = _normalise_url(self.database_config.connection_url) - engine = sqlalchemy.create_engine(url) - try: - with engine.connect() as conn: - # ... - finally: - engine.dispose() + engine = self._get_engine() + with engine.connect() as conn: + # ...
53-66: URL normalization edge case: schemes with+that partially match a prefix.The iteration
scheme.startswith(prefix + "+")means scheme"mssql+pyodbc"matches prefix"mssql"and gets rewritten to"mssql+pymssql", which may not be desired if a user intentionally chosepyodbc. This is generally the intended behavior per the PR description (preferring pure-Python drivers), but it silently overrides an explicit user choice. Consider logging a warning when overriding a user-specified driver.Suggested: log when overriding an explicit driver
for prefix, replacement in _DRIVER_MAP.items(): if scheme == prefix or scheme.startswith(prefix + "+"): - # Only replace if the user hasn't already picked the right driver if scheme != replacement: + logger.info( + "Rewriting database URL scheme from %s to %s for pure-Python driver compatibility", + scheme, replacement, + ) return raw_url.replace(scheme, replacement, 1) return raw_url
190-201:_serialize_value:dictandlistvalues may contain non-JSON-safe nested values.If a database returns a JSON/JSONB column, it could contain nested
datetime,Decimal, orbytesvalues inside dicts/lists. The current passthrough on Lines 198-199 skips recursive serialization. Consider whether nested serialization is needed, or if this is acceptable for the expected data shapes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@holmes/plugins/toolsets/database/database.py`:
- Around line 327-359: The DatabaseListTables._invoke and
DatabaseDescribeTable._invoke methods call sqlalchemy.create_engine /
sqlalchemy.inspect directly without checking whether the optional sqlalchemy
import succeeded; add the same guard used in execute_query (check if sqlalchemy
is None) at the start of each _invoke and raise a clear RuntimeError like the
one in execute_query explaining that SQLAlchemy is required (include context
such as the tool name or params), so users get a descriptive error instead of an
AttributeError.
🧹 Nitpick comments (6)
holmes/plugins/toolsets/database/database.py (4)
42-47:_WRITE_ANYWHERE_PATTERNwill false-positive on SQL keywords inside string literals.A legitimate query like
SELECT * FROM events WHERE action = 'DELETE'will be rejected because\bDELETE\bmatches inside the string literal. This also applies toUPDATE,DROP, etc. in column values.This is hard to solve perfectly without a SQL parser, but worth documenting as a known limitation or considering a simple heuristic (e.g., strip quoted strings before matching).
54-62:"mysql+mysqldb"entry in_DRIVER_MAPis unreachable dead code.The
"mysql"entry matches first due toscheme.startswith("mysql+"), so"mysql+mysqldb"is never reached in the loop. The testtest_mysql_mysqldb_rewrittenpasses, but via the"mysql"key. The entry is harmless but misleading — consider removing it or adding a comment.
185-212: Consider reusing the SQLAlchemy engine instead of creating one per query.
create_engineis called on everyexecute_query,DatabaseListTables._invoke, andDatabaseDescribeTable._invokecall. SQLAlchemy engines are designed to be long-lived and manage their own connection pools. Creating and disposing an engine per call adds overhead (DNS resolution, connection setup, SSL handshake).Consider creating the engine once (e.g., lazily on first use) and storing it on the toolset, then disposing in a teardown/cleanup method.
215-226:_serialize_valuedoesn't recursively serialize nested structures.
dictandlistvalues are passed through as-is (Line 223-224), but they may contain non-JSON-serializable nested values (e.g.,Decimal,datetime). This is a minor edge case since most DB drivers return simple types in JSON/JSONB columns, but PostgreSQL JSONB columns can containDecimalvalues in some drivers.tests/test_database_toolset.py (2)
253-275: Tool access by index is fragile.
toolset.tools[0],tools[1],tools[2]will silently test the wrong tool if the order changes inDatabaseToolset.__init__. Consider looking up by name:Proposed approach
- query_tool = toolset.tools[0] # DatabaseQuery + query_tool = next(t for t in toolset.tools if t.name == "database_query")Same for the other two tests.
99-108: Minor:datetimeandDecimalimports are inside test methods.Per coding guidelines, Python imports should be at the top of the file. These could be moved to the top alongside the other imports, though this is a common test pattern.
As per coding guidelines: "ALWAYS place Python imports at the top of the file, not inside functions or methods."
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_database_toolset.py`:
- Around line 100-109: Tests test_datetime_to_str and test_decimal_to_str
currently import datetime and Decimal inside the test bodies; move "from
datetime import datetime" and "from decimal import Decimal" to the module-level
imports at the top of the file and remove the inline imports from the functions
so the tests (and references to _serialize_value) use the top-level imports
instead.
🧹 Nitpick comments (3)
tests/test_database_toolset.py (3)
11-11: Remove unnecessarynoqadirective.The
# noqa: E402comment is flagged as unused by Ruff because the E402 rule (module level import not at top of file) is not enabled in your configuration. While this pattern is correct for imports afterimportorskip, the directive can be safely removed.♻️ Proposed fix
-from holmes.plugins.toolsets.database.database import ( # noqa: E402 +from holmes.plugins.toolsets.database.database import ( DatabaseConfig, DatabaseToolset,
139-252: Add test coverage for database error handling with detailed messages.The tests comprehensively verify write-operation blocking, but there's no test for how the toolset handles database errors from valid read queries (e.g., syntax errors, missing tables, invalid column names). Based on learnings, all toolsets must return detailed error messages from underlying APIs, including the exact query executed and full error response to enable LLM self-correction.
Consider adding a test case like:
def test_database_error_returns_detailed_message(self): """Verify that database errors include the query and full error details.""" with pytest.raises(Exception) as exc_info: self.toolset.execute_query("SELECT nonexistent_column FROM nonexistent_table") error_msg = str(exc_info.value) # Verify error includes the actual query assert "SELECT nonexistent_column FROM nonexistent_table" in error_msg # Verify error includes database-specific details assert any(keyword in error_msg.lower() for keyword in ["table", "column", "not found", "does not exist"])Based on learnings: All toolsets must return detailed error messages from underlying APIs to enable LLM self-correction, including exact query/command executed and full API error response.
254-276: Consider accessing tools by name instead of index.The tests access tools by numeric index (
tools[0],tools[1],tools[2]), which is brittle if tool order changes. Consider finding tools by name for more robust tests.♻️ Suggested improvement
def test_query_one_liner(self): toolset = DatabaseToolset() - query_tool = toolset.tools[0] # DatabaseQuery + query_tool = next(t for t in toolset.tools if t.name == "database_query") result = query_tool.get_parameterized_one_liner( {"sql": "SELECT * FROM users WHERE active = true LIMIT 10"} ) assert "Database" in result assert "SELECT" in result def test_list_tables_one_liner(self): toolset = DatabaseToolset() - list_tool = toolset.tools[1] # DatabaseListTables + list_tool = next(t for t in toolset.tools if t.name == "database_list_tables") result = list_tool.get_parameterized_one_liner({"schema": "public"}) assert "Database" in result assert "public" in result def test_describe_one_liner(self): toolset = DatabaseToolset() - desc_tool = toolset.tools[2] # DatabaseDescribeTable + desc_tool = next(t for t in toolset.tools if t.name == "database_describe_table") result = desc_tool.get_parameterized_one_liner({"table_name": "orders"}) assert "Database" in result assert "orders" in result
d42e785 to
be52c55
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@holmes/plugins/toolsets/database/database.py`:
- Around line 42-47: The current _WRITE_ANYWHERE_PATTERN can match write
keywords inside SQL string literals (e.g., "WHERE action = 'DELETE'"); update
the code by (1) adding a concise comment above _WRITE_ANYWHERE_PATTERN
documenting this known limitation and advising that a real SQL parser is the
robust fix, and (2) implement a lightweight preprocessing step (e.g., a helper
like strip_sql_string_literals or sanitize_query_literals) and use it before
applying _WRITE_ANYWHERE_PATTERN so quoted literals are removed/replaced first
to avoid false positives; reference the _WRITE_ANYWHERE_PATTERN symbol and any
query-checking function that uses it so you replace the raw regex check with the
sanitized-query check.
🧹 Nitpick comments (6)
tests/llm/fixtures/test_ask_holmes/217_database_sql_query/test_case.yaml (1)
11-15: User prompt directly names the target table — consider testing discovery more.The prompt explicitly tells the LLM to "look at the
holmes_eval_217table," which bypasses schema discovery. Per coding guidelines, eval tests should "test discovery, not recognition." Consider a prompt like: "Find the verification code stored in the database" and assert that Holmes discovers the table vialist_tables+describe_tableon its own. That said, this is acceptable for an initial test since the primary goal is validating the database toolset end-to-end.tests/test_database_toolset.py (2)
11-11: Remove unusednoqadirective.Ruff flags
# noqa: E402as unnecessary here. Sincepytest.importorskipon line 9 is not itself a regular import statement, the E402 rule doesn't apply to the imports on line 11.-from holmes.plugins.toolsets.database.database import ( # noqa: E402 +from holmes.plugins.toolsets.database.database import (
254-276: Tool access by positional index is fragile.
toolset.tools[0],[1],[2]will silently break if tool order changes inDatabaseToolset.__init__. Consider looking up tools by name instead.Suggested approach
- def test_query_one_liner(self): - toolset = DatabaseToolset() - query_tool = toolset.tools[0] # DatabaseQuery + def test_query_one_liner(self): + toolset = DatabaseToolset() + query_tool = next(t for t in toolset.tools if t.name == "database_query")Apply the same pattern for the other two tests.
holmes/plugins/toolsets/database/database.py (3)
65-78: Dead entry in_DRIVER_MAP:"mysql+mysqldb"is never reached.When
_normalise_urlreceives"mysql+mysqldb://...", the"mysql"entry matches first viascheme.startswith("mysql+"), producing the correct result. The explicit"mysql+mysqldb"entry at line 58 is therefore unreachable. This isn't a bug (behavior is correct), but it's misleading — the entry suggests it's doing something when it isn't.Consider either removing the dead entry or reordering to check more-specific prefixes first (longer keys before shorter ones).
185-212: A new SQLAlchemy engine is created on everyexecute_querycall.Each invocation of
execute_querycallscreate_engine()anddispose(), which means a full connection setup/teardown per query with no connection pooling benefit. For a toolset that an LLM might call multiple times in a single investigation, this adds unnecessary latency.Consider creating and caching the engine once (e.g., during health check or on first use) and reusing it across calls, disposing only on toolset teardown.
191-194: Consider logging the suppressed exception for debuggability.The bare
except Exception: passwhen settingREAD ONLYtransaction mode silently swallows errors. A debug-level log would help troubleshoot connection issues without noise in production.Suggested fix
try: conn.execute(sqlalchemy.text("SET TRANSACTION READ ONLY")) except Exception: - pass # Not all dialects support this; regex check is primary guard + logger.debug("SET TRANSACTION READ ONLY not supported by this dialect, skipping")
Adds a new Python toolset that provides read-only SQL access via SQLAlchemy. Supports PostgreSQL, MySQL/MariaDB, SQLite, and SQL Server using pure-Python drivers (pg8000, PyMySQL, pymssql). Write operations are blocked by regex validation. Includes 3 tools: database_query, database_list_tables, and database_describe_table. - 40 unit tests covering URL normalisation, read-only validation, serialization - Eval test (217) using Supabase PostgreSQL with verification code pattern - Toolset config uses DATABASE_URL env var https://claude.ai/code/session_01H8VBEN3oLGYE7CRvSi2zvC Signed-off-by: Claude <noreply@anthropic.com>
Use a database-specific env var name to allow separate evals for different database backends in the future. https://claude.ai/code/session_01H8VBEN3oLGYE7CRvSi2zvC Signed-off-by: Claude <noreply@anthropic.com>
- Block writable CTEs (WITH ... DELETE/INSERT/UPDATE) via _WRITE_ANYWHERE_PATTERN - Add defense-in-depth with SET TRANSACTION READ ONLY at DB level - Move sqlalchemy import to module level with try/except for optional dep - Include schema parameter in DatabaseListTables error messages - Use ValidationError instead of bare Exception in test - Replace deprecated tempfile.mktemp with NamedTemporaryFile - Fix typo in test_case.yaml: "to to" → "due to" - Add 4 new tests for writable CTE detection https://claude.ai/code/session_01MDfhnnq4yosJYeU5hdFztJ Signed-off-by: Claude <noreply@anthropic.com>
Use pytest.importorskip("sqlalchemy") to gracefully skip the entire
test module in CI where sqlalchemy is an optional dependency.
https://claude.ai/code/session_01MDfhnnq4yosJYeU5hdFztJ
Signed-off-by: Claude <noreply@anthropic.com>
3b64206 to
559e761
Compare
|
/eval |
Adds pytestmark = pytest.mark.database so all 44 tests can be run with: pytest -m database https://claude.ai/code/session_01MDfhnnq4yosJYeU5hdFztJ Signed-off-by: Claude <noreply@anthropic.com>
|
@arikalon1 Your eval run has finished. ✅ Completed successfully 🧪 Manual Eval Results
Results of HolmesGPT evals
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
Commands: CLI: |
Replace database/question-answer/fast/easy tags with single db-connectors tag on eval 217 and unit tests. https://claude.ai/code/session_01MDfhnnq4yosJYeU5hdFztJ Signed-off-by: Claude <noreply@anthropic.com>
|
/eval |
|
@arikalon1 Your eval run has finished. ✅ Completed successfully 🧪 Manual Eval Results
Results of HolmesGPT evals
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
Commands: CLI: |
The secret was added to GitHub but not passed to the test runner environment, causing the database toolset eval to fail. https://claude.ai/code/session_01MDfhnnq4yosJYeU5hdFztJ Signed-off-by: Claude <noreply@anthropic.com>
|
/eval |
|
@arikalon1 Your eval run has finished. ✅ Completed successfully 🧪 Manual Eval Results
Results of HolmesGPT evals
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
Commands: CLI: |
Signed-off-by: Arik Alon <alon.arik@gmail.com>
Avi-Robusta
left a comment
Avi-Robusta
left a comment
There was a problem hiding this comment.
Hey,
some small changes
4 participants
Summary
This PR introduces a new Database/SQL toolset that enables Holmes to query SQL databases (PostgreSQL, MySQL, SQLite, SQL Server) with read-only access. The toolset provides three tools for exploring and querying databases safely.
Key Changes
New Database Toolset (
holmes/plugins/toolsets/database/)database.py: Core toolset implementation with:DatabaseToolset: Main toolset class managing database connectionsDatabaseConfig: Configuration class for connection URLsDatabaseQuery: Tool to execute read-only SQL queries (SELECT, SHOW, DESCRIBE, EXPLAIN, WITH)DatabaseListTables: Tool to list tables and views in a databaseDatabaseDescribeTable: Tool to inspect table schema (columns, types, constraints, indexes)_normalise_url(): Converts database URLs to use pure-Python drivers (pg8000, PyMySQL, pymssql) for better compatibility_serialize_value(): Converts database values to JSON-safe typesinstructions.jinja2: LLM instructions for using the database toolsTests
tests/test_database_toolset.py: Comprehensive unit tests covering:tests/llm/fixtures/test_ask_holmes/217_database_sql_query/: Integration test case:test_case.yaml: Test scenario that verifies Holmes can discover table structure and query datatoolsets.yaml: Toolset configuration for the testIntegration
holmes/plugins/toolsets/__init__.pyto register the new DatabaseToolsetholmes/plugins/toolsets/database/__init__.pymodule initializationNotable Implementation Details
Security: Write operations are blocked at two levels:
Driver Compatibility: Automatically rewrites connection URLs to use pure-Python drivers (no C extensions required), making the toolset work in containerized environments
Database Support: Works with PostgreSQL, MySQL/MariaDB, SQLite, and SQL Server via SQLAlchemy
Health Checks: Includes connection validation during prerequisite checks
Code Quality: Includes formatting improvements to several existing files (line length, import ordering)
https://claude.ai/code/session_01MDfhnnq4yosJYeU5hdFztJ
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores