From f8bec205c66624dfd74815b29db2b17b8c175381 Mon Sep 17 00:00:00 2001 From: preethisighavi Date: Sun, 26 Jul 2026 10:11:59 -0700 Subject: [PATCH 1/3] fix(ai): SQL base EXPLAIN-exhaustion isValid + real-bool crash Mirrors the fix already applied to the graph base (#1584): after every EXPLAIN attempt fails, isValid is now forced to False with the error carried, instead of silently returning the LLM's last isValid: true. isValid parsing now goes through ai.common.utils.parse_bool instead of .lower() == 'true', which crashed on a real JSON boolean. Fixes #1601 --- .../ai/common/database/db_instance_base.py | 17 +++-- .../tests/ai/common/database/test_db_base.py | 73 +++++++++++++++++++ 2 files changed, 84 insertions(+), 6 deletions(-) diff --git a/packages/ai/src/ai/common/database/db_instance_base.py b/packages/ai/src/ai/common/database/db_instance_base.py index e0a7ebd0f..7c2c3e8c4 100644 --- a/packages/ai/src/ai/common/database/db_instance_base.py +++ b/packages/ai/src/ai/common/database/db_instance_base.py @@ -47,6 +47,7 @@ from ai.common.schema import Answer, Question, QuestionType from ai.common.table import Table +from ai.common.utils import parse_bool from rocketlib.types import IInvokeLLM from .db_global_base import DEFAULT_MAX_EXECUTE_ROWS, DatabaseGlobalBase @@ -237,7 +238,7 @@ def get_sql(self, args): limit = args.get('limit', 250) result = self._buildSQLQuery(question, limit=limit) - is_valid = result.get('isValid', '').lower() == 'true' + is_valid = parse_bool(result.get('isValid')) sql_query = result.get('query', '') if is_valid and sql_query and is_sql_safe(sql_query): @@ -349,7 +350,7 @@ def _buildSQLQuery(self, question_text: str, *, limit: int = 250) -> dict: for attempt in range(self.IGlobal.max_validation_attempts): result = self._buildSQLQueryOnce(question_text, limit=limit, previous_sql=previous_sql, error=last_error) - is_valid = result.get('isValid', '').lower() == 'true' + is_valid = parse_bool(result.get('isValid')) sql_query = result.get('query', '') # If the LLM decided the question isn't a DB query, or the safety @@ -370,9 +371,13 @@ def _buildSQLQuery(self, question_text: str, *, limit: int = 250) -> dict: previous_sql = sql_query last_error = explain_error - warning( - f'SQL validation failed after {self.IGlobal.max_validation_attempts} attempt(s); returning last result.' - ) + warning(f'SQL validation failed after {self.IGlobal.max_validation_attempts} attempt(s); rejecting the query.') + # Every EXPLAIN attempt failed. Force isValid False so callers (get_sql, + # get_data, writeQuestions) don't execute a query the database already + # refused; keep the last query for context and carry the EXPLAIN error + # so callers can tell this apart from a non-SQL question. + result['isValid'] = False + result['error'] = last_error or 'SQL validation failed' return result def _buildSQLQueryOnce( @@ -564,7 +569,7 @@ def writeQuestions(self, question: Question) -> None: try: # Ask the LLM to translate the natural-language question into SQL. query_json = self._buildSQLQuery(question_text) - is_valid_query = query_json.get('isValid', '').lower() == 'true' + is_valid_query = parse_bool(query_json.get('isValid')) sql_query = query_json.get('query') # Execute the query only when the LLM flagged it as valid SQL and diff --git a/packages/ai/tests/ai/common/database/test_db_base.py b/packages/ai/tests/ai/common/database/test_db_base.py index a219d36b8..8b0da589a 100644 --- a/packages/ai/tests/ai/common/database/test_db_base.py +++ b/packages/ai/tests/ai/common/database/test_db_base.py @@ -35,6 +35,7 @@ from ai.common.database.db_global_base import DatabaseGlobalBase from ai.common.database.db_instance_base import DatabaseInstanceBase +from ai.common.utils import parse_bool # --------------------------------------------------------------------------- @@ -218,6 +219,78 @@ def test_sanitize_row_scalar_input(): assert DatabaseInstanceBase._sanitize_row(42) == 42 +# --------------------------------------------------------------------------- +# _buildSQLQuery (EXPLAIN exhaustion + isValid parsing) — regression for #1601 +# --------------------------------------------------------------------------- + + +class _TestableInstance(DatabaseInstanceBase): + """Concrete DatabaseInstanceBase that satisfies the two abstract methods.""" + + def _db_display_name(self): + """Human-readable name used in tool descriptions.""" + return 'TestDB' + + def _db_dialect(self): + """Machine-readable dialect identifier.""" + return 'testdb' + + +class _FakeGlobal: + """Stub IGlobal: every EXPLAIN attempt rejects the query.""" + + def __init__(self, max_attempts=2, explain_error='syntax error near FROM'): + self.max_validation_attempts = max_attempts + self._explain_error = explain_error + + def _validateQuery(self, sql): + return False, self._explain_error + + +def _sql_instance(fake_global): + inst = _TestableInstance.__new__(_TestableInstance) + inst.IGlobal = fake_global + return inst + + +def test_build_sql_query_marks_invalid_after_explain_exhaustion(): + """Every EXPLAIN attempt failing must flip isValid to False, not leave the LLM's 'true'. + + Before the fix, exhaustion returned the last LLM response unchanged, so + get_sql/get_data/writeQuestions executed a statement the database had + already refused on every attempt. + """ + inst = _sql_instance(_FakeGlobal()) + inst._buildSQLQueryOnce = lambda question_text, *, limit, previous_sql, error: { + 'isValid': 'true', + 'query': 'SELECT * FROM users', + } + + result = inst._buildSQLQuery('all users') + + assert parse_bool(result.get('isValid')) is False + assert result.get('error') # the EXPLAIN error is carried for the caller + + +def test_build_sql_query_accepts_real_json_bool_isvalid(): + """A real JSON bool from the LLM must not crash isValid parsing. + + Before the fix, `result.get('isValid', '').lower()` raised AttributeError + the moment the LLM returned a real bool instead of the string 'true'. + """ + fake_global = _FakeGlobal() + fake_global._validateQuery = lambda sql: (True, None) + inst = _sql_instance(fake_global) + inst._buildSQLQueryOnce = lambda question_text, *, limit, previous_sql, error: { + 'isValid': True, # real bool, not the string 'true' + 'query': 'SELECT 1', + } + + result = inst._buildSQLQuery('anything') # must not raise AttributeError + + assert parse_bool(result.get('isValid')) is True + + # --------------------------------------------------------------------------- # _tableExists (engine-backed) # --------------------------------------------------------------------------- From 9878bdb18bbd54069f5f7fdb74c4fb9d02f25293 Mon Sep 17 00:00:00 2001 From: preethisighavi Date: Mon, 27 Jul 2026 18:13:58 -0700 Subject: [PATCH 2/3] fix(ai): preserve EXPLAIN error in get_sql() and writeQuestions() Addresses CodeRabbit review on PR #1696: after the earlier fix, _buildSQLQuery() correctly carries result['error'] on EXPLAIN exhaustion, but get_sql() discarded it in its invalid-result branch and writeQuestions() showed the rejected SQL as plain text instead. Both now surface the real EXPLAIN error, so callers can tell an EXPLAIN rejection apart from a genuinely off-topic question. Adds test_get_sql_surfaces_explain_error_not_rejected_sql. --- .../ai/common/database/db_instance_base.py | 10 +++++++++- .../tests/ai/common/database/test_db_base.py | 19 +++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/packages/ai/src/ai/common/database/db_instance_base.py b/packages/ai/src/ai/common/database/db_instance_base.py index 7c2c3e8c4..e77c8d1e3 100644 --- a/packages/ai/src/ai/common/database/db_instance_base.py +++ b/packages/ai/src/ai/common/database/db_instance_base.py @@ -245,6 +245,11 @@ def get_sql(self, args): return {'sql': sql_query, 'valid': True} elif is_valid and sql_query: return {'error': 'Generated query contains unsafe SQL', 'sql': sql_query, 'valid': False} + elif result.get('error'): + # EXPLAIN rejected the query on every attempt -- surface the real + # database error instead of the rejected SQL, so callers can tell + # this apart from a genuinely off-topic question. + return {'error': result['error'], 'valid': False} else: return {'answer': sql_query, 'valid': False} @@ -574,10 +579,13 @@ def writeQuestions(self, question: Question) -> None: # Execute the query only when the LLM flagged it as valid SQL and # the safety check passes; otherwise return the LLM's text response. + # When EXPLAIN rejected the query on every attempt, prefer the real + # database error over the rejected SQL text, which reads as prose + # and would be indistinguishable from a genuine off-topic answer. if is_valid_query and sql_query and is_sql_safe(sql_query): result = self._executeSQLQuery(sql_query) else: - result = sql_query + result = query_json.get('error') or sql_query if 'text' in lanes: self.instance.writeText(str(result)) diff --git a/packages/ai/tests/ai/common/database/test_db_base.py b/packages/ai/tests/ai/common/database/test_db_base.py index 8b0da589a..710503043 100644 --- a/packages/ai/tests/ai/common/database/test_db_base.py +++ b/packages/ai/tests/ai/common/database/test_db_base.py @@ -291,6 +291,25 @@ def test_build_sql_query_accepts_real_json_bool_isvalid(): assert parse_bool(result.get('isValid')) is True +def test_get_sql_surfaces_explain_error_not_rejected_sql(): + """get_sql() must return the EXPLAIN error, not the rejected SQL as prose. + + Before this fix, the invalid branch always returned {'answer': sql_query, + 'valid': False}, so a caller could not tell an EXPLAIN rejection (rejected + SQL text) apart from a genuinely off-topic question (LLM prose answer). + """ + fake_global = _FakeGlobal(explain_error='column "userz" does not exist') + inst = _sql_instance(fake_global) + inst._buildSQLQueryOnce = lambda question_text, *, limit, previous_sql, error: { + 'isValid': 'true', + 'query': 'SELECT * FROM userz', + } + + result = inst.get_sql({'question': 'all users'}) + + assert result == {'error': 'column "userz" does not exist', 'valid': False} + + # --------------------------------------------------------------------------- # _tableExists (engine-backed) # --------------------------------------------------------------------------- From 4cb4e91fa57580732e396fb9e58b721f4335de5f Mon Sep 17 00:00:00 2001 From: preethisighavi Date: Mon, 3 Aug 2026 18:31:53 -0700 Subject: [PATCH 3/3] test(ai): add regression coverage for writeQuestions() error propagation Addresses CodeRabbit follow-up on PR #1696: the new get_sql() test didn't cover writeQuestions(), which has its own separate fallback path in db_instance_base.py that could regress independently. --- .../tests/ai/common/database/test_db_base.py | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/packages/ai/tests/ai/common/database/test_db_base.py b/packages/ai/tests/ai/common/database/test_db_base.py index 710503043..d5a6ba499 100644 --- a/packages/ai/tests/ai/common/database/test_db_base.py +++ b/packages/ai/tests/ai/common/database/test_db_base.py @@ -35,6 +35,7 @@ from ai.common.database.db_global_base import DatabaseGlobalBase from ai.common.database.db_instance_base import DatabaseInstanceBase +from ai.common.schema import Question from ai.common.utils import parse_bool @@ -310,6 +311,52 @@ def test_get_sql_surfaces_explain_error_not_rejected_sql(): assert result == {'error': 'column "userz" does not exist', 'valid': False} +class _FakeInstance: + """Minimal stand-in for IFilterInstance: records what each lane receives.""" + + def __init__(self, lanes): + self._lanes = lanes + self.text_written = None + self.answer_written = None + + def getListeners(self): + return list(self._lanes) + + def writeText(self, text): + self.text_written = text + + def writeTable(self, markdown): + pass + + def writeAnswers(self, answer): + self.answer_written = answer + + +def test_write_questions_surfaces_explain_error_not_rejected_sql(): + """writeQuestions() must report the EXPLAIN error on text/answer lanes, not the rejected SQL. + + Mirrors the get_sql() fix: writeQuestions() has its own separate fallback + path (db_instance_base.py) that must not regress to emitting the rejected + SQL as if it were the LLM's prose answer. + """ + fake_global = _FakeGlobal(explain_error='syntax error near FROM') + inst = _sql_instance(fake_global) + inst._buildSQLQueryOnce = lambda question_text, *, limit, previous_sql, error: { + 'isValid': 'true', + 'query': 'SELECT * FROM', + } + fake_instance = _FakeInstance(lanes=['text', 'answers']) + inst.instance = fake_instance + + question = Question() + question.addQuestion('all users') + + inst.writeQuestions(question) + + assert fake_instance.text_written == 'syntax error near FROM' + assert fake_instance.answer_written.getText() == 'syntax error near FROM' + + # --------------------------------------------------------------------------- # _tableExists (engine-backed) # ---------------------------------------------------------------------------