feat: add native Trino driver#795
Merged
Merged
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds native Trino support: new TypeScript Trino driver (trino-client), config aliasing and normalization, dbt/Docker/env/URL discovery, driver registry/export wiring, tests, docs, publishing updates, and defensive tooling improvements. ChangesTrino Warehouse Driver Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
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. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 23 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/docs/data-engineering/tools/warehouse-tools.md">
<violation number="1" location="docs/docs/data-engineering/tools/warehouse-tools.md:57">
P3: Docker discovery is documented inconsistently across sections; the summary table drops ClickHouse/MongoDB while the command docs still list them.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -54,7 +54,7 @@ env_bigquery | bigquery | GOOGLE_APPLICATION_CREDENTIALS | |||
| | **dbt project** | Walks up directories for `dbt_project.yml`, reads name/profile | | |||
There was a problem hiding this comment.
P3: Docker discovery is documented inconsistently across sections; the summary table drops ClickHouse/MongoDB while the command docs still list them.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/docs/data-engineering/tools/warehouse-tools.md, line 57:
<comment>Docker discovery is documented inconsistently across sections; the summary table drops ClickHouse/MongoDB while the command docs still list them.</comment>
<file context>
@@ -54,7 +54,7 @@ env_bigquery | bigquery | GOOGLE_APPLICATION_CREDENTIALS
| **dbt manifest** | Parses `target/manifest.json` for model/source/test counts |
| **dbt profiles** | Searches for `profiles.yml`: `DBT_PROFILES_DIR` env var → project root → `<home>/.dbt/profiles.yml` |
-| **Docker DBs** | Bridge call to discover running PostgreSQL/MySQL/MSSQL containers |
+| **Docker DBs** | Bridge call to discover running PostgreSQL/MySQL/MSSQL/Trino containers |
| **Existing connections** | Bridge call to list already-configured warehouses |
| **Environment variables** | Scans `process.env` for warehouse signals (see table below) |
</file context>
Suggested change
| | **dbt project** | Walks up directories for `dbt_project.yml`, reads name/profile | | |
| | **Docker DBs** | Bridge call to discover running PostgreSQL/MySQL/MSSQL/Trino/ClickHouse/MongoDB containers | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/src/altimate/native/connections/dbt-profiles.ts (1)
56-64:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
env_varregex only matches single-quoted args; dbt also accepts double quotes.Both
{{ env_var('DBT_USER') }}and{{ env_var("DBT_USER") }}are valid in dbt's Jinja, and double-quoted forms are very common in real profiles. The current regex only matches single quotes, so any profile using double quotes will fall through unresolved (and the literal{{ env_var("…") }}ends up as the field value).🛠️ Suggested fix — accept either quote style for both name and default
function resolveEnvVars(value: unknown): unknown { if (typeof value !== "string") return value return value.replace( - /\{\{\s*env_var\s*\(\s*'([^']+)'\s*(?:,\s*'([^']*)'\s*)?\)\s*\}\}/g, - (_match, envName: string, defaultValue?: string) => { - return process.env[envName] ?? defaultValue ?? "" + /\{\{\s*env_var\s*\(\s*(['"])([^'"]+)\1\s*(?:,\s*(['"])([^'"]*)\3\s*)?\)\s*\}\}/g, + (_match, _q1, envName: string, _q2, defaultValue?: string) => { + return process.env[envName] ?? defaultValue ?? "" }, ) }🤖 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 `@packages/opencode/src/altimate/native/connections/dbt-profiles.ts` around lines 56 - 64, The resolveEnvVars function's regex only matches single-quoted args so double-quoted env_var forms are left unresolved; update the regex in resolveEnvVars to accept either single or double quotes (use a capture for the opening quote like (['"]) and a backreference \1 for the closing quote) for both the env name and the optional default, then use the captured groups for envName and defaultValue in the replacement so both {{ env_var('NAME') }}, {{ env_var("NAME") }}, and defaults with either quote style are handled.
🧹 Nitpick comments (2)
packages/opencode/test/tool/project-scan.test.ts (1)
567-602: 💤 Low valueLGTM — Trino env-var and
DATABASE_URLdetection tests look correct.The
TRINO_HOSTtest covers field mapping includingcatalog,schema, and password masking. TheDATABASE_URLscheme loop correctly asserts all four Trino/Presto URL schemes map totype === "trino".One minor note: the
for...ofscheme loop at line 591 combines four scheme assertions in a singletest(). If one scheme fails, Bun's error output will report the test name but not automatically identify the failing scheme. Consider splitting into separate test cases or addingschemeto theexpectfailure message if you ever find the test brittle in CI.🤖 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 `@packages/opencode/test/tool/project-scan.test.ts` around lines 567 - 602, The test "detects Trino via DATABASE_URL with trino schemes" embeds a for...of loop over schemes which hides which scheme fails; update the test to run each scheme as its own subtest by using test.each or describe.each so each scheme is reported individually, e.g., replace the for...of loop with test.each([...]) or describe.each([...]) and inside call clearWarehouseEnvVars(), set process.env.DATABASE_URL, await detectEnvVars(), and assert trino fields (including scheme in the test title) to get per-scheme failure diagnostics.packages/drivers/src/trino.ts (1)
180-186: 💤 Low valueStatic-analysis SQL-injection hint is a false positive — but worth a short comment.
OpenGrep flags the template-literal concatenation in
listSchemas/listTables/describeTable. Catalog goes throughquoteIdent(doubles") and schema/table go throughescapeStringLiteral(doubles'), which are the correct primitives for Trino —information_schemaqueries can't accept catalog as a bind parameter, and Trino's standard string-literal escape is''. Consider adding a short inline comment explaining why bind parameters aren't used here, so future readers (and the linter) don't try to "fix" this:+ // information_schema queries are catalog-qualified at the FROM clause, + // which Trino cannot bind. We escape via quoteIdent / escapeStringLiteral. const result = await connector.execute( `SELECT table_name, table_type FROM ${quoteIdent(catalog)}.information_schema.tables WHERE table_schema = '${escapeStringLiteral(schema)}' ORDER BY table_name`, 10000, )Also applies to: 196-202, 219-226
🤖 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 `@packages/drivers/src/trino.ts` around lines 180 - 186, Add a short inline comment in the SQL-building sites (functions listSchemas, listTables, describeTable) explaining that the template-literal concatenation is intentional and safe because identifiers are sanitized with quoteIdent (which doubles ") and values use escapeStringLiteral (which doubles '), and that Trino does not support using bind parameters for catalog/schema/table identifiers in information_schema queries; this will silence static-analysis false positives and guide future readers not to replace these with parameterized binds.
🤖 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.
Inline comments:
In `@docs/docs/configure/warehouses.md`:
- Around line 518-519: The Docker auto-discovery list in the table row that
currently reads "Docker containers | Finds running PostgreSQL, MySQL, SQL
Server, Trino, and ClickHouse containers" is missing MongoDB and MariaDB; update
that table row to include "MongoDB" and "MariaDB" so it matches the documented
discovery behavior elsewhere in the PR (look for the table row text "Docker
containers | Finds running PostgreSQL, MySQL, SQL Server, Trino, and ClickHouse
containers" to locate the exact spot to edit).
In `@docs/docs/data-engineering/tools/warehouse-tools.md`:
- Line 57: The Docker DBs detection-method table row ("| **Docker DBs** | Bridge
call to discover running PostgreSQL/MySQL/MSSQL/Trino containers |") is
inconsistent with the warehouse_discover section; update that table row to list
the same databases (add ClickHouse, MongoDB, and MariaDB alongside
PostgreSQL/MySQL/MSSQL/Trino) so the documentation matches the
warehouse_discover section and ensures parity between the two descriptions.
In `@docs/docs/drivers.md`:
- Around line 126-133: Add a blank line between the "### Trino" heading and the
following Markdown table so the table does not immediately follow the heading
(this will satisfy markdownlint MD058); locate the "### Trino" heading and the
table block listing auth methods and insert one empty line between them.
In `@packages/drivers/src/trino.ts`:
- Around line 82-91: The current try-catch around the dynamic import and the
export validation masks export-shape errors as "not installed"; split the logic
so the import("trino-client") is inside its own try-catch and only that catch
throws "Trino driver not installed...", then perform the validation of
Trino/BasicAuth/Trino.create outside that catch and, if validation fails, throw
a distinct error like "Trino.create export not found in trino-client" (or
include the actual module shape) so missing/renamed exports are reported
accurately; reference the Trino and BasicAuth bindings and the
import("trino-client") call when updating the control flow.
In `@packages/opencode/src/altimate/native/connections/registry.ts`:
- Around line 133-134: When constructing/normalizing connection data for display
in warehouse.list, ensure Trino connections use catalog as a fallback for
database: detect the registry entry for "trino" (the trino key in registry.ts)
and if a connection's config has no config.database but has config.catalog, set
config.database = config.catalog before rendering/storing the connection object;
this ensures warehouse.list shows a meaningful Database value for Trino.
---
Outside diff comments:
In `@packages/opencode/src/altimate/native/connections/dbt-profiles.ts`:
- Around line 56-64: The resolveEnvVars function's regex only matches
single-quoted args so double-quoted env_var forms are left unresolved; update
the regex in resolveEnvVars to accept either single or double quotes (use a
capture for the opening quote like (['"]) and a backreference \1 for the closing
quote) for both the env name and the optional default, then use the captured
groups for envName and defaultValue in the replacement so both {{
env_var('NAME') }}, {{ env_var("NAME") }}, and defaults with either quote style
are handled.
---
Nitpick comments:
In `@packages/drivers/src/trino.ts`:
- Around line 180-186: Add a short inline comment in the SQL-building sites
(functions listSchemas, listTables, describeTable) explaining that the
template-literal concatenation is intentional and safe because identifiers are
sanitized with quoteIdent (which doubles ") and values use escapeStringLiteral
(which doubles '), and that Trino does not support using bind parameters for
catalog/schema/table identifiers in information_schema queries; this will
silence static-analysis false positives and guide future readers not to replace
these with parameterized binds.
In `@packages/opencode/test/tool/project-scan.test.ts`:
- Around line 567-602: The test "detects Trino via DATABASE_URL with trino
schemes" embeds a for...of loop over schemes which hides which scheme fails;
update the test to run each scheme as its own subtest by using test.each or
describe.each so each scheme is reported individually, e.g., replace the
for...of loop with test.each([...]) or describe.each([...]) and inside call
clearWarehouseEnvVars(), set process.env.DATABASE_URL, await detectEnvVars(),
and assert trino fields (including scheme in the test title) to get per-scheme
failure diagnostics.
🪄 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: a24a4e41-d302-4fb1-b333-74272c0e5444
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
README.mddocs/docs/configure/warehouses.mddocs/docs/data-engineering/tools/warehouse-tools.mddocs/docs/drivers.mddocs/docs/getting-started/index.mdpackages/drivers/package.jsonpackages/drivers/src/index.tspackages/drivers/src/normalize.tspackages/drivers/src/trino.tspackages/drivers/test/trino-unit.test.tspackages/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/register.tspackages/opencode/src/altimate/native/connections/registry.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/connections.test.tspackages/opencode/test/altimate/driver-normalize.test.tspackages/opencode/test/altimate/tools/sql-explain.test.tspackages/opencode/test/tool/project-scan.test.ts
Comment on lines
518
to
519
| | Docker containers | Finds running PostgreSQL, MySQL, SQL Server, Trino, and ClickHouse containers | | ||
| | Environment variables | Scans for `SNOWFLAKE_ACCOUNT`, `PGHOST`, `DATABRICKS_HOST`, etc. | |
There was a problem hiding this comment.
Auto-discovery Docker list is incomplete for current behavior/docs.
This line omits MongoDB (and MariaDB), which are documented as discoverable elsewhere in this PR.
Suggested patch
-| Docker containers | Finds running PostgreSQL, MySQL, SQL Server, Trino, and ClickHouse containers |
+| Docker containers | Finds running PostgreSQL, MySQL, MariaDB, SQL Server, Trino, ClickHouse, and MongoDB containers |📝 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
| | Docker containers | Finds running PostgreSQL, MySQL, SQL Server, Trino, and ClickHouse containers | | |
| | Environment variables | Scans for `SNOWFLAKE_ACCOUNT`, `PGHOST`, `DATABRICKS_HOST`, etc. | | |
| | Docker containers | Finds running PostgreSQL, MySQL, MariaDB, SQL Server, Trino, ClickHouse, and MongoDB containers | | |
| | Environment variables | Scans for `SNOWFLAKE_ACCOUNT`, `PGHOST`, `DATABRICKS_HOST`, etc. | |
🤖 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 `@docs/docs/configure/warehouses.md` around lines 518 - 519, The Docker
auto-discovery list in the table row that currently reads "Docker containers |
Finds running PostgreSQL, MySQL, SQL Server, Trino, and ClickHouse containers"
is missing MongoDB and MariaDB; update that table row to include "MongoDB" and
"MariaDB" so it matches the documented discovery behavior elsewhere in the PR
(look for the table row text "Docker containers | Finds running PostgreSQL,
MySQL, SQL Server, Trino, and ClickHouse containers" to locate the exact spot to
edit).
| | **dbt manifest** | Parses `target/manifest.json` for model/source/test counts | | ||
| | **dbt profiles** | Searches for `profiles.yml`: `DBT_PROFILES_DIR` env var → project root → `<home>/.dbt/profiles.yml` | | ||
| | **Docker DBs** | Bridge call to discover running PostgreSQL/MySQL/MSSQL containers | | ||
| | **Docker DBs** | Bridge call to discover running PostgreSQL/MySQL/MSSQL/Trino containers | |
There was a problem hiding this comment.
Detection-method table is now inconsistent with the warehouse_discover section.
This row omits ClickHouse/MongoDB (and MariaDB) even though the same page documents them as discovered containers.
Suggested wording update
-| **Docker DBs** | Bridge call to discover running PostgreSQL/MySQL/MSSQL/Trino containers |
+| **Docker DBs** | Bridge call to discover running PostgreSQL/MySQL/MariaDB/MSSQL/Trino/ClickHouse/MongoDB containers |📝 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
| | **Docker DBs** | Bridge call to discover running PostgreSQL/MySQL/MSSQL/Trino containers | | |
| | **Docker DBs** | Bridge call to discover running PostgreSQL/MySQL/MariaDB/MSSQL/Trino/ClickHouse/MongoDB containers | |
🤖 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 `@docs/docs/data-engineering/tools/warehouse-tools.md` at line 57, The Docker
DBs detection-method table row ("| **Docker DBs** | Bridge call to discover
running PostgreSQL/MySQL/MSSQL/Trino containers |") is inconsistent with the
warehouse_discover section; update that table row to list the same databases
(add ClickHouse, MongoDB, and MariaDB alongside PostgreSQL/MySQL/MSSQL/Trino) so
the documentation matches the warehouse_discover section and ensures parity
between the two descriptions.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/src/altimate/tools/sql-analyze.ts (1)
45-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFailure detection should be presence-based, not truthy-based.
At Line 45,
!!result.errormisclassifieserror: ""as success; the same truthy check also affects title/metadata at Line 65 and Line 72.Suggested fix
- const isRealFailure = !!result.error + const isRealFailure = result.error !== null && result.error !== undefined + const normalizedError = isRealFailure ? String(result.error ?? "").trim() || "Analysis failed." : undefined @@ - let output = formatAnalysis(result) + let output = isRealFailure ? `Analysis failed: ${normalizedError}` : formatAnalysis(result) @@ - title: `Analyze: ${result.error ? "ERROR" : `${result.issue_count ?? 0} issue${(result.issue_count ?? 0) !== 1 ? "s" : ""}`} [${result.confidence ?? "unknown"}]`, + title: `Analyze: ${isRealFailure ? "ERROR" : `${result.issue_count ?? 0} issue${(result.issue_count ?? 0) !== 1 ? "s" : ""}`} [${result.confidence ?? "unknown"}]`, @@ - ...(result.error && { error: result.error }), + ...(isRealFailure && { error: normalizedError }),Also applies to: 53-53, 65-73
🤖 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 `@packages/opencode/src/altimate/tools/sql-analyze.ts` at line 45, The code currently treats errors with a truthy check (e.g., isRealFailure = !!result.error) which misclassifies empty-string errors as absent; update all checks that inspect result.error (including where isRealFailure is set and where title/metadata are built around result.error at the other occurrences) to use presence-based checks instead — for example, replace !!result.error with an explicit presence test such as result.error !== undefined && result.error !== null && result.error !== '' (or a small helper like hasError(result) that returns the same boolean) and apply the same presence check wherever result.error is used to decide titles/metadata (the assignments that reference result.error at the spots around isRealFailure, and the title/metadata construction lines).
🤖 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.
Inline comments:
In `@packages/opencode/src/altimate/tools/lineage-check.ts`:
- Around line 36-38: The check "if (result.error)" treats an empty string as
no-error; update both occurrences (the one that returns lineageError at the top
and the block around lines 65-69) to explicitly treat present-but-empty errors
as failures by checking for non-empty string (e.g., if (result.error !==
undefined && result.error !== null && result.error !== '') or if (typeof
result.error === 'string' && result.error.length > 0)) and call
lineageError(result.error) when that condition is met; make the same change at
the second occurrence so malformed/failed dispatcher responses with error: ""
are handled as errors.
In `@packages/opencode/src/altimate/tools/schema-inspect.ts`:
- Around line 28-31: The failure gating currently treats a blank-string error as
falsy by checking responseError, so change the condition to explicitly detect
when the dispatcher returned an error (e.g., use result.error !== undefined &&
result.error !== null or result.error != null) instead of relying on the
truthiness of normalizeError's output; update the condition in the block that
returns schemaError (the one using responseError/normalizeError and
result.success) and make the identical fix in the later block around lines 71-75
so both checks use explicit presence of result.error while still passing the
normalized error (normalizeError) into schemaError.
---
Outside diff comments:
In `@packages/opencode/src/altimate/tools/sql-analyze.ts`:
- Line 45: The code currently treats errors with a truthy check (e.g.,
isRealFailure = !!result.error) which misclassifies empty-string errors as
absent; update all checks that inspect result.error (including where
isRealFailure is set and where title/metadata are built around result.error at
the other occurrences) to use presence-based checks instead — for example,
replace !!result.error with an explicit presence test such as result.error !==
undefined && result.error !== null && result.error !== '' (or a small helper
like hasError(result) that returns the same boolean) and apply the same presence
check wherever result.error is used to decide titles/metadata (the assignments
that reference result.error at the spots around isRealFailure, and the
title/metadata construction lines).
🪄 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: 9e2255d9-3bff-4449-8772-b7553a3e5adf
📒 Files selected for processing (4)
packages/opencode/src/altimate/tools/altimate-core-column-lineage.tspackages/opencode/src/altimate/tools/lineage-check.tspackages/opencode/src/altimate/tools/schema-inspect.tspackages/opencode/src/altimate/tools/sql-analyze.ts
Contributor
|
@cerebrixos Thanks for the contribution. We will review it soon |
Contributor
|
@cerebrixos, can you please fix the failing tests? |
Contributor
Author
|
@dev-punia-altimate @anandgupta42 any update on this? seems its all good now? |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/opencode/src/altimate/tools/altimate-core-column-lineage.ts`:
- Around line 50-59: Extract the duplicated helpers isRecord and normalizeError
into a shared module (e.g., create a new file like altimate/tools/utils.ts or
altimate/utils/response-normalization.ts) and replace the local implementations
in altimate-core-column-lineage.ts (and other tools: sql-analyze.ts,
lineage-check.ts, schema-inspect.ts) with imports from that module; ensure the
new module exports isRecord and normalizeError and update each file to import {
isRecord, normalizeError } from the new module, run type-checks and tests to
confirm no behavioral changes.
🪄 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: 5c845045-cd8f-4605-a80f-e0cdc3b35d48
📒 Files selected for processing (4)
packages/opencode/src/altimate/tools/altimate-core-column-lineage.tspackages/opencode/src/altimate/tools/schema-inspect.tspackages/opencode/src/altimate/tools/sql-analyze.tspackages/opencode/test/altimate/tool-response-normalization.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/src/altimate/tools/schema-inspect.ts
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/altimate/tools/sql-analyze.ts">
<violation number="1" location="packages/opencode/src/altimate/tools/sql-analyze.ts:55">
P2: Failure rendering is gated on `error` but failure detection uses `isRealFailure`, causing inconsistent ERROR metadata with success-style output when `success === false` but no error string is present.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/opencode/src/altimate/tools/sql-analyze.ts`:
- Around line 106-110: normalizeError currently returns "[object Object]" for
non-Error objects; update it to extract a plain object's message before
stringifying: inside normalizeError (function name), after the existing checks,
add a branch that if value is a non-null object and has a string "message"
property (e.g. typeof value === "object" && "message" in value && typeof (value
as any).message === "string") return that message; otherwise attempt a safe
JSON.stringify of the object inside a try/catch (to capture output/metadata
contents) and return the stringified result, falling back to String(value) if
stringify fails.
🪄 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: 9760a5f9-a5c6-4ccf-b7a5-47c66f7c9de3
📒 Files selected for processing (2)
packages/opencode/src/altimate/tools/sql-analyze.tspackages/opencode/test/altimate/tool-response-normalization.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/test/altimate/tool-response-normalization.test.ts
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/altimate/tools/sql-analyze.ts">
<violation number="1" location="packages/opencode/src/altimate/tools/sql-analyze.ts:46">
P1: Blank/whitespace error fields force false failure states, potentially hiding valid analysis results</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/altimate/tools/response-normalization.ts">
<violation number="1" location="packages/opencode/src/altimate/tools/response-normalization.ts:12">
P1: Potential sensitive data exposure: `normalizeError` now JSON-serializes arbitrary record objects, which can expose sensitive fields (e.g., auth headers, connection config, tokens) into user-facing tool output and metadata. Previously these objects collapsed to `[object Object]`, providing an implicit sanitization barrier.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
❌ Tests — Failures DetectedTypeScript — 15 failure(s)
Next StepPlease address the failing cases above and re-run verification. cc @cerebrixos |
- `trino.ts`: split dynamic import from export validation so a present-but-broken `trino-client` reports a distinct version error instead of a misleading "not installed. Run: npm install" message. - `registry.ts`: `warehouse.list` now falls back to `catalog` for Trino connections so the Database column is not blank. - docs: align Docker auto-discovery lists in `warehouses.md` and `warehouse-tools.md` with the actual `IMAGE_MAP` (PostgreSQL/MySQL/MariaDB/MSSQL/Oracle/ClickHouse/Trino; MongoDB is not discovered, contrary to the reviewer suggestion). - docs: add blank line before the Trino auth table in `drivers.md` (markdownlint MD058). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
dev-punia-altimate
left a comment
dev-punia-altimate
left a comment
There was a problem hiding this comment.
Multi-Persona Review — Verdict: ship
The PR adds a well-documented native Trino driver with comprehensive test coverage and integrates it cleanly across the codebase. All changes are focused, non-breaking, and improve support for 13 warehouses. No critical or high-severity issues were found.
14/14 agents completed · 256s · 15 findings (0 critical, 2 high, 8 medium)
High
- [persona_security] User-supplied catalog, schema, or table names are directly interpolated into SQL queries without parameterization or sanitization in Trino driver, enabling SQL injection via malicious metadata inputs. →
packages/drivers/src/trino.ts:145- 💡 Use parameterized queries or escape identifiers via Trino's built-in identifier quoting (e.g.,
"identifier") instead of string concatenation. Validate all identifiers against a whitelist of allowed characters.
- 💡 Use parameterized queries or escape identifiers via Trino's built-in identifier quoting (e.g.,
- [persona_security] Trino driver accepts arbitrary extra headers and session properties from user input without validation, potentially enabling HTTP request smuggling or server-side request forgery (SSRF) via malicious headers. →
packages/drivers/src/trino.ts:89- 💡 Sanitize and validate all user-provided headers and session properties against a strict allowlist of permitted keys and values. Block headers like 'Host', 'Content-Length', 'X-Forwarded-For', and non-standard session properties.
Medium
- [persona_tech-lead] Trino driver implementation in packages/drivers/src/trino.ts directly handles SQL execution and connection logic without abstracting through a standardized driver interface used by other warehouses (e.g., PostgreSQL, Snowflake). This creates a maintenance burden and violates the principle of uniform driver abstraction. →
packages/drivers/src/trino.ts:1- 💡 Refactor Trino driver to conform to the shared Driver interface defined in packages/drivers/src/index.ts. Extract connection logic, query execution, and schema introspection into methods matching the expected signature of other drivers to ensure consistent usage across the system.
- [persona_tech-lead] The new response-normalization.ts file implements error unwrapping and standardization logic that duplicates functionality already present in other tool handlers (e.g., sql-analyze.ts, schema-inspect.ts). This leads to inconsistent behavior and code duplication. →
packages/opencode/src/altimate/tools/response-normalization.ts:1- 💡 Consolidate response normalization logic into a shared utility (e.g., packages/opencode/src/altimate/utils/response-helpers.ts) and import it across all tool handlers instead of duplicating logic in multiple files.
- [persona_cto] Adding a native Trino driver is a strategic expansion of warehouse support, but the PR bundles significant unrelated changes: response normalization helpers, error handling refactors, and lineage tool modifications. These are high-impact cross-cutting changes that affect multiple tools and should be separated to reduce risk and improve reviewability.
- 💡 Split into two PRs: 1) Native Trino driver + config/integration changes; 2) Cross-tool response normalization and error handling improvements as a standalone refactoring.
- [persona_product-manager] PR adds native Trino driver and integrates it across the platform, but does not mention whether Trino support is tied to any specific customer tier or licensing requirement. If Trino is a premium feature, this is not communicated to users in docs or UI.
- 💡 Clarify in documentation or UI whether Trino access requires a paid plan, and if so, update the docs and onboarding flows to reflect this.
- [persona_product-manager] The PR introduces new environment variables (TRINO_*) and config aliases (e.g., server → connection_string), but does not provide clear migration guidance for users currently using the old PostgreSQL-compatible Trino mapping.
- 💡 Include a migration note in the release notes or docs advising users to update their dbt profiles and connection strings to use the new native Trino format.
- [persona_security] Error responses from Trino queries may expose internal stack traces or connection details in debug mode, leaking infrastructure information to unauthorized users. →
packages/opencode/src/altimate/tools/response-normalization.ts:12- 💡 Ensure all error responses are sanitized before returning to clients. Never expose raw error objects, stack traces, or internal connection strings. Use generic failure messages like 'Analysis failed.' consistently.
- [persona_devops] New Trino driver introduces a dependency on 'trino-client' package without verifying version compatibility or fallback behavior in production.
- 💡 Add a version lock in package.json and include a runtime check for trino-client version on driver initialization with a clear error message if incompatible.
- [persona_devops] New environment variables (TRINO_*) are introduced but not documented in deployment runbooks or environment templates.
- 💡 Update deployment documentation and CI/CD environment templates to include TRINO_* variables with examples for Basic Auth and Bearer Token configurations.
Low
- [persona_tech-lead] The Trino driver uses
access_tokenas a config key, but other drivers usetokenfor bearer auth (e.g., in normalize.ts). This inconsistency in naming conventions may confuse users and increase configuration errors. →packages/drivers/src/normalize.ts:105- 💡 Standardize the config key name across all drivers. Prefer
tokenfor bearer auth unless there's a compelling reason to differentiate. Update Trino driver config mapping to usetokenand document the alias in the migration guide.
- 💡 Standardize the config key name across all drivers. Prefer
- [persona_cto] The PR introduces a new dependency (
trino-client) without clear justification for why it's preferred over existing HTTP-based SQL drivers or community-maintained alternatives. The codebase already handles multiple SQL dialects via abstraction layers.- 💡 Add a brief rationale in the PR description or README about why
trino-clientwas chosen over a generic HTTP client + SQL parser approach, especially given Trino's HTTP API is well-documented.
- 💡 Add a brief rationale in the PR description or README about why
- [persona_cto] The Trino driver implementation includes extensive logic for connection normalization, auth handling, and introspection — but these patterns are already established in other drivers (e.g., PostgreSQL, Snowflake). Risk of duplication and inconsistency.
- 💡 Refactor common connection normalization and auth logic into a shared
DriverBaseorHttpSqlDriverclass to reduce duplication and ensure consistent behavior across drivers.
- 💡 Refactor common connection normalization and auth logic into a shared
- [persona_product-manager] While Trino configuration and auth examples are documented, there is no guidance on how to troubleshoot common failures (e.g., authentication errors, connection timeouts, catalog not found).
- 💡 Add a 'Troubleshooting Trino Connections' section in the warehouses.md or drivers.md docs with common error messages and resolutions.
- [persona_devops] No new metrics or alerts are defined for Trino driver failures, connection timeouts, or query errors.
- 💡 Add custom metrics for Trino connection attempts, success/failure rates, and query latency; create alert on >5% failure rate over 5 minutes.
Multi-Persona Review · vllm:qwen3-next-80b (waves) + vllm-fallback (synth) ·
Applies the minor/robustness fixes from the multi-model code review. `trino.ts`: - `cleanSql` no longer treats `\'` as an escape — Trino uses `''` doubling, so a literal ending in a backslash is parsed correctly. - `execute()` clamps the limit (`Math.floor(Number(...))`, finite + positive) so a non-numeric/negative value can never produce `LIMIT NaN`/`LIMIT -5`. - `hasLimit` now also matches `FETCH NEXT` (not just `FETCH FIRST`); documents the subquery-LIMIT heuristic limitation. - `connect()` throws when both `password` (Basic) and `access_token` (Bearer) are configured, instead of silently sending both. - `serverUrl` falls back to the protocol default port when `port` is non-numeric. - `escapeStringLiteral` is null-safe; comments clarify the introspection-escaping and stateless-`close()` rationale. `response-normalization.ts`: - `normalizeError` surfaces object `.error`/`.detail` string fields (without stringifying the whole object) and treats falsy `false`/`0` as "no error". `altimate-core-column-lineage.ts` / `lineage-check.ts`: - Route `data.error` through `normalizeError` in `formatColumnLineage`. - Fail when the inner `data.success === false`, matching `sql_analyze`'s two-layer check (column-lineage and lineage-check). Adds unit tests for auth conflict, `FETCH NEXT`, `noLimit`, non-numeric limit/port, inner `data.success=false`, and object-error non-leakage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Contributor
|
@cerebrixos appolgies for the delay. We were setting up an internal stack to have a testing environment for Trino. It's setup now and I will be releasing this today. Thanks for your contribution! |
Addresses multi-persona review feedback on the Trino driver docs: - Add a migration note covering the dbt `trino` adapter change (was mapped to the PostgreSQL driver, now native) and the `database → catalog` / `token → access_token` aliases. - Add a short Trino troubleshooting list (catalog required, auth conflict, protocol/port). - Correct the auto-discovery list: Docker discovery does not detect MongoDB (not in the image map), so drop it to match the actual behavior.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
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
4 participants
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.
Summary
trino-client.Details
The previous dbt profile mapping treated
trinoas PostgreSQL-compatible. This adds a first-class Trino connector instead, with support for:information_schemaTests
cd packages/drivers && bun test test/trino-unit.test.tscd packages/opencode && bun test test/altimate/driver-normalize.test.ts test/altimate/connections.test.ts test/tool/project-scan.test.ts test/altimate/tools/sql-explain.test.tscd packages/opencode && bun run typecheckNote:
bunx tsc -p packages/drivers/tsconfig.json --noEmitstill fails on the existingbun:sqlitetype resolution issue inpackages/drivers/src/sqlite.ts, unrelated to this Trino change.Summary by cubic
Adds a native Trino driver and integrates it across discovery, dbt, SQL tools, and docs, expanding support to 13 warehouses. Also standardizes tool error handling and prevents sensitive details from appearing in outputs.
New Features
trino-clientover HTTP/HTTPS with Basic or Bearer auth (mutually exclusive), extra headers/session/extra credentials, catalog/schema defaults; clamps SELECT limits, detects truncation, recognizesFETCH NEXT, and supportsEXPLAIN/EXPLAIN ANALYZE(query history not surfaced).connection_string, database/dbname →catalog, token →access_token); dbttrinonow maps to the native driver; Docker discovery matches Trino images; env var detection forTRINO_*andDATABASE_URL(trino,trino+http(s),presto); docs updated with a migration note (dbt mapping change anddatabase→catalog,token→access_token) and troubleshooting (catalog required for introspection, auth conflict, protocol/port defaults).Bug Fixes
sql_analyze,schema_inspect,lineage_check, and column lineage with shared helpers: unwrap dispatcher envelopes, treat falsy/object errors via message/detail fields, avoid exposing object details, standardize titles/metadata; treatsuccess=falseas failure with “Analysis failed.”; lineage tools also fail on innerdata.success=false.trino-clientis present but incompatible; clamps non-numeric/negative limits and detectsFETCH NEXT; falls back to protocol-default port whenportis invalid; throws if both password and access token are set;warehouse.listshows Database fromcatalogfor Trino.Written for commit 68702ed. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Tests
UX