fix(ai): SQL base EXPLAIN-exhaustion isValid + real-bool crash by preethisighavi · Pull Request #1696 · rocketride-org/rocketride-server

preethisighavi · 2026-07-28T00:21:39Z

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

Why this matters

Without this fix, a query the database already rejected could still get executed downstream (masked as a generic error), and any LLM response using a real JSON boolean for 'isValid' (instead of the string "true") would crash the request entirely.

Summary

'_buildSQLQuery': after every EXPLAIN attempt is rejected, set 'isValid=False' and carry the EXPLAIN error, instead of returning the last LLM response unchanged (which still had isValid: true)
Replaced all three 'result.get('isValid', '').lower() == 'true'' call sites ('get_sql', '_buildSQLQuery', 'writeQuestions') with 'ai.common.utils.parse_bool(...)', so a real JSON boolean from the LLM no longer raises AttributeError
Added two regression tests in 'test_db_base.py' covering both cases

Type

Bug fix

Testing

Added 'test_build_sql_query_marks_invalid_after_explain_exhaustion' and 'test_build_sql_query_accepts_real_json_bool_isvalid' to 'packages/ai/tests/ai/common/database/test_db_base.py'
Ran './builder ai:test' locally — all tests pass

Summary by CodeRabbit

Bug Fixes
- Improved interpretation of the LLM isValid field when validating SQL, correctly handling boolean values across formats.
- When SQL EXPLAIN validation exhausts all attempts, the query is now force-marked invalid and returns the final validation error message (not the rejected SQL).
Tests
- Added regression coverage for EXPLAIN/validation exhaustion behavior and for handling native JSON boolean isValid values.

Mirrors the fix already applied to the graph base (rocketride-org#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 rocketride-org#1601

github-actions · 2026-07-28T00:22:06Z

🤖 Internal: Discord sync marker

Auto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete.

coderabbitai · 2026-07-28T00:22:41Z

📝 Walkthrough

Walkthrough

SQL validity parsing now accepts real boolean values across database execution paths. When EXPLAIN validation fails on every attempt, the final result is explicitly marked invalid and includes the validation error. Regression tests cover both behaviors.

Changes

SQL validation result handling

Layer / File(s)	Summary
Normalize SQL validity parsing `packages/ai/src/ai/common/database/db_instance_base.py`, `packages/ai/tests/ai/common/database/test_db_base.py`	Uses `parse_bool` for `isValid` handling in SQL retrieval, validation retries, and question writing, with regression coverage for real JSON booleans.
Reject queries after EXPLAIN exhaustion `packages/ai/src/ai/common/database/db_instance_base.py`, `packages/ai/tests/ai/common/database/test_db_base.py`	Marks exhausted validation results invalid, stores the final EXPLAIN error, and tests error propagation through `get_sql()`.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: jmaionchi, stepmikhaylov, rod-christensen

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and accurately describes the main SQL validation fixes and bool parsing crash.
Linked Issues check	✅ Passed	The changes implement the linked SQL base fixes: EXPLAIN exhaustion now returns invalid with error, and isValid uses parse_bool.
Out of Scope Changes check	✅ Passed	The code and test changes stay within the linked SQL validation and error-propagation work.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/ai/src/ai/common/database/db_instance_base.py`:
- Around line 374-380: Update get_sql() in
packages/ai/src/ai/common/database/db_instance_base.py (lines 374-380) to
preserve and return result['error'] for invalid SQL validation results, and
update writeQuestions() to report that error instead of the rejected SQL text.
Add coverage in packages/ai/tests/ai/common/database/test_db_base.py (lines
269-272) by calling get_sql() and asserting valid is False with the exact
EXPLAIN error.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: dd56968f-2ccd-4617-9534-d1c19ef1f924

📥 Commits

Reviewing files that changed from the base of the PR and between 173048f and f8bec20.

📒 Files selected for processing (2)

packages/ai/src/ai/common/database/db_instance_base.py
packages/ai/tests/ai/common/database/test_db_base.py

Addresses CodeRabbit review on PR rocketride-org#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.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/ai/tests/ai/common/database/test_db_base.py`:
- Around line 294-311: Add regression coverage for writeQuestions() that
configures an EXPLAIN failure and asserts the returned text/answer fields
contain the EXPLAIN error rather than the rejected SQL. Keep the existing
get_sql() test unchanged and exercise the separate fallback path in
db_instance_base.py.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c3469ddf-bfe0-4c90-ba29-dbd633a8816e

📥 Commits

Reviewing files that changed from the base of the PR and between f8bec20 and 9878bdb.

📒 Files selected for processing (2)

packages/ai/src/ai/common/database/db_instance_base.py
packages/ai/tests/ai/common/database/test_db_base.py

coderabbitai · 2026-07-28T01:18:19Z

+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}
+


📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add regression coverage for writeQuestions() error propagation.

The new test covers get_sql() only. The separate fallback changed in db_instance_base.py Lines [582-588] could regress to emitting rejected SQL while this test still passes. Add a test asserting that writeQuestions() surfaces the EXPLAIN error through its text/answer lanes.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai/tests/ai/common/database/test_db_base.py` around lines 294 - 311, Add regression coverage for writeQuestions() that configures an EXPLAIN failure and asserts the returned text/answer fields contain the EXPLAIN error rather than the rejected SQL. Keep the existing get_sql() test unchanged and exercise the separate fallback path in db_instance_base.py.

preethisighavi requested review from Rod-Christensen, jmaionchi and stepmikhaylov as code owners July 28, 2026 00:21

github-actions Bot added the module:ai AI/ML modules label Jul 28, 2026

coderabbitai Bot reviewed Jul 28, 2026

View reviewed changes

Comment thread packages/ai/src/ai/common/database/db_instance_base.py

coderabbitai Bot reviewed Jul 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(ai): SQL base EXPLAIN-exhaustion isValid + real-bool crash - #1696

fix(ai): SQL base EXPLAIN-exhaustion isValid + real-bool crash#1696
preethisighavi wants to merge 2 commits into
rocketride-org:developfrom
preethisighavi:fix/sql-base-isvalid-1601

preethisighavi commented Jul 28, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

github-actions Bot commented Jul 28, 2026

Uh oh!

coderabbitai Bot commented Jul 28, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

preethisighavi commented Jul 28, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why this matters

Summary

Type

Testing

Summary by CodeRabbit

Uh oh!

github-actions Bot commented Jul 28, 2026

Uh oh!

coderabbitai Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

preethisighavi commented Jul 28, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 28, 2026 •

edited

Loading