AltimateAI / AltimateAI/altimate-code
SQL execution telemetry can't distinguish failed-execution from never-executed
- Lenguaje dominante
- TypeScript
- Estrellas
- 811
- Forks
- 134
- Merge medio
- 3 d 2 h
- PR fusionados (30 d)
- 50
Descripción
### What's the problem?
`sql_execute`'s SQL-structure telemetry (`sql_fingerprint`) can't distinguish "a warehouse ran this query and it failed" from "this query never reached a warehouse at all."
`sql.execute` (via `connections/register.ts`) returns the same result shape — `{ ..., error: string }` rather than throwing — for two very different situations:
1. **A warehouse actually ran the query and it failed** (bad SQL, permission error, connection dropped mid-query). This is exactly the kind of failure the fingerprint telemetry should capture — it's real SQL that a warehouse tried to execute.
2. **The query never reached a warehouse at all** — no warehouse configured, `Registry.get()` failed, connector setup/connection failed before any SQL ran.
`packages/opencode/src/altimate/tools/sql-execute.ts`'s result-error branch (`if (responseError !== undefined) { ... }`) sees both cases identically. There is currently no field on the result that says which one happened.
### History
This surfaced across three review rounds on PR #1238 (a follow-up to #1204):
- Round 1: the original ask was just "emit the fingerprint on the result-error branch too, not only on success" (a legitimate gap — failed executions were invisible to the telemetry).
- Round 2 review caught that the thrown-exception `catch` block (which only fires on a genuine non-execution failure, e.g. dispatcher down) was *also* being fingerprinted — double-counting/mislabeling never-executed queries as "failed execution."
- Round 3 review caught the deeper issue: even the result-error branch itself can't reliably claim "this was an execution" — `register.ts` returns that same shape for pre-execution failures.
At that point the fix had gone through three rounds trying to build a correct "was this actually executed" signal purely from the caller's side, without success — the information the caller needs doesn't exist yet at the point `sql_execute` receives the result.
### Decision (PR #1238)
De-scoped to fingerprint-on-success-only — the behavior that predates all of this. It's honest (a fingerprinted query definitely executed) even though it's incomplete (executed-but-failed queries currently aren't captured). This is intentionally the smaller, clearly-correct change rather than building a failed-execution-vs-never-executed taxonomy inside a review-debt cleanup PR.
See the code comment at the result-error branch in `packages/opencode/src/altimate/tools/sql-execute.ts` (references this issue).
### What the real fix needs
An explicit signal from the execution path itself, not something inferred from the result shape after the fact. Something like:
- An `executed: boolean` (or a small enum: `not_attempted` / `executed` / `unknown`) field on the result `register.ts`'s `sql.execute` handler returns, set based on whether the code actually reached the point of calling `connector.execute(...)` — not just whether an `error` string is present.
- `sql-execute.ts` then fingerprints on success OR (result-error AND `executed === true`), and skips fingerprinting for `executed === false`.
### Where to look
- `packages/opencode/src/altimate/native/connections/register.ts` (lines ~548–576 as of PR #1238) — the `sql.execute` handler that catches every connection/query error and returns the result-shaped `{ ..., error }` object. This is where a real "did we reach a warehouse" signal would need to originate.
- `packages/opencode/src/altimate/tools/sql-execute.ts` — the `sql_execute` tool, where the fingerprint would then branch on that signal instead of guessing from the result shape.
- `packages/opencode/test/altimate/telemetry-signals.test.ts` — has the structural test asserting the current (success-only) behavior; would need extending once the executed-phase signal exists.
Guía de contribución
Línea de trabajo
Start in packages/opencode/src/altimate/native/connections/register.ts around the sql.execute handler and trace where connector.execute(...) is actually reached. Then update packages/opencode/src/altimate/tools/sql-execute.ts to use that explicit execution signal for fingerprinting failed results. Extend packages/opencode/test/altimate/telemetry-signals.test.ts so done means success and executed failures are fingerprinted, while never-executed failures are not.
Escrito por el modelo de indexación a partir del texto del issue.
Evaluación
- Stack tecnológico
- sql, typescript
- Área
- backend, data-engineering, observability
- Tipo de issue
- Error
- Dificultad
- 3/5
- Tiempo estimado
- 1-2 días
- Estado de actividad
- Activo
- Claridad
- Bien especificado
- Aptitud para principiantes
- 72/100