Skip to content

tools: dispatcher errors are returned as data, and most tools never check for them #1210

Description

@anandgupta42

What happens

The native dispatcher handlers do not throw. They catch every error and return it as a field on a result-shaped object:

// native/connections/register.ts:437 — sql.execute
return { columns: [], rows: [], row_count: 0, truncated: false, error: errorMsg } as SqlExecuteResult & { error: string }

schema.inspect (:562), sql.explain (:605) and dbt.profiles (:624) do the same. That shape is fine on its own — but it means every consuming tool is responsible for checking error, and most do not.

sql_execute was one of them, and it is fixed in #1204: an unresolvable warehouse rendered as the literal string (0 rows) — followed by a cheerful tip about query optimization — because formatResult() only looked at row_count. An agent asking what tables exist was told the warehouse was healthy and empty.

The rest of the class

These tools call Dispatcher.call(...) and never read error in any form:

  • dbt-lineage.ts
  • impact-analysis.ts
  • project-scan.ts
  • schema-cache-status.ts
  • schema-detect-pii.ts
  • schema-index.ts
  • schema-search.ts
  • sql-autocomplete.ts
  • sql-diff.ts
  • warehouse-list.ts

Not all of them are necessarily wrong — some call handlers that use a success: false convention instead — but each needs checking, because the failure mode is the worst kind: a failure that renders as valid, empty data. There is no fault string for a caller or a harness to key on, and an agent handed an empty result answers confidently from nothing.

schema_inspect, sql_analyze, lineage_check and altimate_core_column_lineage already do this correctly via normalizeError (tools/response-normalization.ts), so the house pattern exists and is cheap to apply.

Suggested fix

  1. Audit each tool above against the handler it calls; apply normalizeError(result.error) where the handler can return one.
  2. Consider a shared helper so a tool cannot forget — e.g. a wrapper around Dispatcher.call that throws when the result carries an error, with an opt-out for the handful of callers that genuinely want to inspect it.
  3. Add a lint or a test that fails when a tool calls the dispatcher and never consults error.

Why not fixed in #1204

#1204 fixes the specific path that produced the reported defect (sql_execute) plus the driver-level causes behind it. Sweeping ten more tools is a separate change with per-tool semantics to establish, and it should be reviewable on its own rather than buried in a warehouse-path fix.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions