fix(redshift): coerce LIMIT before SQL interpolation

[Bartok9]

Summary

Redshift LIMIT path only used int(limit) inline without rejecting negatives. Centralize coerce + tests.

Test plan

• cd core/wren && .venv/bin/python -m pytest tests/unit/test_redshift_coerce_limit.py -q (3 passed)

Summary by CodeRabbit

• Bug Fixes

  • Improved validation of query limits for Redshift queries.
  • Negative or unsafe limit values are now rejected, while unlimited queries remain supported.
  • String-based limits are normalized before being used in generated SQL.

• Tests

  • Added coverage for valid, negative, injection-like, and string limit values.

[coderabbitai]

Caution

Review failed

The head commit changed during the review from f626c83 to 4648019.

Walkthrough

The Redshift connector now validates and normalizes query limits before SQL interpolation. Tests cover invalid limits and confirm that string limits become numeric SQL values.

Changes

Redshift limit validation

Layer / File(s)	Summary
Limit coercion and query validation core/wren/src/wren/connector/redshift.py, core/wren/tests/unit/test_redshift_coerce_limit.py	_coerce_limit preserves None, converts valid values to integers, and rejects negative or injection-like values. query uses the validated limit when building SQL. Unit tests verify these behaviors.

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

Possibly related PRs

• Canner/WrenAI#2624: Adds equivalent shared limit coercion with direct functional overlap.
• Canner/WrenAI#2625: Adds equivalent limit validation for the PostgreSQL connector.
• Canner/WrenAI#2626: Adds equivalent limit validation for the Trino connector.

Suggested reviewers: goldmedal

Poem

A rabbit checks the limit line,
Turns stringy bounds to numbers fine.
Negative hops are stopped with care,
Safe SQL travels through the lair.
Tests nod softly: all is clear.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description includes a summary and test command but omits the required failure reproduction, actual error output, and duplicate check.	Add the observed failure with actual error output and list the open pull requests checked for duplicates.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the Redshift LIMIT coercion and validation change.
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 💡 1

🛠️ Fix failing CI checks 💡

• Fix failing CI checks

🧪 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]

Closing in favour of a single consolidated change — thanks for the work, the underlying tidy-up is worth doing, just not as one PR per connector.

Why this is being closed rather than reviewed:

1. This is one mechanical change spread across eight PRs. The contribution bar added in docs: set an explicit contribution bar for agent-authored PRs #2602 asks for exactly this to be a single diff: "A mechanical change repeated across several files or connectors belongs in one PR, not one PR per file. Reviewers need to see the resulting convention in a single diff."

2. A shared helper already exists in this same batch, and none of these PRs use it. refactor(connector): centralize LIMIT coercion #2624 adds coerce_limit() to connector/base.py. Every other PR in the series re-declares a private _coerce_limit in its own module instead of importing it. Merged as-is the repo would carry nine copies of the same function (the eight here plus the existing one at connector/mysql.py:44).

3. The copies have already diverged before merge. base.py (refactor(connector): centralize LIMIT coercion #2624), postgres, trino, oracle, redshift and bigquery use int(limit) then a negativity check; fix(clickhouse): coerce LIMIT before SQL interpolation #2627 (clickhouse) additionally rejects fractional negatives such as -0.5 via numbers.Number; fix(duckdb): coerce LIMIT before SQL interpolation #2637 (duckdb) additionally rejects bool and non-integral float. Three different semantics for one contract is the specific outcome a shared helper prevents.

4. Coverage is inconsistent. connector/canner.py:255 interpolates a bare {limit} and is not covered by any PR in the series, while bigquery, duckdb and redshift already interpolate {int(limit)} today — for those three the only behavioural delta is the negativity check.

On the fix: label and the stated failure. The reproduction in the description calls connector.query(sql, limit="1; DROP TABLE users") directly. Tracing the call paths:

• run_sql in mcp_server.py declares limit: int | None, so a non-numeric string is rejected by tool-argument validation, and mcp_server.py:84 already rejects negatives and clamps to MAX_ROW_LIMIT.
• The CLI declares --limit/-l as Optional[int], so a non-integer is rejected at parse time.
• That leaves Engine.query(sql, limit) as a Python API. At that boundary the caller already supplies sql verbatim — anyone able to pass limit="1; DROP TABLE t" can pass that as sql instead. limit is not a lower-trust channel than sql there, so this is not an injection path.

What remains is genuine but smaller: a negative limit currently surfaces as a driver-level error instead of a clear ValueError, and the coercion contract is inconsistent across connectors. That is refactor:, per "fix: requires a reproducible failure that the change repairs."

What we would take instead — a single PR, refactor(connector): centralize LIMIT coercion, that:

• keeps one coerce_limit() in connector/base.py, with the strictest semantics of the three variants above (reject bool, non-integral values, and negatives);
• routes every interpolating connector through it — including canner.py, and replacing the private copy in mysql.py;
• leaves ConnectorABC.query's signature as int | None (see the per-PR note on fix(duckdb): coerce LIMIT before SQL interpolation #2637 below);
• tests the helper once in tests/unit/test_coerce_limit.py, with at most a smoke test per connector proving it is wired in, rather than repeating the same six cases eight times.

#2624 is the natural home for that; it is being kept open with a note to that effect.

---

On this PR specifically — redshift.py:50 already interpolates {int(limit)}; the only new behaviour here is rejecting negatives.