Conversation
…t is not a security error
| logger.exception("SQL Parsing Error: %s", e) | ||
| # If we can't parse it, we don't run it. | ||
| return False | ||
| raise e |
There was a problem hiding this comment.
The caller of this method actually raises a PermissionError:
if not self._is_safe_readonly_query(query):
raise PermissionError("Security Violation: Query contains prohibited WRITE operations.")If the error is raised here, then the other one is not required - the caller doesn't need the if statement anymore.
There was a problem hiding this comment.
Not exactly, those are 2 different error cases.
The caller will raise the PermissionError when this function returns False because it has detected an unsafe statement in the query, for example if it has a DELETE statement.
The function will raise an exception if it encounters an error during the check for unsafe statements, for example if the SQL query has a syntax issue (this is how I found this problem).
It is important that the agent receives two different messages for those two cases so they can fix their SQL statements appropriately.
2 participants
Summary
Raising error on SQL internal method instead of returning
Falseto provide better error messages to the agents. Also:.env.exampleto have the Gemini 3.1 models names because Gemini 3 has been deprecated.Clickup Ticket(s): NA
Type of Change
Changes Made
Falseto provide better error messages to the agents.env.exampleto have the Gemini 3.1 models names because Gemini 3 has been deprecated.Testing
uv run pytest tests/)uv run mypy <src_dir>)uv run ruff check src_dir/)Manual testing details:
Screenshots/Recordings
Related Issues
Deployment Notes
Checklist