From 934fc7324f64364869af5a4e9d2c3936dfe710d8 Mon Sep 17 00:00:00 2001 From: BADRI NARAYANAN Date: Sun, 6 Sep 2026 23:34:15 -0700 Subject: [PATCH] fix(chat): name the available views/functions in a rejection, so the model self-corrects Live QA after PR #26 found a real, separate bug: asked "What are the transactions available in this account?", the model queried `SELECT * FROM transactions` (a plausible-sounding guess matching the system prompt's own topic word "transactions") instead of the real view `v_transaction_history`, got validator_rejected, and gave up rather than retrying -- answering "I cannot access transaction details" even though the correct view was available and readable the whole time. Confirmed via information_schema.role_table_grants and a live query through trueup_chat_readonly against real data that this was never a permission/grant issue -- the DB layer works correctly. Confirmed from the actual chat_tool_call row for the live session that the rejection message did reach the model as a tool result, but named only what was rejected, never what was available, giving it nothing to correct toward. Fixed by having both sql_tool_validator rejection paths (off-allow-list relation, disallowed function) name the actual allow-list contents in the same tool result, plus a new system-prompt rule instructing the model to retry once with a corrected name before declining. Verified live against the restarted backend: the same question now correctly resolves to `SELECT * FROM v_transaction_history` on the first or corrected attempt. Also confirms a second finding from the same report: "what model portfolio is chosen" is not a bug -- no curated view covers model assignment, so declining is FR-54's mandated behavior, not a defect. --- .../chat/chat_orchestration_service.py | 4 ++++ .../app/services/chat/sql_tool_validator.py | 19 +++++++++++++++++-- backend/tests/unit/test_sql_tool_validator.py | 18 ++++++++++++++++++ 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/backend/app/services/chat/chat_orchestration_service.py b/backend/app/services/chat/chat_orchestration_service.py index 11a343e..c850a37 100644 --- a/backend/app/services/chat/chat_orchestration_service.py +++ b/backend/app/services/chat/chat_orchestration_service.py @@ -60,6 +60,10 @@ 7. Every view is already scoped to the customer you are answering for -- it returns their rows and no one else's. Never reference `customer_id` in a `WHERE` clause and never invent an id value; there is no id for you to supply. +8. Use only relation and function names exactly as `get_database_schema` returns them -- never a + plausible-sounding guess (a topic word like "transactions" is not a table name). If + `execute_read_only_sql` rejects a query, its error names what is actually available -- retry + once with a corrected name from that list before telling the customer you cannot answer. """ diff --git a/backend/app/services/chat/sql_tool_validator.py b/backend/app/services/chat/sql_tool_validator.py index 3cfaf2b..3fdecb4 100644 --- a/backend/app/services/chat/sql_tool_validator.py +++ b/backend/app/services/chat/sql_tool_validator.py @@ -70,8 +70,18 @@ def validate_query(sql: str) -> ValidationResult: if name in cte_names: continue if name not in CURATED_VIEW_NAMES: + # Naming the actual allow-list here, not just rejecting, is load-bearing: the model + # has been observed guessing a plausible-sounding relation name (e.g. `transactions` + # for the topic word in its own system prompt) instead of the real view name + # (`v_transaction_history`) and then giving up on a bare rejection rather than + # retrying. Putting the correction in the same tool result removes the need for it to + # re-call `get_database_schema` or recall the list from earlier in the turn. return ValidationResult( - ok=False, reason=f"relation '{table.name}' is not available to this assistant" + ok=False, + reason=( + f"relation '{table.name}' is not available to this assistant. " + f"Available views: {', '.join(sorted(CURATED_VIEW_NAMES))}." + ), ) for func in statement.find_all(exp.Func): @@ -86,8 +96,13 @@ def validate_query(sql: str) -> ValidationResult: continue name = func.name.lower() if isinstance(func, exp.Anonymous) else func.sql_name().lower() if name not in _ALLOWED_FUNCTIONS: + # Same self-correcting-message rationale as the relation check above. return ValidationResult( - ok=False, reason=f"function '{name}' is not available to this assistant" + ok=False, + reason=( + f"function '{name}' is not available to this assistant. " + f"Available functions: {', '.join(sorted(_ALLOWED_FUNCTIONS))}." + ), ) return ValidationResult( diff --git a/backend/tests/unit/test_sql_tool_validator.py b/backend/tests/unit/test_sql_tool_validator.py index a0154da..bc2366f 100644 --- a/backend/tests/unit/test_sql_tool_validator.py +++ b/backend/tests/unit/test_sql_tool_validator.py @@ -92,3 +92,21 @@ def test_normalized_sql_drops_comments() -> None: assert result.ok is True assert result.normalized_sql is not None assert "sneaky" not in result.normalized_sql + + +def test_off_allow_list_relation_reason_names_the_available_views() -> None: + """Observed live: the model guessed `transactions` instead of `v_transaction_history` and + gave up on a bare rejection. The error must name the real options in the same tool result.""" + result = validate_query("SELECT * FROM transactions") + assert result.ok is False + assert result.reason is not None + assert "v_transaction_history" in result.reason + assert "v_holdings" in result.reason + + +def test_disallowed_function_reason_names_the_available_functions() -> None: + result = validate_query("SELECT pg_sleep(5) FROM v_holdings") + assert result.ok is False + assert result.reason is not None + assert "sum" in result.reason + assert "count" in result.reason