fix: [AI-5975] propagate tool error messages to telemetry#429
Open
suryaiyer95 wants to merge 12 commits intomainfrom
Open
fix: [AI-5975] propagate tool error messages to telemetry#429suryaiyer95 wants to merge 12 commits intomainfrom
suryaiyer95 wants to merge 12 commits intomainfrom
Conversation
|
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:
📝 WalkthroughWalkthroughStandardizes propagation of human-readable errors into tool metadata ( Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
suryaiyer95
force-pushed
the
fix/ai-5975-tool-error-propagation
branch
2 times, most recently
from
f9e0f1c to
1c0e251
Compare
…f "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>
suryaiyer95
force-pushed
the
fix/ai-5975-tool-error-propagation
branch
from
1c0e251 to
56194d7
Compare
…, `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>
…ent 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>
…ern for error Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…re_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>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Pull request overview
This PR improves telemetry quality by standardizing how tools surface failure details (via metadata.error), adds schema wiring for schema-aware SQL analysis, and introduces regression tests to prevent “unknown error” telemetry entries from recurring.
Changes:
- Add
metadata.erroras a standard tool metadata field and populate it across many tool failure paths. - Improve schema-aware behavior by wiring
schema_path/schema_contextthroughsql_analyzeand updating tool descriptions to guide schema usage. - Add regression tests that validate error extraction/propagation for multiple real-world failure payload shapes.
Reviewed changes
Copilot reviewed 52 out of 52 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/opencode/test/altimate/tool-error-propagation.test.ts | Adds regression tests for error propagation into metadata.error for telemetry. |
| packages/opencode/src/tool/tool.ts | Introduces metadata.error typing and adjusts Tool.define generics. |
| packages/opencode/src/altimate/tools/warehouse-test.ts | Adds metadata.error on error path. |
| packages/opencode/src/altimate/tools/warehouse-remove.ts | Adds metadata.error on error path. |
| packages/opencode/src/altimate/tools/sql-translate.ts | Adds metadata.error on error path. |
| packages/opencode/src/altimate/tools/sql-rewrite.ts | Adds metadata.error on error path. |
| packages/opencode/src/altimate/tools/sql-optimize.ts | Adds metadata.error on error path. |
| packages/opencode/src/altimate/tools/sql-format.ts | Adds metadata.error on failure/error paths. |
| packages/opencode/src/altimate/tools/sql-fix.ts | Adds metadata.error in result metadata and on error path. |
| packages/opencode/src/altimate/tools/sql-explain.ts | Adds metadata.error on all paths for telemetry extraction. |
| packages/opencode/src/altimate/tools/sql-execute.ts | Adds metadata.error on error path. |
| packages/opencode/src/altimate/tools/sql-diff.ts | Adds metadata.error on error path. |
| packages/opencode/src/altimate/tools/sql-analyze.ts | Wires schema params through to dispatcher + improves description and soft-failure semantics. |
| packages/opencode/src/altimate/tools/schema-search.ts | Adds metadata.error on error path. |
| packages/opencode/src/altimate/tools/schema-diff.ts | Adds metadata.error on error path. |
| packages/opencode/src/altimate/tools/schema-detect-pii.ts | Adds metadata.error on error path. |
| packages/opencode/src/altimate/tools/schema-cache-status.ts | Adds metadata.error on error path. |
| packages/opencode/src/altimate/tools/finops-warehouse-advice.ts | Adds metadata.error on failure/error paths. |
| packages/opencode/src/altimate/tools/finops-unused-resources.ts | Adds metadata.error on failure/error paths. |
| packages/opencode/src/altimate/tools/finops-role-access.ts | Adds metadata.error on error paths across role tools. |
| packages/opencode/src/altimate/tools/finops-query-history.ts | Adds metadata.error on failure/error paths. |
| packages/opencode/src/altimate/tools/finops-expensive-queries.ts | Adds metadata.error on failure/error paths. |
| packages/opencode/src/altimate/tools/finops-analyze-credits.ts | Adds metadata.error on failure/error paths. |
| packages/opencode/src/altimate/tools/dbt-lineage.ts | Adds metadata.error on error path. |
| packages/opencode/src/altimate/tools/altimate-core-validate.ts | Adds no-schema early return + extracts nested validation errors into metadata.error. |
| packages/opencode/src/altimate/tools/altimate-core-track-lineage.ts | Updates description with schema guidance. |
| packages/opencode/src/altimate/tools/altimate-core-testgen.ts | Updates description with schema guidance. |
| packages/opencode/src/altimate/tools/altimate-core-semantics.ts | Adds no-schema early return + extracts nested semantics errors into metadata.error. |
| packages/opencode/src/altimate/tools/altimate-core-schema-diff.ts | Updates description (removes boilerplate). |
| packages/opencode/src/altimate/tools/altimate-core-rewrite.ts | Updates description with schema guidance. |
| packages/opencode/src/altimate/tools/altimate-core-resolve-term.ts | Updates description with schema guidance. |
| packages/opencode/src/altimate/tools/altimate-core-query-pii.ts | Updates description with schema guidance. |
| packages/opencode/src/altimate/tools/altimate-core-prune-schema.ts | Updates description with schema guidance. |
| packages/opencode/src/altimate/tools/altimate-core-policy.ts | Updates description with schema guidance. |
| packages/opencode/src/altimate/tools/altimate-core-parse-dbt.ts | Updates description (removes boilerplate). |
| packages/opencode/src/altimate/tools/altimate-core-optimize-context.ts | Updates description with schema guidance. |
| packages/opencode/src/altimate/tools/altimate-core-migration.ts | Updates description (removes boilerplate). |
| packages/opencode/src/altimate/tools/altimate-core-import-ddl.ts | Updates description (removes boilerplate). |
| packages/opencode/src/altimate/tools/altimate-core-grade.ts | Adds metadata.error plumbing. |
| packages/opencode/src/altimate/tools/altimate-core-fix.ts | Extracts nested unfixable/fix errors into metadata.error + adjusts success/title semantics. |
| packages/opencode/src/altimate/tools/altimate-core-fingerprint.ts | Updates description with schema guidance. |
| packages/opencode/src/altimate/tools/altimate-core-extract-metadata.ts | Updates description (removes boilerplate). |
| packages/opencode/src/altimate/tools/altimate-core-export-ddl.ts | Updates description with schema guidance. |
| packages/opencode/src/altimate/tools/altimate-core-equivalence.ts | Adds no-schema early return + extracts nested errors + treats “different” as non-failure when no error. |
| packages/opencode/src/altimate/tools/altimate-core-correct.ts | Extracts nested errors into metadata.error. |
| packages/opencode/src/altimate/tools/altimate-core-complete.ts | Adds metadata.error plumbing. |
| packages/opencode/src/altimate/tools/altimate-core-compare.ts | Updates description (removes boilerplate). |
| packages/opencode/src/altimate/tools/altimate-core-column-lineage.ts | Updates description with schema guidance. |
| packages/opencode/src/altimate/tools/altimate-core-classify-pii.ts | Updates description with schema guidance. |
| packages/opencode/src/altimate/tools/altimate-core-check.ts | Updates description with schema guidance. |
| packages/opencode/src/altimate/native/types.ts | Extends dispatcher type contracts for schema + error plumbing (e.g., SqlAnalyzeParams.schema_path, SqlFixResult.error). |
| packages/opencode/src/altimate/native/sql/register.ts | Extracts unfixable errors into SqlFixResult.error. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/opencode/src/altimate/tools/altimate-core-semantics.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/opencode/src/altimate/tools/altimate-core-import-ddl.ts (1)
24-27:⚠️ Potential issue | 🟠 MajorPropagate the error message in
metadata.erroron failure.The catch path still returns
success: falsewithoutmetadata.error, which can keep telemetry entries as"unknown error"for this tool.Suggested fix
} catch (e) { const msg = e instanceof Error ? e.message : String(e) - return { title: "Import DDL: ERROR", metadata: { success: false }, output: `Failed: ${msg}` } + return { + title: "Import DDL: ERROR", + metadata: { success: false, error: msg }, + output: `Failed: ${msg}`, + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/tools/altimate-core-import-ddl.ts` around lines 24 - 27, In the catch block that returns the failure object (the code that currently builds { title: "Import DDL: ERROR", metadata: { success: false }, output: `Failed: ${msg}` }), add the error message into metadata (e.g. metadata.error = msg) so telemetry receives the actual error string; locate the catch in altimate-core-import-ddl.ts (the return inside the catch that uses msg) and modify the returned object to include metadata.error set to the computed msg.packages/opencode/src/altimate/tools/finops-role-access.ts (1)
118-123:⚠️ Potential issue | 🟠 MajorNon-throwing failure paths still drop the error message in metadata.
On Line 121, Line 154, and Line 193,
metadatasetssuccess: falsebut omitserror. These are the exact soft-failure paths (result.success === false), so telemetry will still fall back to"unknown error"there.Suggested fix
if (!result.success) { return { title: "Role Grants: FAILED", - metadata: { success: false, grant_count: 0 }, + metadata: { success: false, grant_count: 0, error: result.error ?? "Unknown error" }, output: `Failed to query grants: ${result.error ?? "Unknown error"}`, } } @@ if (!result.success) { return { title: "Role Hierarchy: FAILED", - metadata: { success: false, role_count: 0 }, + metadata: { success: false, role_count: 0, error: result.error ?? "Unknown error" }, output: `Failed to query role hierarchy: ${result.error ?? "Unknown error"}`, } } @@ if (!result.success) { return { title: "User Roles: FAILED", - metadata: { success: false, assignment_count: 0 }, + metadata: { success: false, assignment_count: 0, error: result.error ?? "Unknown error" }, output: `Failed to query user roles: ${result.error ?? "Unknown error"}`, } }Also applies to: 151-156, 190-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/tools/finops-role-access.ts` around lines 118 - 123, The soft-failure return blocks (where you check result.success === false and return objects like the "Role Grants: FAILED" response) currently set metadata: { success: false, grant_count: 0 } but omit the actual error; update these return objects to include the error from result (e.g., metadata: { success: false, grant_count: 0, error: result.error ?? "Unknown error" }) so telemetry gets the real failure detail; apply the same change to every similar soft-failure return in finops-role-access.ts that uses result.error.packages/opencode/src/altimate/tools/sql-optimize.ts (1)
34-43:⚠️ Potential issue | 🟠 MajorAdd
metadata.errorfor dispatcher non-success results.Line 35 returns a shared payload for both success and parse-failure, but
metadata.erroris not set whenresult.successis false. This leaves a failure path without propagated error details.Suggested fix
return { title: `Optimize: ${result.success ? `${suggestionCount} suggestion${suggestionCount !== 1 ? "s" : ""}, ${antiPatternCount} anti-pattern${antiPatternCount !== 1 ? "s" : ""}` : "PARSE ERROR"} [${result.confidence}]`, metadata: { success: result.success, suggestionCount, antiPatternCount, hasOptimizedSql: !!result.optimized_sql, confidence: result.confidence, + error: result.success ? undefined : result.error, }, output: formatOptimization(result), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/tools/sql-optimize.ts` around lines 34 - 43, The returned payload from the optimization handler builds metadata but omits error details when result.success is false; update the object returned in the function that constructs title/metadata/output (the block using result, suggestionCount, antiPatternCount, and formatOptimization) to include metadata.error set to the failure info (e.g., metadata.error = result.error ?? result.message ?? null) so non-success/parse-failure paths propagate the error; keep metadata.error unset or null for successful results.packages/opencode/src/altimate/tools/warehouse-remove.ts (1)
22-26:⚠️ Potential issue | 🟠 MajorPropagate
result.errorinto metadata on non-throw failures too.When
result.successis false (Line 22 onward), metadata currently omits the error even though output includes it. This leaves a remaining telemetry blind spot.Suggested patch
return { title: `Remove '${args.name}': FAILED`, - metadata: { success: false }, + metadata: { + success: false, + error: result.error ?? "Connection not found or is defined via environment variable", + }, output: `Failed to remove warehouse '${args.name}'.\nError: ${result.error ?? "Connection not found or is defined via environment variable"}`, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/tools/warehouse-remove.ts` around lines 22 - 26, The failure return path currently sets metadata: { success: false } but does not propagate result.error; update the failure branch that returns the object (the block which returns title `Remove '${args.name}': FAILED` and uses output including result.error) to include the error in metadata (e.g., add an error property with result.error ?? "Connection not found or is defined via environment variable") so telemetry captures the error alongside success, referencing the same result variable and args.name used in that return.packages/opencode/src/altimate/tools/altimate-core-track-lineage.ts (1)
22-30:⚠️ Potential issue | 🟡 MinorMissing
metadata.errorpropagation on both success and catch paths.This tool's description was updated to match sibling tools, but unlike other tools in this PR (e.g.,
altimate-core-grade.ts), themetadataobject does not include theerrorfield. This means failures from this tool will still log "unknown error" in telemetry.🔧 Proposed fix to add error propagation
const data = result.data as Record<string, any> const edgeCount = data.edges?.length ?? 0 + const error = result.error ?? data.error return { title: `Track Lineage: ${edgeCount} edge(s) across ${args.queries.length} queries`, - metadata: { success: result.success, edge_count: edgeCount }, + metadata: { success: result.success, edge_count: edgeCount, error }, output: formatTrackLineage(data), } } catch (e) { const msg = e instanceof Error ? e.message : String(e) - return { title: "Track Lineage: ERROR", metadata: { success: false, edge_count: 0 }, output: `Failed: ${msg}` } + return { title: "Track Lineage: ERROR", metadata: { success: false, edge_count: 0, error: msg }, output: `Failed: ${msg}` } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/tools/altimate-core-track-lineage.ts` around lines 22 - 30, The metadata object returned by the Track Lineage tool lacks an error field; update the success path in the function that returns title/metadata/output (the block that uses result.success, edgeCount and formatTrackLineage) to include metadata.error (e.g., set to result.error or null), and update the catch-path return to include metadata.error with the captured msg (the const msg = e instanceof Error ? e.message : String(e)) so telemetry receives the error on both success and failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/altimate/native/sql/register.ts`:
- Around line 218-220: The unfixable error extraction (variable unfixableError
and the similar occurrence later) currently prefers nested error.error?.message
then reason, which can ignore a top-level message field; update the fallback
order to prefer e.message first, then e.error?.message, then e.reason, then
String(e) (i.e. use e.message ?? e.error?.message ?? e.reason ?? String(e)) for
both the unfixableError mapping and the other analogous mapping to ensure
top-level message fields are preserved in telemetry.
In `@packages/opencode/src/altimate/tools/altimate-core-correct.ts`:
- Around line 20-25: The code assumes result.data exists which can throw and
hide upstream dispatcher failures; change the allocation of data in
altimate-core-correct.ts so result.data is defaulted to an empty object (e.g.,
use result.data ?? {}) before casting, and keep the rest of the logic (error =
result.error ?? data.error ?? extractCorrectErrors(data), metadata iterations
read from data.iterations, and output using formatCorrect(data)) so that missing
data doesn't throw a TypeError and original dispatcher errors remain visible.
In `@packages/opencode/src/altimate/tools/altimate-core-validate.ts`:
- Around line 25-30: The code dereferences result.data without guarding, which
can throw and mask upstream errors; update the error and data handling in the
Validate block (references: result, data, extractValidationErrors,
formatValidate) so you first check whether result.data is present before
accessing its properties — e.g. compute error as result.error ?? (result.data ?
result.data.error ?? extractValidationErrors(result.data) : undefined) and only
call formatValidate(data) when data exists (or provide a safe fallback) so real
errors propagate instead of causing a TypeError.
In `@packages/opencode/src/altimate/tools/finops-analyze-credits.ts`:
- Around line 85-86: The failure metadata currently sets error: result.error
which can be undefined; update the non-success return in
finops-analyze-credits.ts (the object containing metadata and output) to use the
same fallback as output, e.g. set metadata.error to result.error ?? "Unknown
error" (or String(result.error) ?? "Unknown error") so telemetry always contains
a concrete error string.
In `@packages/opencode/src/altimate/tools/schema-cache-status.ts`:
- Line 26: The failure metadata object currently sets error: msg but omits a
success flag; update the metadata literal (the object that currently reads
metadata: { totalTables: 0, totalColumns: 0, warehouseCount: 0, error: msg }) to
include success: false so telemetry checks (metadata.success === false) work
correctly—i.e., change that object to metadata: { totalTables: 0, totalColumns:
0, warehouseCount: 0, error: msg, success: false } in the same error/failure
branch where msg is set.
In `@packages/opencode/src/altimate/tools/sql-execute.ts`:
- Line 50: The telemetry metadata currently stores raw exception text via
metadata.error = msg which may leak sensitive SQL literals or tokens; change the
assignment to pass a sanitized representation instead: implement or call a
sanitizer (e.g., sanitizeError or redactSensitiveInfo) to strip SQL
literals/credentials and only include safe fields (error code, high-level
message, or a redacted message) and/or a non-sensitive hash, then set
metadata.error to that sanitized string and optionally include error.code or a
boolean flag (e.g., metadata.sanitized = true) so downstream consumers know it
was redacted; update the location where metadata (and the msg variable) is set
in sql-execute.ts to use this sanitizer.
---
Outside diff comments:
In `@packages/opencode/src/altimate/tools/altimate-core-import-ddl.ts`:
- Around line 24-27: In the catch block that returns the failure object (the
code that currently builds { title: "Import DDL: ERROR", metadata: { success:
false }, output: `Failed: ${msg}` }), add the error message into metadata (e.g.
metadata.error = msg) so telemetry receives the actual error string; locate the
catch in altimate-core-import-ddl.ts (the return inside the catch that uses msg)
and modify the returned object to include metadata.error set to the computed
msg.
In `@packages/opencode/src/altimate/tools/altimate-core-track-lineage.ts`:
- Around line 22-30: The metadata object returned by the Track Lineage tool
lacks an error field; update the success path in the function that returns
title/metadata/output (the block that uses result.success, edgeCount and
formatTrackLineage) to include metadata.error (e.g., set to result.error or
null), and update the catch-path return to include metadata.error with the
captured msg (the const msg = e instanceof Error ? e.message : String(e)) so
telemetry receives the error on both success and failure.
In `@packages/opencode/src/altimate/tools/finops-role-access.ts`:
- Around line 118-123: The soft-failure return blocks (where you check
result.success === false and return objects like the "Role Grants: FAILED"
response) currently set metadata: { success: false, grant_count: 0 } but omit
the actual error; update these return objects to include the error from result
(e.g., metadata: { success: false, grant_count: 0, error: result.error ??
"Unknown error" }) so telemetry gets the real failure detail; apply the same
change to every similar soft-failure return in finops-role-access.ts that uses
result.error.
In `@packages/opencode/src/altimate/tools/sql-optimize.ts`:
- Around line 34-43: The returned payload from the optimization handler builds
metadata but omits error details when result.success is false; update the object
returned in the function that constructs title/metadata/output (the block using
result, suggestionCount, antiPatternCount, and formatOptimization) to include
metadata.error set to the failure info (e.g., metadata.error = result.error ??
result.message ?? null) so non-success/parse-failure paths propagate the error;
keep metadata.error unset or null for successful results.
In `@packages/opencode/src/altimate/tools/warehouse-remove.ts`:
- Around line 22-26: The failure return path currently sets metadata: { success:
false } but does not propagate result.error; update the failure branch that
returns the object (the block which returns title `Remove '${args.name}':
FAILED` and uses output including result.error) to include the error in metadata
(e.g., add an error property with result.error ?? "Connection not found or is
defined via environment variable") so telemetry captures the error alongside
success, referencing the same result variable and args.name used in that return.
🪄 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: f95131a7-a007-4fea-b23c-a308e4a21208
📒 Files selected for processing (52)
packages/opencode/src/altimate/native/sql/register.tspackages/opencode/src/altimate/native/types.tspackages/opencode/src/altimate/tools/altimate-core-check.tspackages/opencode/src/altimate/tools/altimate-core-classify-pii.tspackages/opencode/src/altimate/tools/altimate-core-column-lineage.tspackages/opencode/src/altimate/tools/altimate-core-compare.tspackages/opencode/src/altimate/tools/altimate-core-complete.tspackages/opencode/src/altimate/tools/altimate-core-correct.tspackages/opencode/src/altimate/tools/altimate-core-equivalence.tspackages/opencode/src/altimate/tools/altimate-core-export-ddl.tspackages/opencode/src/altimate/tools/altimate-core-extract-metadata.tspackages/opencode/src/altimate/tools/altimate-core-fingerprint.tspackages/opencode/src/altimate/tools/altimate-core-fix.tspackages/opencode/src/altimate/tools/altimate-core-grade.tspackages/opencode/src/altimate/tools/altimate-core-import-ddl.tspackages/opencode/src/altimate/tools/altimate-core-migration.tspackages/opencode/src/altimate/tools/altimate-core-optimize-context.tspackages/opencode/src/altimate/tools/altimate-core-parse-dbt.tspackages/opencode/src/altimate/tools/altimate-core-policy.tspackages/opencode/src/altimate/tools/altimate-core-prune-schema.tspackages/opencode/src/altimate/tools/altimate-core-query-pii.tspackages/opencode/src/altimate/tools/altimate-core-resolve-term.tspackages/opencode/src/altimate/tools/altimate-core-rewrite.tspackages/opencode/src/altimate/tools/altimate-core-schema-diff.tspackages/opencode/src/altimate/tools/altimate-core-semantics.tspackages/opencode/src/altimate/tools/altimate-core-testgen.tspackages/opencode/src/altimate/tools/altimate-core-track-lineage.tspackages/opencode/src/altimate/tools/altimate-core-validate.tspackages/opencode/src/altimate/tools/dbt-lineage.tspackages/opencode/src/altimate/tools/finops-analyze-credits.tspackages/opencode/src/altimate/tools/finops-expensive-queries.tspackages/opencode/src/altimate/tools/finops-query-history.tspackages/opencode/src/altimate/tools/finops-role-access.tspackages/opencode/src/altimate/tools/finops-unused-resources.tspackages/opencode/src/altimate/tools/finops-warehouse-advice.tspackages/opencode/src/altimate/tools/schema-cache-status.tspackages/opencode/src/altimate/tools/schema-detect-pii.tspackages/opencode/src/altimate/tools/schema-diff.tspackages/opencode/src/altimate/tools/schema-search.tspackages/opencode/src/altimate/tools/sql-analyze.tspackages/opencode/src/altimate/tools/sql-diff.tspackages/opencode/src/altimate/tools/sql-execute.tspackages/opencode/src/altimate/tools/sql-explain.tspackages/opencode/src/altimate/tools/sql-fix.tspackages/opencode/src/altimate/tools/sql-format.tspackages/opencode/src/altimate/tools/sql-optimize.tspackages/opencode/src/altimate/tools/sql-rewrite.tspackages/opencode/src/altimate/tools/sql-translate.tspackages/opencode/src/altimate/tools/warehouse-remove.tspackages/opencode/src/altimate/tools/warehouse-test.tspackages/opencode/src/tool/tool.tspackages/opencode/test/altimate/tool-error-propagation.test.ts
- 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>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/opencode/src/altimate/tools/altimate-core-semantics.ts (1)
39-43: Normalizevalidation_errorsentries before joining.If
validation_errorsincludes objects, current joining can produce low-signal strings like[object Object]. Consider extracting string/message fields explicitly and trimming empties.Suggested refactor
function extractSemanticsErrors(data: Record<string, any>): string | undefined { if (Array.isArray(data.validation_errors) && data.validation_errors.length > 0) { - const msgs = data.validation_errors.filter(Boolean) + const msgs = data.validation_errors + .map((e) => { + if (typeof e === "string") return e.trim() + if (e && typeof e === "object" && typeof e.message === "string") return e.message.trim() + return "" + }) + .filter((m) => m.length > 0) return msgs.length > 0 ? msgs.join("; ") : undefined } return undefined }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/tools/altimate-core-semantics.ts` around lines 39 - 43, In extractSemanticsErrors, normalize entries in data.validation_errors before joining: map each entry to a trimmed string by handling strings directly, extracting common fields like message, msg, or error from objects (falling back to JSON.stringify or String(entry)), filter out falsy/empty results, then join with "; " and return undefined if none remain; ensure this logic is applied inside the existing Array.isArray check so msgs contains meaningful text instead of "[object Object]".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/altimate/tools/altimate-core-semantics.ts`:
- Around line 26-30: The computed error (const error = result.error ??
data.error ?? extractSemanticsErrors(data)) must be used to determine the
user-facing title and output when present: update the title generation in the
object returned by the semantics handler (the title property currently using
data.valid and issueCount) to show an error state (e.g., "Semantics: ERROR" or
include the error message) whenever error is truthy, and ensure the output (the
value produced by formatSemantics(data)) or metadata reflects the error state
consistently; reference the existing symbols error, result.error, data,
issueCount, title, metadata, output, formatSemantics, and extractSemanticsErrors
to locate and change the logic so any computed error forces the error-style
title/output.
In `@packages/opencode/src/altimate/tools/sql-analyze.ts`:
- Around line 31-38: The title currently hardcodes "PARSE ERROR" whenever
result.error is truthy which mislabels non-parse failures; update the code that
builds the returned title (the string using result.error, result.issue_count,
result.confidence) to use a generic label such as "ERROR" or "ANALYSIS ERROR"
instead of "PARSE ERROR" when result.error is present so the UI shows a correct,
non-misleading status for all error types.
---
Nitpick comments:
In `@packages/opencode/src/altimate/tools/altimate-core-semantics.ts`:
- Around line 39-43: In extractSemanticsErrors, normalize entries in
data.validation_errors before joining: map each entry to a trimmed string by
handling strings directly, extracting common fields like message, msg, or error
from objects (falling back to JSON.stringify or String(entry)), filter out
falsy/empty results, then join with "; " and return undefined if none remain;
ensure this logic is applied inside the existing Array.isArray check so msgs
contains meaningful text instead of "[object Object]".
🪄 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: c772aedf-6986-4614-a56e-2909b0ed7b5f
📒 Files selected for processing (9)
packages/opencode/src/altimate/native/sql/register.tspackages/opencode/src/altimate/tools/altimate-core-correct.tspackages/opencode/src/altimate/tools/altimate-core-equivalence.tspackages/opencode/src/altimate/tools/altimate-core-fix.tspackages/opencode/src/altimate/tools/altimate-core-semantics.tspackages/opencode/src/altimate/tools/altimate-core-validate.tspackages/opencode/src/altimate/tools/finops-analyze-credits.tspackages/opencode/src/altimate/tools/schema-cache-status.tspackages/opencode/src/altimate/tools/sql-analyze.ts
✅ Files skipped from review due to trivial changes (1)
- packages/opencode/src/altimate/tools/altimate-core-equivalence.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/opencode/src/altimate/tools/schema-cache-status.ts
- packages/opencode/src/altimate/native/sql/register.ts
- packages/opencode/src/altimate/tools/altimate-core-validate.ts
- packages/opencode/src/altimate/tools/finops-analyze-credits.ts
- packages/opencode/src/altimate/tools/altimate-core-fix.ts
- packages/opencode/src/altimate/tools/altimate-core-correct.ts
packages/opencode/src/altimate/tools/altimate-core-semantics.ts
Outdated
Show resolved
Hide resolved
…el 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>
…s 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>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/opencode/src/altimate/tools/altimate-core-complete.ts (1)
24-28: Redundantas anycast.
datais already typed asRecord<string, any>(line 22), so theas anycast on line 24 is unnecessary—data.erroris valid.♻️ Suggested simplification
- const error = result.error ?? (data as any).error + const error = result.error ?? data.error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/tools/altimate-core-complete.ts` around lines 24 - 28, The code uses a redundant cast in the line defining error; replace the expression "result.error ?? (data as any).error" with "result.error ?? data.error" to remove the unnecessary "as any" cast—update the const error declaration that references result and data (near the return that calls formatComplete) so it relies on the existing Record<string, any> typing for data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/opencode/src/altimate/tools/altimate-core-complete.ts`:
- Around line 24-28: The code uses a redundant cast in the line defining error;
replace the expression "result.error ?? (data as any).error" with "result.error
?? data.error" to remove the unnecessary "as any" cast—update the const error
declaration that references result and data (near the return that calls
formatComplete) so it relies on the existing Record<string, any> typing for
data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 23d69b12-242b-45c2-a524-9c586644ab1a
📒 Files selected for processing (7)
packages/opencode/src/altimate/tools/altimate-core-complete.tspackages/opencode/src/altimate/tools/altimate-core-grade.tspackages/opencode/src/altimate/tools/finops-expensive-queries.tspackages/opencode/src/altimate/tools/finops-query-history.tspackages/opencode/src/altimate/tools/finops-unused-resources.tspackages/opencode/src/altimate/tools/finops-warehouse-advice.tspackages/opencode/src/altimate/tools/sql-explain.ts
✅ Files skipped from review due to trivial changes (1)
- packages/opencode/src/altimate/tools/sql-explain.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/opencode/src/altimate/tools/altimate-core-grade.ts
- packages/opencode/src/altimate/tools/finops-unused-resources.ts
- packages/opencode/src/altimate/tools/finops-query-history.ts
- packages/opencode/src/altimate/tools/finops-expensive-queries.ts
…d", 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>
There was a problem hiding this comment.
Actionable comments posted: 1
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/altimate-core.ts (1)
90-96:⚠️ Potential issue | 🟠 MajorDon't reuse
successfor “handler completed” while telemetry still treats it as “tool succeeded”.
packages/opencode/src/tool/tool.tsLines 119-146 only emits failure telemetry whenmetadata.success === false(or the tool throws) before it ever readsmetadata.error. With these handlers now forcingok(true, data), validate syntax errors and unfixable fix/correct results can surfacemetadata.errorwhile still being marked successful — the new E2E expectations already show that shape. Those runs will still be tracked as successful tool calls, so the concrete error text never reaches failure telemetry.Either keep
success: falsefor operation-level failures here, or introduce a separate “completed” flag and have the tool layer continue settingmetadata.success = falsewhenever it extracts an error.Also applies to: 187-196, 289-294
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/native/altimate-core.ts` around lines 90 - 96, The handler is marking operation-level failures as success by returning ok(true, data) (see register("altimate_core.validate") using core.validate and toData), which hides error text from telemetry; change the contract so that when the operation result contains an error or is a semantic/validation failure you return ok(false, data) (i.e. set success=false) instead of ok(true, data) so metadata.error is picked up by the tool telemetry; apply the same fix to the other handlers referenced (the blocks around lines 187-196 and 289-294) that currently return ok(true, data), and ensure the returned data still includes the concrete error text so metadata.error is populated.
🧹 Nitpick comments (1)
packages/opencode/test/altimate/e2e-tool-errors.ts (1)
165-167: Avoid hardcoding/tmpfor the nonexistent-schema case.The rest of the script is already temp-root agnostic. This one path makes the manual E2E check Unix-specific for no benefit.
Portable variant
await check("validate: with schema_path (nonexistent file) → error", async () => { - return validateTool.execute({ sql: testSql, schema_path: "/tmp/nonexistent-schema-abc123.json" }, stubCtx()) + const missingSchemaPath = path.join(tmpDir, "missing-schema.json") + return validateTool.execute({ sql: testSql, schema_path: missingSchemaPath }, stubCtx()) }, { metadataSuccess: false, noUnknownError: true })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/altimate/e2e-tool-errors.ts` around lines 165 - 167, The test currently hardcodes "/tmp/nonexistent-schema-abc123.json" which breaks portability; update the check that calls validateTool.execute inside the "validate: with schema_path (nonexistent file) → error" test to build the nonexistent path using the platform temp directory or the existing test tempRoot helper instead of "/tmp" (e.g., use Node's os.tmpdir() or the test suite's tempRoot variable and path.join) so the path is platform-agnostic while leaving the rest of the check and its options ({ metadataSuccess: false, noUnknownError: true }) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/test/altimate/tool-error-propagation.test.ts`:
- Around line 38-43: The helper telemetryWouldExtract currently returns
metadata.error whenever it's a string but ignores the same success gate used in
tool.ts; update telemetryWouldExtract to first check metadata?.success === false
(matching tool.ts) and only then extract and return typeof metadata.error ===
"string" ? metadata.error : "unknown error", otherwise return undefined or a
value indicating no error emitted; reference telemetryWouldExtract,
metadata.success and metadata.error so the test mirrors the production gating
logic.
---
Outside diff comments:
In `@packages/opencode/src/altimate/native/altimate-core.ts`:
- Around line 90-96: The handler is marking operation-level failures as success
by returning ok(true, data) (see register("altimate_core.validate") using
core.validate and toData), which hides error text from telemetry; change the
contract so that when the operation result contains an error or is a
semantic/validation failure you return ok(false, data) (i.e. set success=false)
instead of ok(true, data) so metadata.error is picked up by the tool telemetry;
apply the same fix to the other handlers referenced (the blocks around lines
187-196 and 289-294) that currently return ok(true, data), and ensure the
returned data still includes the concrete error text so metadata.error is
populated.
---
Nitpick comments:
In `@packages/opencode/test/altimate/e2e-tool-errors.ts`:
- Around line 165-167: The test currently hardcodes
"/tmp/nonexistent-schema-abc123.json" which breaks portability; update the check
that calls validateTool.execute inside the "validate: with schema_path
(nonexistent file) → error" test to build the nonexistent path using the
platform temp directory or the existing test tempRoot helper instead of "/tmp"
(e.g., use Node's os.tmpdir() or the test suite's tempRoot variable and
path.join) so the path is platform-agnostic while leaving the rest of the check
and its options ({ metadataSuccess: false, noUnknownError: true }) unchanged.
🪄 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: fb300377-cfb4-4df8-a0b5-bbf3f09fb94a
📒 Files selected for processing (3)
packages/opencode/src/altimate/native/altimate-core.tspackages/opencode/test/altimate/e2e-tool-errors.tspackages/opencode/test/altimate/tool-error-propagation.test.ts
- 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>
…ss` 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>
suryaiyer95
force-pushed
the
fix/ai-5975-tool-error-propagation
branch
from
79ff93a to
03b4dd9
Compare
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>
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
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
metadata.erroron failure pathsdata.errors[],data.unfixable_errors[],data.final_validation.errors[],data.validation_errors[])schema_path/schema_contextthroughsql_analyzetool (was completely disconnected from dispatcher handler)tsgotypecheck failure in sqlserver driver (@ts-expect-error→@ts-ignore)Root causes verified by direct execution
altimate_core_semantics(100%)Table 'users' not foundaltimate_core_equivalence(100%)Query A/B failed validation: Table not foundaltimate_core_validate(92%)Table 'users' not foundsql_analyze(95%)schema_path/schema_contextparams not wired throughaltimate_core_fix/correctNo automatic fix available for E000sql_explain(100%)A password must be specifiedfinops_*(100%)Query history not available for unknown warehousesTest plan
metadata.erroris populatedbun turbo typecheckpasses across all packages🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests