fix(connector): coerce non-string SQL in strip_trailing_semicolon

[Bartok9]

Summary

• Coerce non-string sql arguments in strip_trailing_semicolon (None → "", other values via str(...)).
• Prevents TypeError inside the trailing-semicolon regex when callers pass accidental non-strings.

Motivation

Every connector funnels user/limit/EXOLAIN composition through this helper. A None or int SQL argument currently raises TypeError: expected string or bytes-like object deep in re.sub, masking the real caller bug with a generic regex crash.

License

Apache-2.0 path: core/wren/src/wren/connector/base.py (+ unit test).

Verification

cd core/wren && .venv/bin/python -m pytest tests/unit/test_strip_trailing_semicolon_coerce.py -q → 3 passed

Test plan

• unit tests for string / None / int
• CI green

Summary by CodeRabbit

• Bug Fixes

  • Improved SQL statement handling when input is empty, null, or not provided as text.
  • Non-text values are now safely converted to text before trailing semicolons and whitespace are removed.

• Tests

  • Added coverage for standard strings, null values, and non-text inputs.

[coderabbitai]

Walkthrough

The shared strip_trailing_semicolon helper now handles None and non-string inputs before trimming trailing semicolons. New unit tests cover string, null, and numeric inputs.

Changes

Semicolon Input Normalization

Layer / File(s)	Summary
Normalize helper inputs and verify behavior core/wren/src/wren/connector/base.py, core/wren/tests/unit/test_strip_trailing_semicolon_coerce.py	The helper returns an empty string for None, coerces other non-string values, preserves trailing-semicolon stripping, and adds focused tests for these cases.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• Canner/WrenAI#2517: Updates the same helper and tests equivalent input-coercion behavior.
• Canner/WrenAI#2480: Introduces the shared helper later hardened by this change.
• Canner/WrenAI#2477: Applies the helper in DatabricksConnector.query() with regression coverage.

Suggested reviewers: goldmedal

Poem

I’m a rabbit who trims each tail,
Semicolons vanish without fail.
None hops to an empty line,
Numbers turn to strings just fine.
Tests bloom softly in the trail.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: coercing non-string SQL inputs in strip_trailing_semicolon.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (1)

core/wren/src/wren/connector/base.py (1)

19-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Update the type hint to match the new behavior.

The function is now intentionally designed to accept None and non-string values. However, the function signature on line 11 still types sql as str (def strip_trailing_semicolon(sql: str) -> str:). This will cause static type checkers to report errors when non-string arguments are passed to this function.

Consider updating the type hint to Any (e.g., sql: Any) to accurately reflect this defensive behavior.

# In core/wren/src/wren/connector/base.py, around line 11:
from typing import Any

def strip_trailing_semicolon(sql: Any) -> str:

🤖 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 `@core/wren/src/wren/connector/base.py` around lines 19 - 27, Update the
`strip_trailing_semicolon` parameter annotation to accept the `None` and
non-string values already handled by its implementation, importing `Any` from
`typing` if needed, while keeping the return type as `str`.

🤖 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.

Nitpick comments:
In `@core/wren/src/wren/connector/base.py`:
- Around line 19-27: Update the `strip_trailing_semicolon` parameter annotation
to accept the `None` and non-string values already handled by its
implementation, importing `Any` from `typing` if needed, while keeping the
return type as `str`.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 0cafb939-d329-405d-9f5f-0343e307f5a4

📥 Commits

Reviewing files that changed from the base of the PR and between 3dac00a and c96c2d6.

📒 Files selected for processing (2)

• core/wren/src/wren/connector/base.py
• core/wren/tests/unit/test_strip_trailing_semicolon_coerce.py

[goldmedal]

Thanks for the patch, and for the earlier hardening work in the formatter helpers (#2521, #2522, #2524) — those sat at a real trust boundary, where the input is model/tool-produced JSON. This one is different, and I'd rather not take it. Closing, with the reasoning below.

The input here is internally generated, not untrusted

The only production path into strip_trailing_semicolon is the dialect_sql a connector receives, and that string is produced by Engine.dry_plan() (wren-core + sqlglot generate) — see core/wren/src/wren/engine.py:112 and :128. It is a str by construction. The PR doesn't identify a caller that can pass None or an int, and there's no reproducer for the TypeError it describes.

The coercion makes the failure harder to diagnose, not easier

If None ever did reach the helper, "" composes into invalid SQL that we then send to the warehouse:

connector	resulting SQL
mysql.py:41	"\nLIMIT 10"
postgres.py:254, trino.py:456, canner.py:248	SELECT * FROM () AS _sub LIMIT 10
snowflake.py:69	SELECT * FROM (\n\n) AS _wren_sub LIMIT 10

42 becomes SELECT * FROM (42) AS _sub LIMIT 10. Each of these fails remotely and gets wrapped by engine.py:117-125 as GENERIC_USER_ERROR with phase=SQL_EXECUTION — so a planner/caller bug is reported to the user as an error in their SQL. The current TypeError is ugly, but its traceback points straight at the real caller. The PR's stated goal is to stop masking the caller bug; the effect is to mask it.

Smaller points

• The signature still reads sql: str while the body handles non-strings. Widening it to Any (as suggested in the automated review) isn't the fix either — that removes the static check that catches this class of bug at authoring time. Either the value is a str and the guard is dead code, or the annotation is wrong; neither resolution argues for merging as-is.
• The guard is also partial: connectors that don't route through this helper (e.g. bigquery.py:40) would still raise TypeError, so this doesn't establish a system-level invariant. The place to validate opaque input is the boundary — Engine.query or the MCP tool arguments (already schema-validated).
• core/wren/tests/unit/test_strip_trailing_semicolon.py already exists and covers the string cases via parametrize; test_strips_string in the new file duplicates two of them. Additional cases belong in that file rather than a second module. (Lint is green because ruff here only selects E,F,I,PLC — that isn't a signal about consistency with the surrounding test style.)

A more useful version of this change

The empty-string path is already reachable today: strip_trailing_semicolon(" ; ") returns "" and composes exactly the broken SQL in the table above — untested and unguarded. Rejecting empty/whitespace-only SQL early with an explicit error, or raising a clearly-worded TypeError instead of letting re.sub do it, would serve the motivation in the PR description without silently degrading to invalid SQL. Happy to look at a PR along those lines.

[Bartok9]

Completely fair — thanks for the thorough writeup. You're right that the production path is str by construction via dry_plan(), and coercing to "" would turn a caller bug into invalid SQL sent to the warehouse (surfacing as a misleading GENERIC_USER_ERROR), which defeats the point. The TypeError traceback pointing at the real caller is the more useful signal.

The empty/whitespace-only observation is the interesting one — strip_trailing_semicolon(" ; ") → "" already composes the same broken SQL today, unguarded. Happy to follow up with a small PR that rejects empty/whitespace-only SQL early with an explicit error at the boundary, rather than widening this helper. Appreciate the guidance.