fix: address ClickHouse driver review findings

[VJ-yadav]

Summary

• Add 9 edge-case tests covering all 4 findings from the 6-model code review panel (fix: address code review findings for ClickHouse driver (SQL parsing, nullable regression) #592)
• Nullable regression: verify LowCardinality(Nullable(FixedString(32))) is nullable, Map/Tuple wrappers are not, and undefined type falls back to non-nullable
• SQL parsing: verify leading block comments, multi-line block comments, backslash-escaped strings, and DDL keywords inside comments don't bypass LIMIT injection or misroute queries

Fixes #592

Test Plan

• All 55 tests pass (bun test clickhouse-unit)
• Typecheck clean (only pre-existing @clickhouse/client module error)

Checklist

• Tests cover all 4 review findings (nullable, trailing comment, leading comment, string literals)
• Edge cases: nested types, multi-line comments, escaped strings, DDL in comments
• No source changes needed — PR fix: address 7 P1 findings from v0.5.16 release evaluation #591 already fixed the driver code, these tests confirm correctness

Summary by CodeRabbit

• Tests
  • Extended coverage to ensure LIMIT injection protection still appends LIMIT when SQL has leading single-line or block comments, multi-line comments, or escaped-quote sequences containing comment-like text.
  • Added assertions that SELECT statements are executed via the correct query path even when comments include DDL-like keywords.
  • Expanded nullable/type detection tests for nested nullable, map/tuple patterns, and undefined type handling.

[claude]

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

[github-actions]

This PR doesn't fully meet our contributing guidelines and PR template.

What needs to be fixed:

• PR description is missing required template sections. Please use the PR template.

Please edit this PR description to address the above within 2 hours, or it will be automatically closed.

If you believe this was flagged incorrectly, please let a maintainer know.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 34989ffa-2194-4f77-ade0-717db893cbf9

📥 Commits

Reviewing files that changed from the base of the PR and between c684bf4 and a786481.

📒 Files selected for processing (1)

• packages/drivers/test/clickhouse-unit.test.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• packages/drivers/test/clickhouse-unit.test.ts

---

📝 Walkthrough

Walkthrough

Adds unit tests for ClickHouse driver covering LIMIT-injection edge cases with leading/comments/escaped strings and routing, plus tests for nullable type detection (including LowCardinality(Nullable(...)), container types, and undefined type handling).

Changes

Cohort / File(s)	Summary
ClickHouse LIMIT & Nullable Test Coverage packages/drivers/test/clickhouse-unit.test.ts	Added tests asserting appended LIMIT is not bypassed by leading single-line or block comments, or backslash-escaped quote sequences; ensured SELECT queries route to client.query() even when comments contain DDL keywords; expanded describeTable tests for LowCardinality(Nullable(...)), container types (Map, Tuple) and undefined column type handling.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• fix: address 7 P1 findings from v0.5.16 release evaluation #591: PR that implemented ClickHouse fixes for comment-/string-aware SQL parsing and nullable detection which these tests validate.

Suggested labels

contributor

"I'm a rabbit testing keys and quirks,
nibbling bugs with tiny works.
LIMITs now stick where they belong,
nullable nests sing a new song.
Hopping off — the tests are strong! 🐇"

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	The PR description covers all required sections (Summary, Test Plan, Checklist) and provides comprehensive detail on what was tested. However, the required PINEAPPLE identifier for AI-generated content is missing from the top of the description.	If this is AI-generated content, add 'PINEAPPLE' at the very top of the PR description before any other content, as required by the template.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: adding tests to address ClickHouse driver review findings, reflecting the PR's core objective.
Linked Issues check	✅ Passed	The PR successfully addresses all four findings from issue #592: nullable regression (LowCardinality/Map/Tuple type detection), trailing comments, leading comments, and string literal escaping—with comprehensive test coverage for each.
Out of Scope Changes check	✅ Passed	All changes are strictly in-scope: 57 new lines of unit tests covering the four findings from #592. No unrelated modifications or source code changes outside the stated objectives.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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 and usage tips.}

[dev-punia-altimate]

❌ Tests — Failures Detected

TypeScript — 15 failure(s)

• connection_refused [3.24ms]
• timeout [2.69ms]
• permission_denied [3.43ms]
• parse_error [2.49ms]
• oom [2.79ms]
• network_error [2.54ms]
• auth_failure [2.77ms]
• rate_limit [2.94ms]
• internal_error [3.02ms]
• empty_error [0.27ms]
• connection_refused [0.16ms]
• timeout [0.08ms]
• permission_denied [0.08ms]
• parse_error [0.08ms]
• oom [0.09ms]

cc @VJ-yadav
_{Tested at 09eca2ff | Run log | Powered by QA Autopilot}

[anandgupta42]

Review — Ready for merge

Scope: Test-only PR adding 9 edge-case tests for all 4 findings from the 6-model review of #591 (closes #592). No source changes — validates the existing driver fix.

Analysis

• packages/drivers/test/clickhouse-unit.test.ts (+57, single file) — cleanly targeted
• Covers all 4 findings from fix: address code review findings for ClickHouse driver (SQL parsing, nullable regression) #592:
  • Nullable regression: LowCardinality(Nullable(FixedString(32))), Map/Tuple wrappers, undefined fallback
  • SQL parsing: leading block comments, multi-line block comments, backslash-escaped strings, DDL keywords in comments
• No marker-guard implications (test file, not upstream-shared)
• Risk: very low — test additions only

CI status (commit 09eca2ff)

• ✅ TypeScript, anti-slop, check-standards, check-compliance, Marker Guard, GitGuardian — all green
• ⚠️ centralized-test-results shows 15 failures, but the same generic error-classifier failures (connection_refused, timeout, permission_denied, oom, auth_failure…) appear identically on every open PR in this batch (fix: update remaining opencode references in help output and CLI #547, fix: re-trace system prompt on agent switch #548, fix: pass warehouse dialect to altimate-core tools instead of hardcoding snowflake #550, fix: add pagination support for recap trace list #596, fix: URL-encode special characters in connection string passwords #597, fix: address ClickHouse driver review findings #599). This is pre-existing flakiness from the altimate-qa external test runner, not a regression from this PR.

Recommendation
✅ Ready for merge — rebase on main (currently BEHIND) to pick up latest release tags; no code review changes required.

cc @anandgupta42 — ready for /release once rebased.

[anandgupta42]

Consensus code-review applied — no changes needed

Ran a 4-participant consensus review (Claude + GPT 5.4 + Gemini 3.1 Pro + self-review).

Verdict: APPROVE — GPT concluded this is "mergeable as-is". Gemini separately identified a pre-existing ClickHouse driver bug (LIMIT-bypass via double-quoted/backtick identifiers in packages/drivers/src/clickhouse.ts:73) but explicitly noted it's not introduced by this PR (which is test-only).

Gemini's finding is a legitimate scope-expansion candidate — file as follow-up issue:

• sqlCleaned pipeline in the ClickHouse driver strips single-quoted strings but not double-quoted identifiers or backticks
• SQL like SELECT * FROM t AS "my LIMIT bypass" causes /\bLIMIT\b/i to match falsely, skipping LIMIT injection
• Fix: add .replace(/"(?:[^"\\]|\\.|"")*"/g, "") and backtick stripping to sqlCleaned
• Add regression tests for double-quoted/backticked identifiers containing LIMIT

Both models agree this PR's 9 edge-case tests correctly cover the 4 findings from the #592 review panel (nullable regression, trailing comment, leading comment, string literals). No changes to this PR are required.

All CI green. Ready for merge as-is.