Commit f87e9ac
fix: [AI-5975] propagate tool error messages to telemetry (#429)
* fix: [AI-5975] propagate actual error messages to telemetry instead of "unknown error"
Root cause: 6,905+ telemetry entries showed "unknown error" because tools
did not set `metadata.error` on failure paths. The telemetry layer in
`tool.ts` reads `metadata.error` — when missing, it logs "unknown error".
Changes:
- Add `error` field to `metadata` on all failure paths across 12 failing tools:
`altimate_core_validate`, `altimate_core_fix`, `altimate_core_correct`,
`altimate_core_semantics`, `altimate_core_equivalence`, `sql_explain`,
`sql_analyze`, `finops_query_history`, `finops_expensive_queries`,
`finops_analyze_credits`, `finops_unused_resources`, `finops_warehouse_advice`
- Add error extraction functions for nested napi-rs response structures:
`extractValidationErrors()` (data.errors[]),
`extractFixErrors()` (data.unfixable_errors[]),
`extractCorrectErrors()` (data.final_validation.errors[]),
`extractSemanticsErrors()` (data.validation_errors[]),
`extractEquivalenceErrors()` (data.validation_errors[])
- Wire `schema_path`/`schema_context` params through `sql_analyze` tool
to dispatcher (were completely disconnected — handler expected them)
- Add `schema_path` to `SqlAnalyzeParams` type
- Surface `unfixable_errors` from `sql.fix` handler in `register.ts`
- Clean up tool descriptions: remove "Rust-based altimate-core engine"
boilerplate, add schema guidance where applicable
- Add `error?: string` to `Tool.Metadata` interface in `tool.ts`
- Add 18 regression tests using mock dispatcher with real failure shapes
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: [AI-5975] early-return "No schema provided" error for `validate`, `semantics`, `equivalence`
Tools that require schema (`altimate_core_validate`, `altimate_core_semantics`,
`altimate_core_equivalence`) now return immediately with a clear error when
neither `schema_path` nor `schema_context` is provided, instead of calling
the napi handler and getting a cryptic "Table ? not found" error.
- Handles edge cases: empty string `schema_path` and empty object `schema_context`
- Cleaned up dead `noSchema` / `hasSchemaErrors` hint logic in validate
- Updated unit + E2E tests for no-schema early return behavior
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: [AI-5975] add `.filter(Boolean)` to all error extractors to prevent empty string errors
All 5 `extractXxxErrors()` functions could return `""` if error entries had
empty message fields, causing `"Error: "` output and breaking `alreadyValid`
logic in the fix tool. Added `.filter(Boolean)` to all extractors.
Also added regression test for empty string edge case.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: [AI-5975] treat "not equivalent" as valid result in `altimate_core_equivalence`
The handler returns `success: false` when queries are not equivalent,
but "not equivalent" is a valid analysis result — not a failure. This
was causing false `core_failure` telemetry events with "unknown error".
Uses the same `isRealFailure = !!error` pattern already applied to
`sql_analyze` and other tools.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: [AI-5975] address PR review feedback
- Guard `result.data ?? {}` to prevent TypeError when dispatcher returns no data
- Keep formatted output for LLM context; use `metadata.error` only for telemetry
- Filter empty strings and add `e.message` fallback in `register.ts` unfixable error extraction
- Fix misleading comment in `sql-analyze.ts` about handler success semantics
- Ensure `finops-analyze-credits` always sets concrete error string in metadata
- Add `success: false` to `schema-cache-status` catch path metadata
- Show "ERROR" title in equivalence when there's a real error (not "DIFFERENT")
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: [AI-5975] use error-driven title in semantics, generic error label in analyze
- Semantics: show "ERROR" title and pass error to `formatSemantics()` when
`result.error` or `validation_errors` are present
- Analyze: use generic "ERROR" label instead of "PARSE ERROR" since the
error path can carry non-parse failures too
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: [AI-5975] ensure `metadata.error` is never `undefined` on success paths
- `complete`, `grade`: use conditional spread `...(error && { error })` to
avoid setting `metadata.error = undefined` on success
- `sql-explain`: remove `error` from success path metadata entirely, add
`?? "Unknown error"` fallback to failure path
- `finops-expensive-queries`, `finops-query-history`, `finops-unused-resources`,
`finops-warehouse-advice`: add `?? "Unknown error"` fallback so metadata
always has a concrete error string on failure
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: [AI-5975] dispatcher wrappers: `success` means "handler completed", not "result was positive"
12 `altimate_core.*` dispatcher handlers derived `success` from result
data (`data.equivalent !== false`, `data.valid !== false`, etc.),
conflating "the tool crashed" with "the tool gave a negative finding".
Now all handlers return `ok(true, data)` when the Rust NAPI call
completes without throwing. Parse failures still throw → `fail(e)` →
`success: false` with the actual error message.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: [AI-5975] address code review findings for tool wrappers
- Add contract documentation to `ok()` explaining `success` semantics
- Make `extractEquivalenceErrors` and `extractSemanticsErrors` defensive
against object-type `validation_errors` entries
- Apply `isRealFailure` pattern to validate tool for consistent error handling
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: [AI-5975] revert validate tool to use dispatcher's `result.success` flag
The `isRealFailure` pattern from the review fix commit should not have
been applied to validate. The dispatcher already returns the correct
`success` flag via `ok()`. Validation findings (table not found) are
semantic results reported in data fields — the dispatcher's success
flag already handles this correctly.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: [AI-5975] make error propagation tests self-contained with mocks
Tests previously depended on the real NAPI binary which isn't available
in CI. Replace all real handler calls with dispatcher mocks that return
the same response shapes, matching actual tool wrapper behavior:
- validate: `result.success` passthrough — validation findings are
semantic results, not operational failures
- semantics: `result.success` passthrough — `validation_errors` surfaced
in `metadata.error` for telemetry but `success` unchanged
- equivalence: `isRealFailure` overrides `success` when `validation_errors`
exist (existing wrapper logic)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: [AI-5975] propagate tool error messages to telemetry across all 25 tools
- Add `?? {}` null guard on `result.data` for all altimate-core tool wrappers
- Extract `result.error ?? data.error` and spread into metadata conditionally
- Add `error: msg` to catch block metadata for impact-analysis, lineage-check
- Fix sql-fix unconditional error spread to conditional `...(result.error && { error })`
- Add comprehensive test suite (90 tests) covering all error propagation paths
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: [AI-5975] add missing `dialect` fields to test args for type-check
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: [AI-5975] remove spurious `dialect` from grade test (no such param)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: [AI-5975] update test expectation to match error title format after merge
The test expected "PARSE ERROR" in title but the actual format is
"Analyze: ERROR [confidence]". Updated assertion to match.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat: [AI-5975] add sql_quality telemetry for issue prevention metrics (#446)
* feat: [AI-5975] add `sql_quality` telemetry for issue prevention metrics
Add a new `sql_quality` telemetry event that fires whenever tools
successfully detect SQL issues — turning findings into measurable
"issues prevented" data in App Insights.
Architecture:
- New `sql_quality` event type in `Telemetry.Event` with `finding_count`,
`by_severity`, `by_category`, `has_schema`, `dialect`, `duration_ms`
- New `Telemetry.Finding` interface and `aggregateFindings()` helper
- Centralized emission in `tool.ts` — checks `metadata.findings` array
after any tool completes, aggregates counts, emits event
- Tools populate `metadata.findings` with `{category, severity}` pairs:
- `sql_analyze`: issue type + severity from lint/semantic/safety analysis
- `altimate_core_validate`: classified validation errors (missing_table,
missing_column, syntax_error, type_mismatch)
- `altimate_core_semantics`: rule/type + severity from semantic checks
- `altimate_core_fix`: fix_applied / unfixable_error categories
- `altimate_core_correct`: correction_applied findings
- `altimate_core_equivalence`: equivalence_difference findings
PII-safe: only category names and severity levels flow to telemetry,
never SQL content.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* refactor: [AI-5975] drop `by_severity` from `sql_quality` telemetry, address PR review
- Remove `severity` from `Finding` interface — only `category` matters
- Drop `by_severity` from `sql_quality` event type and `aggregateFindings`
- Gate `sql_quality` emission on `!isSoftFailure` to avoid double-counting
with `core_failure` events (Copilot review feedback)
- Simplify semantics tool: use fixed `"semantic_issue"` category instead of
dead `issue.rule ?? issue.type` fallback chain (CodeRabbit review feedback)
- Update test header to accurately describe what tests cover
- Fix sql_analyze test to use honest coarse categories (`"lint"`, `"safety"`)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* refactor: [AI-5975] drop `by_severity` from `sql_quality` telemetry, address PR review
- Remove `severity` from `Finding` interface — only `category` matters
- Drop `by_severity` from `sql_quality` event type and `aggregateFindings`
- Gate `sql_quality` emission on `!isSoftFailure` to avoid double-counting
with `core_failure` events (Copilot review feedback)
- Simplify semantics tool: use fixed `"semantic_issue"` category instead of
dead `issue.rule ?? issue.type` fallback chain (CodeRabbit review feedback)
- Update test header to accurately describe what tests cover
- Fix sql_analyze test to use honest coarse categories (`"lint"`, `"safety"`)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: [AI-5975] decouple `metadata.success` from domain outcomes in finding tools
Five tools were suppressing `sql_quality` telemetry because their
`metadata.success` tracked domain outcomes (SQL invalid, policy violated,
queries not equivalent) rather than engine execution success.
`tool.ts` gate: `!isSoftFailure && findings.length > 0`
- `isSoftFailure = metadata.success === false`
- Tools that found issues had `success: false` → findings suppressed
Fix: set `success: true` when the engine ran (even if it found problems).
Domain outcomes remain in dedicated fields (`valid`, `pass`, `equivalent`,
`fixed`). Only catch blocks set `success: false` (real engine crashes).
Affected tools:
- `altimate_core_validate` — validation errors now emit `sql_quality`
- `altimate_core_semantics` — semantic issues now emit `sql_quality`
- `altimate_core_policy` — policy violations now emit `sql_quality`
- `altimate_core_equivalence` — differences now emit `sql_quality`
- `altimate_core_fix` — unfixable errors now emit `sql_quality`
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* refactor: [AI-5975] remove hardcoded `dialect: "snowflake"` from core tools
- Remove `dialect` from metadata in 8 altimate-core/impact tools that don't
accept a dialect parameter (it was always hardcoded to "snowflake")
- Make `dialect` optional in `sql_quality` telemetry event type
- Only emit `dialect` when the tool actually provides it (sql-analyze,
sql-optimize, schema-diff still do via `args.dialect`)
- Tracked as #455 for adding proper dialect parameter support later
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: [AI-5975] guard finding arrays with `Array.isArray` for defensive safety
If a dispatcher returns a non-array for `errors`, `violations`, `issues`,
or `changes`, the `?? []` fallback handles null/undefined but not other
types. `Array.isArray` prevents `.map()` from throwing on unexpected payloads.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* fix: add missing `altimate_change` markers and prettier formatting in `tool.ts`
* style: prettier formatting on all changed tool files
* fix: preserve original formatting of `errorMsg` ternary to avoid marker guard false positive
* fix: restore original `errorMsg` indentation to match `main`
* fix: correct indentation of `altimate_change end` marker in `tool.ts`
* fix: propagate `metadata.error` in `sql-optimize` and `sql-translate`, compute `has_schema` in `sql-analyze`
* fix: remove unused `@ts-expect-error` directives now that `define()` generic is simplified
---------
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: anandgupta42 <93243293+anandgupta42@users.noreply.github.com>
Co-authored-by: anandgupta42 <anand@altimate.ai>1 parent 722153d commit f87e9ac
File tree
62 files changed
+3463
-1099
lines changed- packages/opencode
- src
- altimate
- native
- sql
- telemetry
- tools
- tool
- test/altimate
- tools
Some content is hidden
Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
62 files changed
+3463
-1099
lines changedLines changed: 417 additions & 438 deletions
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
24 | 24 | | |
25 | 25 | | |
26 | 26 | | |
| 27 | + | |
27 | 28 | | |
28 | 29 | | |
29 | 30 | | |
30 | 31 | | |
31 | 32 | | |
| 33 | + | |
32 | 34 | | |
33 | 35 | | |
34 | 36 | | |
| |||
385 | 387 | | |
386 | 388 | | |
387 | 389 | | |
| 390 | + | |
388 | 391 | | |
389 | 392 | | |
390 | 393 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
48 | 48 | | |
49 | 49 | | |
50 | 50 | | |
51 | | - | |
52 | | - | |
| 51 | + | |
| 52 | + | |
53 | 53 | | |
54 | 54 | | |
55 | 55 | | |
| |||
401 | 401 | | |
402 | 402 | | |
403 | 403 | | |
404 | | - | |
405 | | - | |
406 | | - | |
407 | | - | |
408 | | - | |
409 | | - | |
410 | | - | |
411 | | - | |
| 404 | + | |
412 | 405 | | |
413 | 406 | | |
414 | 407 | | |
415 | 408 | | |
416 | 409 | | |
| 410 | + | |
| 411 | + | |
| 412 | + | |
| 413 | + | |
| 414 | + | |
| 415 | + | |
| 416 | + | |
| 417 | + | |
| 418 | + | |
| 419 | + | |
| 420 | + | |
| 421 | + | |
| 422 | + | |
| 423 | + | |
| 424 | + | |
417 | 425 | | |
418 | 426 | | |
419 | 427 | | |
| |||
476 | 484 | | |
477 | 485 | | |
478 | 486 | | |
479 | | - | |
480 | | - | |
481 | | - | |
482 | | - | |
| 487 | + | |
| 488 | + | |
| 489 | + | |
| 490 | + | |
| 491 | + | |
| 492 | + | |
| 493 | + | |
| 494 | + | |
| 495 | + | |
| 496 | + | |
| 497 | + | |
| 498 | + | |
| 499 | + | |
| 500 | + | |
| 501 | + | |
| 502 | + | |
| 503 | + | |
| 504 | + | |
| 505 | + | |
| 506 | + | |
483 | 507 | | |
484 | | - | |
485 | | - | |
| 508 | + | |
| 509 | + | |
| 510 | + | |
| 511 | + | |
| 512 | + | |
| 513 | + | |
| 514 | + | |
| 515 | + | |
486 | 516 | | |
487 | 517 | | |
488 | 518 | | |
489 | 519 | | |
490 | | - | |
491 | | - | |
492 | | - | |
| 520 | + | |
493 | 521 | | |
494 | 522 | | |
495 | 523 | | |
| |||
674 | 702 | | |
675 | 703 | | |
676 | 704 | | |
677 | | - | |
| 705 | + | |
678 | 706 | | |
679 | 707 | | |
680 | 708 | | |
| |||
789 | 817 | | |
790 | 818 | | |
791 | 819 | | |
| 820 | + | |
| 821 | + | |
| 822 | + | |
| 823 | + | |
| 824 | + | |
| 825 | + | |
| 826 | + | |
| 827 | + | |
| 828 | + | |
| 829 | + | |
| 830 | + | |
| 831 | + | |
| 832 | + | |
| 833 | + | |
| 834 | + | |
| 835 | + | |
792 | 836 | | |
793 | 837 | | |
794 | 838 | | |
| |||
Lines changed: 31 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | 2 | | |
3 | 3 | | |
| 4 | + | |
4 | 5 | | |
5 | 6 | | |
6 | 7 | | |
7 | | - | |
| 8 | + | |
8 | 9 | | |
9 | 10 | | |
10 | 11 | | |
11 | 12 | | |
12 | 13 | | |
13 | 14 | | |
| 15 | + | |
14 | 16 | | |
15 | 17 | | |
16 | 18 | | |
17 | 19 | | |
18 | 20 | | |
19 | 21 | | |
20 | | - | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
21 | 39 | | |
22 | 40 | | |
23 | | - | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
24 | 47 | | |
25 | 48 | | |
26 | 49 | | |
27 | 50 | | |
28 | | - | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
29 | 56 | | |
30 | 57 | | |
31 | 58 | | |
| |||
Lines changed: 9 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
4 | 4 | | |
5 | 5 | | |
6 | 6 | | |
7 | | - | |
| 7 | + | |
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
| |||
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
18 | | - | |
| 18 | + | |
19 | 19 | | |
20 | 20 | | |
| 21 | + | |
21 | 22 | | |
22 | 23 | | |
23 | | - | |
| 24 | + | |
24 | 25 | | |
25 | 26 | | |
26 | 27 | | |
27 | 28 | | |
28 | | - | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
29 | 34 | | |
30 | 35 | | |
31 | 36 | | |
| |||
Lines changed: 9 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
4 | 4 | | |
5 | 5 | | |
6 | 6 | | |
7 | | - | |
| 7 | + | |
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
| |||
19 | 19 | | |
20 | 20 | | |
21 | 21 | | |
22 | | - | |
| 22 | + | |
23 | 23 | | |
| 24 | + | |
24 | 25 | | |
25 | 26 | | |
26 | | - | |
| 27 | + | |
27 | 28 | | |
28 | 29 | | |
29 | 30 | | |
30 | 31 | | |
31 | | - | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
32 | 37 | | |
33 | 38 | | |
34 | 39 | | |
| |||
Lines changed: 9 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
4 | 4 | | |
5 | 5 | | |
6 | 6 | | |
7 | | - | |
| 7 | + | |
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
| |||
17 | 17 | | |
18 | 18 | | |
19 | 19 | | |
20 | | - | |
| 20 | + | |
21 | 21 | | |
| 22 | + | |
22 | 23 | | |
23 | 24 | | |
24 | | - | |
| 25 | + | |
25 | 26 | | |
26 | 27 | | |
27 | 28 | | |
28 | 29 | | |
29 | | - | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
30 | 35 | | |
31 | 36 | | |
32 | 37 | | |
| |||
Lines changed: 9 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
4 | 4 | | |
5 | 5 | | |
6 | 6 | | |
7 | | - | |
| 7 | + | |
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
| |||
19 | 19 | | |
20 | 20 | | |
21 | 21 | | |
22 | | - | |
| 22 | + | |
23 | 23 | | |
| 24 | + | |
24 | 25 | | |
25 | 26 | | |
26 | | - | |
| 27 | + | |
27 | 28 | | |
28 | 29 | | |
29 | 30 | | |
30 | 31 | | |
31 | | - | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
32 | 37 | | |
33 | 38 | | |
34 | 39 | | |
| |||
0 commit comments