Open
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
📝 WalkthroughWalkthroughThis PR introduces ClickHouse as the 12th native database driver for Altimate Code. It includes driver implementation with connection and query handling, full integration across 23 integration points (registry, discovery, dbt, finops), E2E test coverage for four server versions, and comprehensive documentation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs(No PRs with strong code-level connections were identified in the provided context.) Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Add first-class ClickHouse support as the 12th database driver: **Driver (`packages/drivers/src/clickhouse.ts`):** - Official `@clickhouse/client` over HTTP(S) - Supports ClickHouse server 23.3+ (all non-EOL versions) - Password, connection string, and TLS/mTLS auth - ClickHouse Cloud and self-hosted compatible - Parameterized queries for SQL injection prevention - DML-aware LIMIT injection (won't break `WITH...INSERT`) **Integration (23 touchpoints):** - Registry: `DRIVER_MAP`, import switch, `PASSWORD_DRIVERS` - Discovery: Docker containers, env vars (`CLICKHOUSE_HOST`/`CLICKHOUSE_URL`), dbt profiles (`ADAPTER_TYPE_MAP`), dbt lineage dialect - FinOps: `system.query_log` query history template - Normalization: aliases for `connectionString`, `requestTimeout`, TLS fields - Publish: `@clickhouse/client` in `peerDependencies` **Tests:** - 30+ E2E tests across 5 suites (latest, LTS 23.8, 24.3, 24.8, connection string) - 14 config normalization tests for all ClickHouse aliases - MergeTree variants, materialized views, Nullable columns, Array/Map/IPv4 types **Documentation:** - Full config section in `warehouses.md` (standard, Cloud, connection string) - Support matrix entry in `drivers.md` with auth methods - Dedicated guide (`guides/clickhouse.md`): MergeTree optimization, materialized view pipelines, dialect translation, LowCardinality tips, dbt integration - Updated README, getting-started, warehouse-tools docs **Engineering:** - `packages/drivers/ADDING_A_DRIVER.md` — 23-point checklist for adding future drivers - `.claude/commands/add-database-driver.md` — Claude skill to automate the process Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
anandgupta42
force-pushed
the
feat/add-clickhouse-driver
branch
from
350e956 to
e63d6c6
Compare
- `execute()` now uses `client.command()` for INSERT/CREATE/DROP/ALTER queries instead of `client.query()` with JSONEachRow format, which caused parse errors on INSERT VALUES - Add `CLICKHOUSE_DEFAULT_ACCESS_MANAGEMENT=1` to all LTS Docker containers (required for passwordless default user) - Fix UInt64 assertion to handle both string and number JSON encoding Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add `clickhouse/clickhouse-server:latest` as a GitHub Actions service - Add test step running `drivers-clickhouse-e2e.test.ts` with CI env vars - Add test file to change detection paths for the `drivers` filter Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ilures) Ran 167 adversarial tests against real ClickHouse Docker containers covering SQL injection, unicode, NULLs, LIMIT edge cases, exotic types, error handling, large data, MergeTree variants, views, system tables, concurrent operations, and return value edge cases. **Bugs found and fixed:** 1. **DESCRIBE/EXISTS get LIMIT appended** — `isSelectLike` regex matched DESCRIBE/EXISTS but ClickHouse doesn't support LIMIT on these statements. Fix: narrowed `supportsLimit` to only `SELECT` and `WITH` queries. 2. **`limit=0` returns 0 rows** — truncation check `rows.length > 0` was always true, causing `slice(0, 0)` to return empty array. Fix: guard with `effectiveLimit > 0 &&` before truncation check. 3. **`limit=0` treated as `limit=1000`** — `0 ?? 1000` returns 0 (correct) but `limit === undefined ? 1000 : limit` properly distinguishes "not provided" from "explicitly zero". Changed from `??` to explicit check. **Regression tests added (5 tests in main E2E suite):** - DESCRIBE TABLE without LIMIT error - EXISTS TABLE without LIMIT error - limit=0 returns all rows without truncation - INSERT uses `client.command()` not `client.query()` - WITH...INSERT does not get LIMIT appended Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
LTS 24.3 and 24.8 test suites incorrectly used `CH_USE_CI` (tied to `TEST_CLICKHOUSE_HOST`) which made them skip Docker startup in CI but still try to connect on ports 18125/18126 — causing 60s timeouts. Give each LTS suite its own `_USE_CI` flag (`CH_243_USE_CI`, `CH_248_USE_CI`) so they properly fall through to Docker-start-or-skip behavior, matching how LTS 23.8 already works with `CH_LTS_USE_CI`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
packages/opencode/test/altimate/driver-normalize.test.ts (1)
887-922: Add coverage for the remaining ClickHouse TLS aliases (ca_cert,ssl_cert).
CLICKHOUSE_ALIASESincludes both, but this suite currently doesn’t assert them. Adding these two cases will make alias coverage complete and protect against regressions.✅ Suggested additional tests
+ test("ca_cert → tls_ca_cert", () => { + const result = normalizeConfig({ + type: "clickhouse", + ca_cert: "/path/to/ca.pem", + }) + expect(result.tls_ca_cert).toBe("/path/to/ca.pem") + expect(result.ca_cert).toBeUndefined() + }) + + test("ssl_cert → tls_cert", () => { + const result = normalizeConfig({ + type: "clickhouse", + ssl_cert: "/path/to/cert.pem", + }) + expect(result.tls_cert).toBe("/path/to/cert.pem") + expect(result.ssl_cert).toBeUndefined() + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/altimate/driver-normalize.test.ts` around lines 887 - 922, Add two tests to fully cover ClickHouse TLS aliases: create a test named "ca_cert → tls_ca_cert" that calls normalizeConfig({ type: "clickhouse", ca_cert: "/path/to/ca.pem" }) and assert result.tls_ca_cert === "/path/to/ca.pem" and result.ca_cert is undefined; and create a test named "ssl_cert → tls_cert" that calls normalizeConfig({ type: "clickhouse", ssl_cert: "/path/to/cert.pem" }) and assert result.tls_cert === "/path/to/cert.pem" and result.ssl_cert is undefined; reference normalizeConfig and the new test names when locating where to add them in the suite.packages/opencode/src/altimate/native/dbt/lineage.ts (1)
103-113: Consider de-duplicating adapter→dialect mapping to avoid drift.
detectDialect()now has its own map including ClickHouse, whilepackages/opencode/src/altimate/native/connections/dbt-profiles.tsmaintains another adapter map. A shared constant would reduce future mismatch risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/native/dbt/lineage.ts` around lines 103 - 113, detectDialect() duplicates an adapter→dialect map that also exists in the dbt-profiles adapter map; extract a single shared constant (e.g., export ADAPTER_TO_DIALECT or ADAPTER_DIALECT_MAP) into a common module and replace the local dialectMap in detectDialect() with an import of that constant, then update the other module to import the same constant instead of maintaining its own map so both detectDialect() and the dbt-profiles adapter mapping reference the single source of truth.packages/opencode/src/altimate/tools/warehouse-add.ts (1)
14-29: Document MongoDB in the canonical field list too.Line 36 now advertises
mongodbas a supportedconfig.type, but this description block still omits it. Since this text is what the tool planner sees, the mismatch makes MongoDB additions less reliable than the runtime behavior.Also applies to: 36-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/tools/warehouse-add.ts` around lines 14 - 29, The config description string on the z.record(...) describe(...) (symbol: config) is missing canonical MongoDB fields even though mongodb is advertised as a supported config.type; update that long description to include a "mongodb" bullet with canonical fields (e.g., uri/connection_string, host, port, database, user, password, authSource, replicaSet, tls/tlsCAFile/tlsCertificateKeyFile, ssl, replicaSet) and add a short MongoDB auth/connection example so the planner and runtime are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/commands/add-database-driver.md:
- Around line 129-142: The test block currently runs "cd packages/opencode &&
bun test test/altimate/driver-normalize.test.ts
test/altimate/connections.test.ts" which leaves the shell in packages/opencode
so the subsequent "bun turbo typecheck" and "bun run script/upstream/analyze.ts
--markers --base main --strict" run from the wrong directory and the E2E suite
isn't executed; change the test invocation to run in a subshell (e.g., wrap the
bun test invocation in parentheses) and include
"test/altimate/drivers-{database}-e2e.test.ts" in the bun test arguments so the
new driver’s Docker tests execute, and ensure "bun turbo typecheck" and "bun run
script/upstream/analyze.ts --markers --base main --strict" are executed from the
repo root (i.e., not affected by the cd).
In `@docs/docs/configure/warehouses.md`:
- Around line 291-350: The docs still contain stale references that mark
ClickHouse as unsupported and omit it from Docker auto-discovery; locate the
ClickHouse section and any mentions of “unsupported” or “not supported”
referring to ClickHouse (search for the "ClickHouse" heading and the Docker
auto-discovery paragraph) and remove or update those lines to reflect ClickHouse
is supported, and add ClickHouse to the Docker auto-discovery list/description
where other supported warehouses are enumerated (also update the other two
occurrences mentioned in the review). Ensure the wording matches the new
supported phrasing used in the ClickHouse section (e.g., reference supported
versions and cloud options).
In `@docs/docs/drivers.md`:
- Around line 138-144: Add blank lines before and after the ClickHouse table
block to satisfy markdownlint MD058: ensure there's an empty line between the
"### ClickHouse" header and the table start, and another empty line after the
table end; edit the block containing the "### ClickHouse" header and the
subsequent table (`| Method | Config Fields |` ... `| TLS/HTTPS | ... |`) so the
table is separated by blank lines from surrounding text.
In `@packages/drivers/ADDING_A_DRIVER.md`:
- Line 181: Add a language tag to the fenced code block at the shown snippet in
ADDING_A_DRIVER.md (the file-map block starting with "packages/drivers/") to
satisfy markdownlint MD040; update the opening triple-backticks to include a
language like text (e.g., change ``` to ```text) so the block is explicitly
marked as plain text.
In `@packages/drivers/src/clickhouse.ts`:
- Line 63: The execute(sql: string, limit?: number, _binds?: any[]) function
currently accepts but ignores _binds and then runs raw SQL (the raw query
execution block), which can silently bypass parameterization; update execute to
fail fast when binds are provided or implement proper parameter mapping—e.g., at
the start of execute() check if _binds is non-empty and throw a clear error like
"binds not supported for ClickHouse execute" (or alternatively map _binds into
ClickHouse parameters and pass them to the client query API), and ensure the raw
SQL execution code path (the block that currently executes the text query) is
only used when no binds are supplied.
In `@packages/drivers/src/normalize.ts`:
- Around line 90-93: The config normalization maps ssl_* → tls_* (see
normalizeConfig), but saveConnection uses SENSITIVE_FIELDS to strip secrets and
currently lacks the normalized keys; update the SENSITIVE_FIELDS array (in
credential-store.ts where SENSITIVE_FIELDS is defined) to include "tls_ca_cert",
"tls_cert", and "tls_key" so that saveConnection will recognize and strip the
normalized tls_* fields after normalizeConfig runs.
In `@packages/opencode/src/altimate/native/finops/query-history.ts`:
- Around line 169-175: The ClickHouse branch is embedding unvalidated days and
limit into CLICKHOUSE_HISTORY_SQL; clamp and sanitize days and limit before
substitution to prevent NaN/Infinity/negative values producing invalid UInt32
SQL. Convert inputs with Number(...), treat non-finite values as 0, clamp to the
UInt32 range (0 to 4294967295), and use Math.floor on the sanitized value before
replacing the "{days:UInt32}" and "{limit:UInt32}" tokens in the
CLICKHOUSE_HISTORY_SQL string inside the whType === "clickhouse" block so the
returned sql always contains valid UInt32 integers.
In `@packages/opencode/src/altimate/tools/project-scan.ts`:
- Around line 224-235: The new ClickHouse entry (type: "clickhouse") was added
but the generic DATABASE_URL classifier still maps unknown schemes to Postgres,
so DATABASE_URL=clickhouse://... will be misclassified; update the DATABASE_URL
parsing logic in project-scan.ts to recognize the "clickhouse" scheme (and any
common aliases like "clickhouse+http"/"clickhouse+native" if used) and return
the "clickhouse" type instead of defaulting to "postgres", making the classifier
consistent with the configMap for the clickhouse entry.
In `@packages/opencode/test/altimate/drivers-clickhouse-e2e.test.ts`:
- Around line 25-30: The waitForPort helper currently hardcodes the host to
"127.0.0.1" (see function waitForPort and its use of createConnection), which
breaks tests when suites configure a different host; change waitForPort
signature to accept an optional host parameter (defaulting to "127.0.0.1") and
use that host when calling createConnection, then update all call sites (the
tests around lines referenced in the comment) to pass their configured host
instead of relying on localhost; apply the same change to the other versioned
suites that copy this helper so all readiness checks use the suite-configured
host.
- Around line 51-69: The waitForDbReady function leaks connectors on failed
attempts: after calling connectFn() you must ensure any connector returned is
closed when the attempt fails; modify waitForDbReady (around the
connectFn/connector usage) to call and await a safe close/disconnect on the
returned connector (e.g., await connector.close() or connector.disconnect() if
present) inside the catch (or a per-attempt finally) and guard the call with a
typeof/exists check and its own try/catch so close errors don’t mask the
original error; keep using lastErr for the final thrown message.
---
Nitpick comments:
In `@packages/opencode/src/altimate/native/dbt/lineage.ts`:
- Around line 103-113: detectDialect() duplicates an adapter→dialect map that
also exists in the dbt-profiles adapter map; extract a single shared constant
(e.g., export ADAPTER_TO_DIALECT or ADAPTER_DIALECT_MAP) into a common module
and replace the local dialectMap in detectDialect() with an import of that
constant, then update the other module to import the same constant instead of
maintaining its own map so both detectDialect() and the dbt-profiles adapter
mapping reference the single source of truth.
In `@packages/opencode/src/altimate/tools/warehouse-add.ts`:
- Around line 14-29: The config description string on the z.record(...)
describe(...) (symbol: config) is missing canonical MongoDB fields even though
mongodb is advertised as a supported config.type; update that long description
to include a "mongodb" bullet with canonical fields (e.g.,
uri/connection_string, host, port, database, user, password, authSource,
replicaSet, tls/tlsCAFile/tlsCertificateKeyFile, ssl, replicaSet) and add a
short MongoDB auth/connection example so the planner and runtime are consistent.
In `@packages/opencode/test/altimate/driver-normalize.test.ts`:
- Around line 887-922: Add two tests to fully cover ClickHouse TLS aliases:
create a test named "ca_cert → tls_ca_cert" that calls normalizeConfig({ type:
"clickhouse", ca_cert: "/path/to/ca.pem" }) and assert result.tls_ca_cert ===
"/path/to/ca.pem" and result.ca_cert is undefined; and create a test named
"ssl_cert → tls_cert" that calls normalizeConfig({ type: "clickhouse", ssl_cert:
"/path/to/cert.pem" }) and assert result.tls_cert === "/path/to/cert.pem" and
result.ssl_cert is undefined; reference normalizeConfig and the new test names
when locating where to add them in the suite.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5211562f-d2d1-4cd7-8599-181354eeec9b
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.claude/commands/add-database-driver.md.github/meta/commit.txt.github/workflows/ci.ymlREADME.mddocs/docs/configure/warehouses.mddocs/docs/data-engineering/guides/clickhouse.mddocs/docs/data-engineering/guides/index.mddocs/docs/data-engineering/tools/warehouse-tools.mddocs/docs/drivers.mddocs/docs/getting-started/index.mddocs/mkdocs.ymlpackages/drivers/ADDING_A_DRIVER.mdpackages/drivers/package.jsonpackages/drivers/src/clickhouse.tspackages/drivers/src/index.tspackages/drivers/src/normalize.tspackages/opencode/.github/meta/commit.txtpackages/opencode/script/publish.tspackages/opencode/src/altimate/native/connections/dbt-profiles.tspackages/opencode/src/altimate/native/connections/docker-discovery.tspackages/opencode/src/altimate/native/connections/registry.tspackages/opencode/src/altimate/native/dbt/lineage.tspackages/opencode/src/altimate/native/finops/query-history.tspackages/opencode/src/altimate/tools/project-scan.tspackages/opencode/src/altimate/tools/warehouse-add.tspackages/opencode/test/altimate/driver-normalize.test.tspackages/opencode/test/altimate/drivers-clickhouse-e2e.test.ts
Comment on lines
+129
to
+142
| ## Step 4: Run Quality Gates | ||
|
|
||
| ```bash | ||
| # Tests (from packages/opencode/) | ||
| cd packages/opencode && bun test test/altimate/driver-normalize.test.ts test/altimate/connections.test.ts | ||
|
|
||
| # Typecheck | ||
| bun turbo typecheck | ||
|
|
||
| # Marker check | ||
| bun run script/upstream/analyze.ts --markers --base main --strict | ||
| ``` | ||
|
|
||
| All three must pass before proceeding. |
There was a problem hiding this comment.
Fix the quality-gate recipe's working-directory and coverage gap.
After the first line, the shell stays in packages/opencode, so bun turbo typecheck and the marker check run from the wrong directory. This block also never executes test/altimate/drivers-{database}-e2e.test.ts, so the workflow can declare the new driver green without running its Docker suite.
🛠️ Proposed fix
```bash
# Tests (from packages/opencode/)
-cd packages/opencode && bun test test/altimate/driver-normalize.test.ts test/altimate/connections.test.ts
+(cd packages/opencode && bun test \
+ test/altimate/driver-normalize.test.ts \
+ test/altimate/connections.test.ts \
+ test/altimate/drivers-{database}-e2e.test.ts)
-# Typecheck
+# Typecheck (from repo root)
bun turbo typecheck
-# Marker check
+# Marker check (from repo root)
bun run script/upstream/analyze.ts --markers --base main --strict</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
## Step 4: Run Quality Gates
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/commands/add-database-driver.md around lines 129 - 142, The test
block currently runs "cd packages/opencode && bun test
test/altimate/driver-normalize.test.ts test/altimate/connections.test.ts" which
leaves the shell in packages/opencode so the subsequent "bun turbo typecheck"
and "bun run script/upstream/analyze.ts --markers --base main --strict" run from
the wrong directory and the E2E suite isn't executed; change the test invocation
to run in a subshell (e.g., wrap the bun test invocation in parentheses) and
include "test/altimate/drivers-{database}-e2e.test.ts" in the bun test arguments
so the new driver’s Docker tests execute, and ensure "bun turbo typecheck" and
"bun run script/upstream/analyze.ts --markers --base main --strict" are executed
from the repo root (i.e., not affected by the cd).
Comment on lines
+291
to
+350
| ## ClickHouse | ||
|
|
||
| ```json | ||
| { | ||
| "clickhouse-prod": { | ||
| "type": "clickhouse", | ||
| "host": "localhost", | ||
| "port": 8123, | ||
| "database": "analytics", | ||
| "user": "default", | ||
| "password": "{env:CLICKHOUSE_PASSWORD}" | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| | Field | Required | Description | | ||
| |-------|----------|-------------| | ||
| | `connection_string` | No | Full URL (alternative to individual fields, e.g. `http://user:pass@host:8123`) | | ||
| | `host` | No | Hostname (default: `localhost`) | | ||
| | `port` | No | HTTP port (default: `8123`) | | ||
| | `database` | No | Database name (default: `default`) | | ||
| | `user` | No | Username (default: `default`) | | ||
| | `password` | No | Password | | ||
| | `protocol` | No | `http` or `https` (default: `http`) | | ||
| | `request_timeout` | No | Request timeout in ms (default: `30000`) | | ||
| | `tls_ca_cert` | No | Path to CA certificate for TLS | | ||
| | `tls_cert` | No | Path to client certificate for mutual TLS | | ||
| | `tls_key` | No | Path to client key for mutual TLS | | ||
| | `clickhouse_settings` | No | Object of ClickHouse server settings | | ||
|
|
||
| ### ClickHouse Cloud | ||
|
|
||
| ```json | ||
| { | ||
| "clickhouse-cloud": { | ||
| "type": "clickhouse", | ||
| "host": "abc123.us-east-1.aws.clickhouse.cloud", | ||
| "port": 8443, | ||
| "protocol": "https", | ||
| "user": "default", | ||
| "password": "{env:CLICKHOUSE_CLOUD_PASSWORD}", | ||
| "database": "default" | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### Using a connection string | ||
|
|
||
| ```json | ||
| { | ||
| "clickhouse-prod": { | ||
| "type": "clickhouse", | ||
| "connection_string": "https://default:secret@my-ch.cloud:8443" | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| !!! info "Server compatibility" | ||
| The ClickHouse driver supports ClickHouse server versions 23.3 and later, covering all non-EOL releases. This includes LTS releases 23.8, 24.3, 24.8, and all stable releases through the current version. | ||
|
|
There was a problem hiding this comment.
Remove stale “unsupported” references now that ClickHouse is documented as supported.
The new ClickHouse section conflicts with two older lines in the same doc: ClickHouse is still listed as unsupported, and Docker auto-discovery text omits ClickHouse.
📝 Suggested doc patch
## Unsupported Databases
@@
-| ClickHouse | Use the bash tool with `clickhouse-client` or `curl` to query directly |
| Cassandra | Use the bash tool with `cqlsh` to query directly |
| CockroachDB | PostgreSQL-compatible — use `type: postgres` |
| TimescaleDB | PostgreSQL extension — use `type: postgres` |
@@
-| Docker containers | Finds running PostgreSQL, MySQL, and SQL Server containers |
+| Docker containers | Finds running PostgreSQL, MySQL, SQL Server, and ClickHouse containers |Also applies to: 383-383, 425-425
🧰 Tools
🪛 LanguageTool
[style] ~347-~347: Using many exclamation marks might seem excessive (in this case: 18 exclamation marks for a text that’s 9075 characters long)
Context: ...ult:secret@my-ch.cloud:8443" } } ``` !!! info "Server compatibility" The Cli...
(EN_EXCESSIVE_EXCLAMATION)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/docs/configure/warehouses.md` around lines 291 - 350, The docs still
contain stale references that mark ClickHouse as unsupported and omit it from
Docker auto-discovery; locate the ClickHouse section and any mentions of
“unsupported” or “not supported” referring to ClickHouse (search for the
"ClickHouse" heading and the Docker auto-discovery paragraph) and remove or
update those lines to reflect ClickHouse is supported, and add ClickHouse to the
Docker auto-discovery list/description where other supported warehouses are
enumerated (also update the other two occurrences mentioned in the review).
Ensure the wording matches the new supported phrasing used in the ClickHouse
section (e.g., reference supported versions and cloud options).
Comment on lines
+138
to
+144
| ### ClickHouse | ||
| | Method | Config Fields | | ||
| |--------|--------------| | ||
| | Password | `host`, `port`, `database`, `user`, `password` | | ||
| | Connection String | `connection_string: "http://user:pass@host:8123"` | | ||
| | TLS/HTTPS | `protocol: "https"`, `tls_ca_cert`, `tls_cert`, `tls_key` | | ||
|
|
There was a problem hiding this comment.
Add blank lines around the ClickHouse auth table to satisfy markdownlint.
MD058 is triggered here; add spacing before/after the table block.
🧹 Minimal lint fix
### ClickHouse
+
| Method | Config Fields |
|--------|--------------|
| Password | `host`, `port`, `database`, `user`, `password` |
| Connection String | `connection_string: "http://user:pass@host:8123"` |
| TLS/HTTPS | `protocol: "https"`, `tls_ca_cert`, `tls_cert`, `tls_key` |
+
ClickHouse driver supports server versions 23.3+ (all non-EOL releases). Uses the official `@clickhouse/client` package over HTTP(S). Compatible with ClickHouse Cloud, self-hosted, and Altinity.Cloud. Query history available via `system.query_log`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ### ClickHouse | |
| | Method | Config Fields | | |
| |--------|--------------| | |
| | Password | `host`, `port`, `database`, `user`, `password` | | |
| | Connection String | `connection_string: "http://user:pass@host:8123"` | | |
| | TLS/HTTPS | `protocol: "https"`, `tls_ca_cert`, `tls_cert`, `tls_key` | | |
| ### ClickHouse | |
| | Method | Config Fields | | |
| |--------|--------------| | |
| | Password | `host`, `port`, `database`, `user`, `password` | | |
| | Connection String | `connection_string: "http://user:pass@host:8123"` | | |
| | TLS/HTTPS | `protocol: "https"`, `tls_ca_cert`, `tls_cert`, `tls_key` | | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 139-139: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/docs/drivers.md` around lines 138 - 144, Add blank lines before and
after the ClickHouse table block to satisfy markdownlint MD058: ensure there's
an empty line between the "### ClickHouse" header and the table start, and
another empty line after the table end; edit the block containing the "###
ClickHouse" header and the subsequent table (`| Method | Config Fields |` ... `|
TLS/HTTPS | ... |`) so the table is separated by blank lines from surrounding
text.
|
|
||
| ## File Map | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced block at Line 181.
Markdownlint MD040 is valid here. Use a language like text for the file-map block.
Minimal fix
-```
+```text
packages/drivers/
src/
{database}.ts ← NEW: driver implementation
@@
docs/mkdocs.yml ← nav (if adding guide)</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 181-181: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @packages/drivers/ADDING_A_DRIVER.md at line 181, Add a language tag to the
fenced code block at the shown snippet in ADDING_A_DRIVER.md (the file-map block
starting with "packages/drivers/") to satisfy markdownlint MD040; update the
opening triple-backticks to include a language like text (e.g., change ``` to
| client = createClient(clientConfig) | ||
| }, | ||
|
|
||
| async execute(sql: string, limit?: number, _binds?: any[]): Promise<ConnectorResult> { |
There was a problem hiding this comment.
Do not silently ignore binds in execute().
At Line 63, _binds is accepted but never used, and Lines 84–87 execute raw SQL text. This can create false confidence that parameterization happened when it did not. Either map binds into ClickHouse parameters or fail fast when binds are provided.
Suggested fail-fast safeguard
- async execute(sql: string, limit?: number, _binds?: any[]): Promise<ConnectorResult> {
+ async execute(sql: string, limit?: number, binds?: any[]): Promise<ConnectorResult> {
+ if (binds && binds.length > 0) {
+ throw new Error("ClickHouse driver execute() does not support positional binds yet")
+ }
const effectiveLimit = limit === undefined ? 1000 : limitAlso applies to: 84-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/drivers/src/clickhouse.ts` at line 63, The execute(sql: string,
limit?: number, _binds?: any[]) function currently accepts but ignores _binds
and then runs raw SQL (the raw query execution block), which can silently bypass
parameterization; update execute to fail fast when binds are provided or
implement proper parameter mapping—e.g., at the start of execute() check if
_binds is non-empty and throw a clear error like "binds not supported for
ClickHouse execute" (or alternatively map _binds into ClickHouse parameters and
pass them to the client query API), and ensure the raw SQL execution code path
(the block that currently executes the text query) is only used when no binds
are supplied.
Comment on lines
+90
to
+93
| tls_ca_cert: ["tlsCaCert", "ssl_ca", "ca_cert"], | ||
| tls_cert: ["tlsCert", "ssl_cert"], | ||
| tls_key: ["tlsKey", "ssl_key"], | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Find where normalization and sensitive-field stripping are called.
rg -n "normalizeConfig\\(|isSensitiveField\\(" packages/opencode/src packages/opencode/test packages/drivers/src -C3
# 2) Inspect whether tls_* keys are included in sensitive checks/config stripping logic.
rg -n "tls_key|tls_cert|tls_ca_cert|ssl_key|ssl_cert|ssl_ca" packages/opencode/src packages/opencode/test packages/drivers/src -C3Repository: AltimateAI/altimate-code
Length of output: 50380
🏁 Script executed:
#!/bin/bash
# Find the CredentialStore implementation
fd -t f "CredentialStore" packages/ | head -20
# Also search for isSensitiveField implementation
rg -l "isSensitiveField" packages/ -t tsRepository: AltimateAI/altimate-code
Length of output: 299
🏁 Script executed:
#!/bin/bash
# Get the actual SENSITIVE_FIELDS list and isSensitiveField implementation
rg -n "SENSITIVE_FIELDS|isSensitiveField.*=|const.*isSensitiveField" packages/opencode/src -A 30 -t tsRepository: AltimateAI/altimate-code
Length of output: 7711
🏁 Script executed:
#!/bin/bash
# Check if tls_* fields are mentioned anywhere as sensitive in source code
rg -n "tls_key|tls_cert|tls_ca_cert" packages/opencode/src packages/drivers/src -B 2 -A 2Repository: AltimateAI/altimate-code
Length of output: 2380
🏁 Script executed:
#!/bin/bash
# Find where normalizeConfig and credential stripping/sanitization are called together
rg -n "normalizeConfig|stripSensitive|sanitize" packages/opencode/src -B 3 -A 3 -t ts | head -100Repository: AltimateAI/altimate-code
Length of output: 8014
🏁 Script executed:
#!/bin/bash
# Find where normalizeConfig and saveConnection are called together
rg -n "normalizeConfig|saveConnection" packages/opencode/src -B 5 -A 5 -t ts | grep -A 10 -B 10 "normalizeConfig\|saveConnection" | head -150Repository: AltimateAI/altimate-code
Length of output: 5215
🏁 Script executed:
#!/bin/bash
# Search for the connection handling flow that uses both functions
rg -n "saveConnection" packages/opencode/src -B 3 -A 3 -t tsRepository: AltimateAI/altimate-code
Length of output: 2469
🏁 Script executed:
#!/bin/bash
# Check connections handler/workflow
fd -t f "connection" packages/opencode/src | xargs grep -l "normalizeConfig\|saveConnection" | head -5Repository: AltimateAI/altimate-code
Length of output: 50
Add tls_* fields to the sensitive credential list — critical credential leakage.
The normalizeConfig() call in packages/opencode/src/altimate/native/connections/registry.ts:428 converts ssl_ca → tls_ca_cert, ssl_cert → tls_cert, and ssl_key → tls_key. Immediately after, saveConnection() at line 431 strips sensitive fields by checking against SENSITIVE_FIELDS in packages/opencode/src/altimate/native/connections/credential-store.ts:15. However, SENSITIVE_FIELDS includes ssl_ca, ssl_cert, and ssl_key but not tls_ca_cert, tls_cert, or tls_key.
Because normalization happens before sanitization, the config passed to saveConnection() contains the normalized tls_* keys, which are then not found in the sensitive field list and are persisted unencrypted to the config file. Add the tls_* variants to SENSITIVE_FIELDS to prevent cert/key material from leaking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/drivers/src/normalize.ts` around lines 90 - 93, The config
normalization maps ssl_* → tls_* (see normalizeConfig), but saveConnection uses
SENSITIVE_FIELDS to strip secrets and currently lacks the normalized keys;
update the SENSITIVE_FIELDS array (in credential-store.ts where SENSITIVE_FIELDS
is defined) to include "tls_ca_cert", "tls_cert", and "tls_key" so that
saveConnection will recognize and strip the normalized tls_* fields after
normalizeConfig runs.
Comment on lines
+169
to
+175
| if (whType === "clickhouse") { | ||
| const sql = CLICKHOUSE_HISTORY_SQL.replace("{days:UInt32}", String(Math.floor(Number(days)))).replace( | ||
| "{limit:UInt32}", | ||
| String(Math.floor(Number(limit))), | ||
| ) | ||
| return { sql, binds: [] } | ||
| } |
There was a problem hiding this comment.
Clamp days and limit before embedding them in the ClickHouse SQL.
This branch string-substitutes raw values into UInt32 placeholders. Because the finops handler accepts unvalidated inputs, NaN, Infinity, or negative numbers currently turn into invalid SQL instead of a controlled failure.
🛠️ Proposed fix
if (whType === "clickhouse") {
- const sql = CLICKHOUSE_HISTORY_SQL.replace("{days:UInt32}", String(Math.floor(Number(days)))).replace(
- "{limit:UInt32}",
- String(Math.floor(Number(limit))),
- )
+ const parsedDays = Number(days)
+ const parsedLimit = Number(limit)
+ const safeDays = Number.isFinite(parsedDays) ? Math.max(0, Math.floor(parsedDays)) : 7
+ const safeLimit = Number.isFinite(parsedLimit) ? Math.max(1, Math.floor(parsedLimit)) : 100
+ const sql = CLICKHOUSE_HISTORY_SQL.replace("{days:UInt32}", String(safeDays)).replace(
+ "{limit:UInt32}",
+ String(safeLimit),
+ )
return { sql, binds: [] }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (whType === "clickhouse") { | |
| const sql = CLICKHOUSE_HISTORY_SQL.replace("{days:UInt32}", String(Math.floor(Number(days)))).replace( | |
| "{limit:UInt32}", | |
| String(Math.floor(Number(limit))), | |
| ) | |
| return { sql, binds: [] } | |
| } | |
| if (whType === "clickhouse") { | |
| const parsedDays = Number(days) | |
| const parsedLimit = Number(limit) | |
| const safeDays = Number.isFinite(parsedDays) ? Math.max(0, Math.floor(parsedDays)) : 7 | |
| const safeLimit = Number.isFinite(parsedLimit) ? Math.max(1, Math.floor(parsedLimit)) : 100 | |
| const sql = CLICKHOUSE_HISTORY_SQL.replace("{days:UInt32}", String(safeDays)).replace( | |
| "{limit:UInt32}", | |
| String(safeLimit), | |
| ) | |
| return { sql, binds: [] } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/src/altimate/native/finops/query-history.ts` around lines
169 - 175, The ClickHouse branch is embedding unvalidated days and limit into
CLICKHOUSE_HISTORY_SQL; clamp and sanitize days and limit before substitution to
prevent NaN/Infinity/negative values producing invalid UInt32 SQL. Convert
inputs with Number(...), treat non-finite values as 0, clamp to the UInt32 range
(0 to 4294967295), and use Math.floor on the sanitized value before replacing
the "{days:UInt32}" and "{limit:UInt32}" tokens in the CLICKHOUSE_HISTORY_SQL
string inside the whType === "clickhouse" block so the returned sql always
contains valid UInt32 integers.
Comment on lines
+224
to
+235
| { | ||
| type: "clickhouse", | ||
| signals: ["CLICKHOUSE_HOST", "CLICKHOUSE_URL"], | ||
| configMap: { | ||
| host: "CLICKHOUSE_HOST", | ||
| port: "CLICKHOUSE_PORT", | ||
| database: ["CLICKHOUSE_DB", "CLICKHOUSE_DATABASE"], | ||
| user: ["CLICKHOUSE_USER", "CLICKHOUSE_USERNAME"], | ||
| password: "CLICKHOUSE_PASSWORD", | ||
| connection_string: "CLICKHOUSE_URL", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Teach the DATABASE_URL fallback about ClickHouse.
Adding CLICKHOUSE_HOST/CLICKHOUSE_URL here is only half the job: the generic DATABASE_URL classifier below still maps unknown schemes to Postgres. A project with only DATABASE_URL=clickhouse://... will therefore be surfaced as postgres, which breaks the new env-based discovery path for a common setup.
🛠️ Proposed fix
const schemeTypeMap: Record<string, string> = {
postgresql: "postgres",
postgres: "postgres",
mysql: "mysql",
mysql2: "mysql",
redshift: "redshift",
sqlite: "sqlite",
sqlite3: "sqlite",
mssql: "sqlserver",
sqlserver: "sqlserver",
oracle: "oracle",
duckdb: "duckdb",
databricks: "databricks",
+ clickhouse: "clickhouse",
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/src/altimate/tools/project-scan.ts` around lines 224 - 235,
The new ClickHouse entry (type: "clickhouse") was added but the generic
DATABASE_URL classifier still maps unknown schemes to Postgres, so
DATABASE_URL=clickhouse://... will be misclassified; update the DATABASE_URL
parsing logic in project-scan.ts to recognize the "clickhouse" scheme (and any
common aliases like "clickhouse+http"/"clickhouse+native" if used) and return
the "clickhouse" type instead of defaulting to "postgres", making the classifier
consistent with the configMap for the clickhouse entry.
Comment on lines
+25
to
+30
| function waitForPort(port: number, timeout = 30000): Promise<void> { | ||
| return new Promise((resolve, reject) => { | ||
| const start = Date.now() | ||
| const attempt = () => { | ||
| const sock = createConnection({ host: "127.0.0.1", port }) | ||
| sock.once("connect", () => { |
There was a problem hiding this comment.
Avoid hardcoding localhost in waitForPort.
At Line 29, readiness checks always target 127.0.0.1, but the suites allow configurable hosts (e.g., Lines 90, 432, 544, 622). This can fail when using non-local service hosts.
Suggested portability fix
-function waitForPort(port: number, timeout = 30000): Promise<void> {
+function waitForPort(host: string, port: number, timeout = 30000): Promise<void> {
return new Promise((resolve, reject) => {
const start = Date.now()
const attempt = () => {
- const sock = createConnection({ host: "127.0.0.1", port })
+ const sock = createConnection({ host, port })- await waitForPort(CH_PORT, 60000)
+ await waitForPort(CH_HOST, CH_PORT, 60000)(Apply the same call-site update to the other versioned suites.)
Also applies to: 90-91, 432-433, 544-545, 622-623
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/altimate/drivers-clickhouse-e2e.test.ts` around lines
25 - 30, The waitForPort helper currently hardcodes the host to "127.0.0.1" (see
function waitForPort and its use of createConnection), which breaks tests when
suites configure a different host; change waitForPort signature to accept an
optional host parameter (defaulting to "127.0.0.1") and use that host when
calling createConnection, then update all call sites (the tests around lines
referenced in the comment) to pass their configured host instead of relying on
localhost; apply the same change to the other versioned suites that copy this
helper so all readiness checks use the suite-configured host.
Comment on lines
+51
to
+69
| async function waitForDbReady( | ||
| connectFn: () => Promise<{ connector: any; testQuery: string }>, | ||
| timeout = 60000, | ||
| ): Promise<any> { | ||
| const start = Date.now() | ||
| let lastErr: any | ||
| while (Date.now() - start < timeout) { | ||
| try { | ||
| const { connector, testQuery } = await connectFn() | ||
| await connector.connect() | ||
| await connector.execute(testQuery) | ||
| return connector | ||
| } catch (e: any) { | ||
| lastErr = e | ||
| await new Promise((r) => setTimeout(r, 2000)) | ||
| } | ||
| } | ||
| throw new Error(`ClickHouse not ready after ${timeout}ms: ${lastErr?.message}`) | ||
| } |
There was a problem hiding this comment.
Close failed connectors during readiness retries.
In waitForDbReady, failed attempts keep created connectors open. Over multiple retries this can leak connections and make E2E startup flaky.
Suggested retry cleanup change
async function waitForDbReady(
connectFn: () => Promise<{ connector: any; testQuery: string }>,
timeout = 60000,
): Promise<any> {
const start = Date.now()
let lastErr: any
while (Date.now() - start < timeout) {
+ let connector: any
try {
- const { connector, testQuery } = await connectFn()
+ const ready = await connectFn()
+ connector = ready.connector
+ const testQuery = ready.testQuery
await connector.connect()
await connector.execute(testQuery)
return connector
} catch (e: any) {
lastErr = e
+ if (connector) {
+ try {
+ await connector.close()
+ } catch {}
+ }
await new Promise((r) => setTimeout(r, 2000))
}
}
throw new Error(`ClickHouse not ready after ${timeout}ms: ${lastErr?.message}`)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function waitForDbReady( | |
| connectFn: () => Promise<{ connector: any; testQuery: string }>, | |
| timeout = 60000, | |
| ): Promise<any> { | |
| const start = Date.now() | |
| let lastErr: any | |
| while (Date.now() - start < timeout) { | |
| try { | |
| const { connector, testQuery } = await connectFn() | |
| await connector.connect() | |
| await connector.execute(testQuery) | |
| return connector | |
| } catch (e: any) { | |
| lastErr = e | |
| await new Promise((r) => setTimeout(r, 2000)) | |
| } | |
| } | |
| throw new Error(`ClickHouse not ready after ${timeout}ms: ${lastErr?.message}`) | |
| } | |
| async function waitForDbReady( | |
| connectFn: () => Promise<{ connector: any; testQuery: string }>, | |
| timeout = 60000, | |
| ): Promise<any> { | |
| const start = Date.now() | |
| let lastErr: any | |
| while (Date.now() - start < timeout) { | |
| let connector: any | |
| try { | |
| const ready = await connectFn() | |
| connector = ready.connector | |
| const testQuery = ready.testQuery | |
| await connector.connect() | |
| await connector.execute(testQuery) | |
| return connector | |
| } catch (e: any) { | |
| lastErr = e | |
| if (connector) { | |
| try { | |
| await connector.close() | |
| } catch {} | |
| } | |
| await new Promise((r) => setTimeout(r, 2000)) | |
| } | |
| } | |
| throw new Error(`ClickHouse not ready after ${timeout}ms: ${lastErr?.message}`) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/altimate/drivers-clickhouse-e2e.test.ts` around lines
51 - 69, The waitForDbReady function leaks connectors on failed attempts: after
calling connectFn() you must ensure any connector returned is closed when the
attempt fails; modify waitForDbReady (around the connectFn/connector usage) to
call and await a safe close/disconnect on the returned connector (e.g., await
connector.close() or connector.disconnect() if present) inside the catch (or a
per-attempt finally) and guard the call with a typeof/exists check and its own
try/catch so close errors don’t mask the original error; keep using lastErr for
the final thrown message.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds first-class ClickHouse support as the 12th database driver in Altimate Code. Full integration across all 23 codebase touchpoints — driver, registry, config normalization, Docker discovery, environment variable detection, dbt profile mapping, dbt lineage dialect, query history (finops), publish peer dependencies, tool descriptions, tests, and documentation.
Key highlights:
@clickhouse/clientover HTTP(S), supporting ClickHouse server 23.3+ (all non-EOL versions)WITH...INSERTCTEs)system.query_logquery history template for finopspackages/drivers/ADDING_A_DRIVER.md(23-point checklist for future drivers)/add-database-driverto automate adding future database driversType of change
Issue for this PR
Closes #573
How did you verify your code works?
Checklist
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests