feat: add native Trino driver#795
Open
cerebrixos wants to merge 3 commits intoAltimateAI:mainfrom
Open
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
Comment on lines
+126
to
+133
| ### Trino | ||
| | Method | Config Fields | | ||
| |--------|--------------| | ||
| | No auth | `host`, `port`, `catalog`, `schema`, `user` | | ||
| | Basic | `host`, `port`, `catalog`, `schema`, `user`, `password` | | ||
| | Bearer token | `host`, `port`, `catalog`, `schema`, `access_token` | | ||
| | Connection String | `connection_string: "https://trino.example.com:8443"` | | ||
|
|
There was a problem hiding this comment.
Add a blank line before the Trino auth table to satisfy markdownlint (MD058).
The table starts immediately under the heading, which triggers the configured lint warning.
Suggested patch
### Trino
+
| Method | Config Fields |
|--------|--------------|
| No auth | `host`, `port`, `catalog`, `schema`, `user` |🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 127-127: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 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/drivers.md` around lines 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.
Comment on lines
+82
to
+91
| try { | ||
| const mod = await import("trino-client") | ||
| Trino = mod.Trino ?? mod.default?.Trino ?? mod.default | ||
| BasicAuth = mod.BasicAuth ?? mod.default?.BasicAuth | ||
| if (!Trino?.create) { | ||
| throw new Error("Trino.create export not found in trino-client") | ||
| } | ||
| } catch { | ||
| throw new Error("Trino driver not installed. Run: npm install trino-client") | ||
| } |
There was a problem hiding this comment.
Misleading error when trino-client is installed but missing the expected exports.
The try wraps both the dynamic import("trino-client") and the if (!Trino?.create) throw new Error("Trino.create export not found …") validation. The inner throw is caught by the same catch on line 89 and rewritten to "Trino driver not installed. Run: npm install trino-client". If a user has trino-client installed but its shape changes (e.g., a major-version bump that re-exports differently), they'll be told to run a npm install that won't fix anything.
🛠️ Suggested fix — only treat actual import failure as "not installed"
export async function connect(config: ConnectionConfig): Promise<Connector> {
let Trino: any
let BasicAuth: any
+ let mod: any
try {
- const mod = await import("trino-client")
- Trino = mod.Trino ?? mod.default?.Trino ?? mod.default
- BasicAuth = mod.BasicAuth ?? mod.default?.BasicAuth
- if (!Trino?.create) {
- throw new Error("Trino.create export not found in trino-client")
- }
+ mod = await import("trino-client")
} catch {
throw new Error("Trino driver not installed. Run: npm install trino-client")
}
+ Trino = mod.Trino ?? mod.default?.Trino ?? mod.default
+ BasicAuth = mod.BasicAuth ?? mod.default?.BasicAuth
+ if (!Trino?.create) {
+ throw new Error("Trino.create export not found in trino-client — check the installed package version")
+ }🤖 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 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.
Comment on lines
+133
to
134
| trino: "@altimateai/drivers/trino", | ||
| } |
There was a problem hiding this comment.
Trino entries may show blank “Database” in warehouse.list.
Trino uses catalog as canonical, but list output reads only config.database. Add a Trino fallback so discovered/saved Trino connections render meaningful context.
Suggested patch
for (const [name, config] of configs) {
+ const type = typeof config.type === "string" ? config.type.toLowerCase() : ""
warehouses.push({
name,
- type: config.type,
- database: config.database as string | undefined,
+ type: config.type,
+ database:
+ type === "trino"
+ ? ((config.catalog as string | undefined) ?? (config.database as string | undefined))
+ : (config.database as string | undefined),
})
}🤖 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/registry.ts` around lines
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.
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
Comment on lines
36
to
38
| if (result.error) { | ||
| return { | ||
| title: "Lineage: ERROR", | ||
| metadata: { success: false, error: result.error }, | ||
| output: `Error: ${result.error}`, | ||
| } | ||
| return lineageError(result.error) | ||
| } |
There was a problem hiding this comment.
Handle present-but-empty dispatcher errors as failures.
At Line 36, if (result.error) skips the error path for error: "", so malformed/failed responses can be treated as non-error output.
Suggested fix
- if (result.error) {
- return lineageError(result.error)
- }
+ const hasDispatcherError =
+ Object.prototype.hasOwnProperty.call(result, "error") &&
+ result.error !== null &&
+ result.error !== undefined
+ if (hasDispatcherError) {
+ return lineageError(normalizeError(result.error) || "Lineage check failed.")
+ }Also applies to: 65-69
🤖 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/lineage-check.ts` around lines 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.
Comment on lines
+28
to
+31
| const responseError = normalizeError(result.error) | ||
| if (result.success === false || responseError) { | ||
| return schemaError(responseError || "Schema inspection failed.") | ||
| } |
There was a problem hiding this comment.
Don’t rely on truthiness for result.error in failure gating.
At Line 29, responseError is falsy for error: "", so dispatcher failures can slip into the success path.
Suggested fix
- const responseError = normalizeError(result.error)
- if (result.success === false || responseError) {
- return schemaError(responseError || "Schema inspection failed.")
- }
+ const hasResponseError =
+ Object.prototype.hasOwnProperty.call(result, "error") &&
+ result.error !== null &&
+ result.error !== undefined
+ if (result.success === false || hasResponseError) {
+ return schemaError(normalizeError(result.error) || "Schema inspection failed.")
+ }Also applies to: 71-75
🤖 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/schema-inspect.ts` around lines 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.
❌ Tests — Failures DetectedTypeScript — 15 failure(s)
Next StepPlease address the failing cases above and re-run verification. cc @cerebrixos |
Contributor
|
@cerebrixos Thanks for the contribution. We will review it soon |
Contributor
|
@cerebrixos, can you please fix the failing tests? |
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 wires it into discovery, dbt, SQL tools, and docs, expanding support to 13 warehouses. Also hardens tool responses and improves lineage formatting for more reliable results.
New Features
trino-clientwith HTTP/HTTPS, Basic/Bearer auth, extra headers/session/extra credentials, and catalog/schema defaults.EXPLAIN/EXPLAIN ANALYZEsupported; query history intentionally not surfaced.connection_string, database/dbname →catalog, token →access_token); dbttrinomaps to the native driver; Docker discovery matches Trino images; env var detection forTRINO_*andDATABASE_URL(trino/presto); docs updated.Bug Fixes
lineage_check,schema_inspect, andsql_analyze: validate dispatcher payloads, normalize errors, and return consistent titles/metadata.Written for commit 8a7563d. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Tests
UX