refactor(mssql): strip trailing semicolon on unlimited query path for connector consistency

[Bartok9]

Summary

Strip trailing semicolons on the MSSQL unlimited _raw_cursor_sql path so it matches dry_run / limited composition and sibling connectors (e.g. #2595 MySQL).

Addressing @goldmedal review

You're right that the earlier fix(...) framing over-claimed a pyodbc multi-statement failure. I still cannot reproduce pyodbc rejecting a single statement with a lone trailing ;.

This update is therefore an explicit consistency refactor, not a driver-bug fix:

• Unlimited path now calling strip_trailing_semicolon (same as limit rewrite / dry_run / MySQL refactor(mysql): strip trailing semicolon on unlimited query path for connector consistency #2595).
• Test comment no longer says "execute path allows it" — it documents the new consistency contract and states the non-claim on pyodbc.
• Rebased onto current main.

Motivation

Client-pasted SQL with a trailing ; should not take a different terminator shape solely because limit is None. Align MSSQL with the shared connector strip helper.

Verification

./.venv/bin/python -m pytest tests/unit/test_mssql_semicolon.py -v
3 passed

@goldmedal ready for another look when convenient — thank you again for the careful catch on the original framing.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f8a93511-31fb-42bc-b09b-1e5c5102790e

📥 Commits

Reviewing files that changed from the base of the PR and between 507bda6 and 1daa222.

📒 Files selected for processing (2)

• core/wren/src/wren/connector/mssql.py
• core/wren/tests/unit/test_mssql_semicolon.py

---

Walkthrough

MSSQL raw SQL generation now strips trailing semicolons regardless of whether a limit is provided. Unit tests cover single and repeated semicolon terminators on unlimited queries.

Changes

MSSQL semicolon normalization

Layer / File(s)	Summary
Normalize unlimited MSSQL SQL core/wren/src/wren/connector/mssql.py, core/wren/tests/unit/test_mssql_semicolon.py	_raw_cursor_sql strips trailing semicolons before the unlimited-query return path, with tests covering single and repeated terminators.

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

Possibly related issues

• Strip trailing semicolon on the unlimited query() path across connectors #2497 — Directly covers stripping trailing semicolons on the MSSQL unlimited query path.

Possibly related PRs

• Canner/WrenAI#2482 — Applies the same unlimited-path semicolon normalization pattern in another connector.
• Canner/WrenAI#2488 — Unconditionally strips trailing semicolons on an unlimited connector query path with corresponding tests.
• Canner/WrenAI#2490 — Updates connector SQL handling and tests for unlimited-query semicolon stripping.

Suggested labels: python, core

Suggested reviewers: goldmedal

Poem

A bunny found a semicolon trail,
And swept it clean from every SQL tale.
One mark, or two,
Now both paths come through—
Crisp queries hop onward without fail.

🚥 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 clearly and accurately summarizes the main MSSQL connector fix in the unlimited query path.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.}

[goldmedal]

Code review

This reverses a decision that main documents as deliberate. The existing test doesn't just happen to assert the old behaviour — it explains it:

WrenAI/core/wren/tests/unit/test_mssql_semicolon.py

Lines 23 to 26 in 5d2637c

	def test_raw_cursor_sql_no_limit_unchanged_except_strip_not_required():
	# limit None returns original (including trailing ;) — execute path allows it
	assert MSSqlConnector._raw_cursor_sql("SELECT 1;", None) == "SELECT 1;"

"execute path allows it" is a recorded finding, and this PR rewrites both the assertion and the comment to the opposite conclusion. That may well be the right call, but it needs to be argued rather than swapped: what changed since that test was written?

The stated reason — that stripping avoids tripping "pyodbc multi-statement restrictions with a lone terminator" — is the part to substantiate. A single statement with a trailing ; is accepted by pyodbc, so if there's a configuration where it isn't, please include the failure in the description. Without that, this is a consistency change like #2595 and should be titled refactor(...) accordingly.

Rebase before merge — 9 commits behind main.

[goldmedal]

To clean my review queue, I changed the PR status to draft. After addressing the review comment, you can request me again.

[Bartok9]

Thanks @goldmedal — you're right, and this is a fair challenge. I can't produce a case where pyodbc actually rejects a single statement with a lone trailing ; on the unlimited path, so the stated justification doesn't hold. Rather than rewrite a deliberately-documented decision ("execute path allows it") without evidence, I'll close this. If a concrete pyodbc failure with a lone terminator ever turns up, I'll reopen with the reproduction attached. Appreciate the careful read.

[Bartok9]

Addressed your review @goldmedal:

1. Dropped the pyodbc multi-statement claim — I still can't produce a lone-; rejection, so that justification was wrong.
2. Retitled to refactor(mssql): … for connector consistency (same pattern as refactor(mysql): strip trailing semicolon on unlimited query path for connector consistency #2595).
3. Honest test comment — documents consistency with dry_run/limit paths; no reverse of a driver finding presented as fact without evidence.
4. Rebased onto current main; tests/unit/test_mssql_semicolon.py → 3 passed.

Requesting your review again now that this is framed correctly. No rush — queue-cleaners appreciated last time.

[Bartok9]

Follow-up: GitHub would not reopen this PR after the branch was recreated on current main (force-push/recreate validation).

Addressed your points on a new PR (refactor title, no pyodbc over-claim, rebased, tests green) and requested your review there.