Skip to content

Commit 8bb695e

Browse files
JuliaEdomclaude
andcommitted
fix(guards): stop TokenError escaping the SQL guard as pipeline_exception
sqlglot's failure modes are siblings under SqlglotError, not a chain: the parser raises ParseError, the lexer raises TokenError. `_safe_parse` caught only ParseError, so an unterminated string literal escaped validate_sql, crashed the pipeline node and was recorded as `pipeline_exception` — a model failure wearing a transport failure's label, on a project whose runs are judged clean by exactly that counter. Observed live: q1399 of the 2026-07-22 student-base run ("Error tokenizing '...T1.last_name = 'Mclean'", pred_sql lost, in_tok 0). Three consequences, not one: the run's health signal lied; the offending SQL was dropped from the record; and the question never reached repair_once, because a raised exception skips routing that invalid_sql would have taken. Probing the parser for other escapes found a second one: RecursionError on deeply nested parens, which is not a SqlglotError at all. Both now come back as violations — the guard's documented contract is to report, never raise. The same narrow `except ParseError` sat on three more parse sites whose own docstrings promise a fallback instead of a crash (eval/dataset.extract_gold_ tables, autotune/build_dataset.{extract_tables,sql_parses}, p3f_acceptance). Corrected there too; measured unreachable on current data (370 distinct gold SQLs, 9428 bird_train rows all parse), so S1's "parse-fail 0" is unchanged. Gate §7.3 green: ruff, format 190, mypy 108, pytest 555 passed / 14 skipped cov 91.05%, hygiene 542, no_raw_compare. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent b7eb4d6 commit 8bb695e

5 files changed

Lines changed: 63 additions & 7 deletions

File tree

‎scripts/autotune/build_dataset.py‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,10 @@ def extract_tables(sql: str) -> list[str]:
5454
"""Distinct table names in order of appearance; [] when unparseable."""
5555
try:
5656
tree = sqlglot.parse_one(sql, dialect=DIALECT)
57-
except sqlglot.errors.ParseError:
57+
except (sqlglot.errors.SqlglotError, RecursionError):
58+
# TokenError (lexer) is a sibling of ParseError, RecursionError is
59+
# neither — all are "unparseable", which this function promises to
60+
# answer with [] rather than by killing the dataset build.
5861
return []
5962
seen: list[str] = []
6063
for table in tree.find_all(sqlglot_exp.Table):
@@ -95,7 +98,9 @@ def build_example(row: Any) -> dict[str, str]:
9598
def sql_parses(sql: str) -> bool:
9699
try:
97100
sqlglot.parse_one(sql, dialect=DIALECT)
98-
except sqlglot.errors.ParseError:
101+
except (sqlglot.errors.SqlglotError, RecursionError):
102+
# A parse-check predicate must answer False, never raise: a single
103+
# token-broken train row would otherwise abort the whole build.
99104
return False
100105
return True
101106

‎scripts/p3f_acceptance.py‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@
2020
from typing import Any
2121

2222
from sqlglot import exp, parse_one
23-
from sqlglot.errors import ParseError
23+
from sqlglot.errors import SqlglotError
2424

2525
ColumnRef = tuple[str, str]
2626

@@ -200,7 +200,9 @@ def _qualified_columns(sql: str) -> tuple[set[ColumnRef], str | None]:
200200
return set(), None
201201
try:
202202
tree = parse_one(sql, read="sqlite")
203-
except ParseError as exc:
203+
except (SqlglotError, RecursionError) as exc:
204+
# SqlglotError covers the lexer's TokenError too (sibling of
205+
# ParseError); RecursionError covers pathological nesting.
204206
return set(), f"SQL parse failed: {exc}"
205207

206208
alias_to_table: dict[str, str] = {}

‎src/nl_sql/eval/dataset.py‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,11 @@ def extract_gold_tables(sql: str) -> list[str]:
137137
"""
138138
try:
139139
tree = sqlglot.parse_one(sql, read="sqlite")
140-
except sqlglot.errors.ParseError:
140+
except (sqlglot.errors.SqlglotError, RecursionError):
141+
# SqlglotError, not ParseError: the lexer raises TokenError, which is a
142+
# sibling class — and deep nesting raises RecursionError, which is
143+
# neither. All three mean "no AST", which is exactly the fallback case
144+
# this docstring promises.
141145
return _extract_via_regex(sql)
142146
if tree is None:
143147
return _extract_via_regex(sql)

‎src/nl_sql/execution/guards.py‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919

2020
import sqlglot
2121
from sqlglot import exp
22-
from sqlglot.errors import ParseError
22+
from sqlglot.errors import SqlglotError
2323

2424
from nl_sql.db.connection import Dialect
2525

@@ -123,9 +123,21 @@ def _safe_parse(
123123
# sqlglot.parse returns list[Optional[Expression]] but its internal
124124
# Expr alias trips strict mypy; cast keeps the public API typed.
125125
return cast("list[exp.Expression | None]", sqlglot.parse(sql, read=sqlglot_name))
126-
except ParseError as exc:
126+
except SqlglotError as exc:
127+
# sqlglot's failure modes are SIBLINGS, not a chain: the parser raises
128+
# ParseError, the lexer raises TokenError (unterminated string or
129+
# identifier quote). Catching only ParseError let TokenError escape
130+
# validate_sql and kill the whole pipeline run — a model writing bad
131+
# SQL then surfaced as `pipeline_exception`, i.e. indistinguishable
132+
# from a transport failure (q1399, 2026-07-22 student-base run).
127133
report.add("parse_error", str(exc))
128134
return None
135+
except RecursionError:
136+
# Deeply nested parens exhaust the interpreter stack inside sqlglot,
137+
# and RecursionError is not a SqlglotError. No AST is no AST: same
138+
# verdict as any other parse failure — reject, never propagate.
139+
report.add("parse_too_deep", "SQL nesting is too deep to parse")
140+
return None
129141

130142

131143
def _check_select_only(parsed: exp.Expression, report: ValidationReport) -> None:

‎tests/test_execution_guards.py‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,37 @@ def test_garbage_input_is_rejected() -> None:
216216
assert not report.ok
217217

218218

219+
def test_unterminated_string_literal_is_reported_not_raised() -> None:
220+
"""Lexer-level breakage must come back as a report, not as an exception.
221+
222+
sqlglot raises TokenError (a *sibling* of ParseError, not a subclass) when
223+
a string literal never closes. Real regression: q1399 of the 2026-07-22
224+
student-base run, where the escaping TokenError was recorded as
225+
`pipeline_exception` — i.e. a model failure disguised as a transport one.
226+
"""
227+
sql = "SELECT T1.member_id FROM member AS T1 WHERE T1.first_name = 'Maya AND T1.last_name = 'Mclean'"
228+
report = validate_sql(sql)
229+
assert not report.ok
230+
assert any(v.code == "parse_error" for v in report.violations)
231+
232+
233+
def test_unterminated_identifier_quote_is_reported_not_raised() -> None:
234+
report = validate_sql('SELECT "col FROM Artists')
235+
assert not report.ok
236+
assert any(v.code == "parse_error" for v in report.violations)
237+
238+
239+
def test_deeply_nested_sql_is_reported_not_raised() -> None:
240+
"""Nesting deep enough to exhaust the interpreter stack inside sqlglot.
241+
242+
RecursionError is not a SqlglotError, so it escapes the parse guard on its
243+
own path. No AST means the same verdict as any other parse failure: reject.
244+
"""
245+
report = validate_sql("SELECT " + "(" * 2000 + "1" + ")" * 2000)
246+
assert not report.ok
247+
assert any(v.code == "parse_too_deep" for v in report.violations)
248+
249+
219250
@pytest.mark.parametrize(
220251
"code",
221252
[
@@ -226,6 +257,8 @@ def test_garbage_input_is_rejected() -> None:
226257
"denied_table",
227258
"generate_series_too_large",
228259
"select_into",
260+
"parse_error",
261+
"parse_too_deep",
229262
],
230263
)
231264
def test_violation_codes_are_documented(code: str) -> None:

0 commit comments

Comments
 (0)