feat: [AI-5975] add sql_quality telemetry for issue prevention metrics#446
Merged
suryaiyer95 merged 6 commits intofix/ai-5975-tool-error-propagationfrom Mar 24, 2026
Conversation
![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.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Tool Runner
participant Tool as Tool.execute()
participant Aggregator as Telemetry.aggregateFindings()
participant Tracker as Telemetry.track()
Runner->>Tool: execute(args)
Tool-->>Runner: result { metadata, duration_ms }
alt result.metadata.findings non-empty & not soft-failure
Runner->>Aggregator: aggregateFindings(findings)
Aggregator-->>Runner: byCategoryCounts
Runner->>Tracker: track({ type: "sql_quality", finding_count, by_category, has_schema, dialect, duration_ms })
Tracker-->>Runner: ack
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/opencode/src/altimate/tools/altimate-core-validate.ts (1)
47-55: Classification logic is sound; consider future consolidation.The function correctly prioritizes "column not found" over "table not found" to avoid false positives when messages contain both keywords. The heuristic-based classification is reasonable for telemetry categorization.
If similar classification is needed elsewhere, consider extracting to a shared utility in the telemetry module.
🤖 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-validate.ts` around lines 47 - 55, The classifyValidationError heuristic in function classifyValidationError prioritizes "column not found" before "table not found" which is fine; refactor this logic into a shared telemetry utility (e.g., export a classifyValidationError function from a new/existing telemetry utils module) and replace any duplicate implementations across the codebase with an import to that single exported function; ensure the original function name (classifyValidationError) and behavior are preserved, export it from the telemetry module, update call sites to import it, and add a small unit test to cover the classification cases (missing_column, missing_table, syntax_error, type_mismatch, fallback validation_error).
🤖 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 25-28: The category fallback chain in the findings mapping is dead
code because semantic results from core.checkSemantics() do not include rule or
type; update the mapping that builds the const findings: Telemetry.Finding[]
from (data.issues ?? []) so that each finding uses a fixed category
"semantic_issue" (i.e. replace the expression issue.rule ?? issue.type ??
"semantic_issue" with the literal "semantic_issue") and keep the rest of the
mapping (severity, message, suggestion, explanation) unchanged.
In `@packages/opencode/test/altimate/sql-quality-telemetry.test.ts`:
- Around line 209-219: The test documents that no telemetry event should be
emitted for empty findings but doesn't assert it; add a mock/spy for the
telemetry emitter used by tool.ts (the telemetry client or event function that
would be called when findings exist) and in the "empty issues array produces
empty findings" test verify it was not invoked (e.g. use jest.spyOn or a mocked
telemetry client and expect(spy).not.toHaveBeenCalled()). Keep references to the
existing Telemetry.Finding/findings array and assert the emitter call count is
zero in this test.
- Around line 95-204: Tests are building Telemetry.Finding[] locally instead of
exercising the real mapping logic, which can mask regressions; update each test
(those creating findings inline and calling Telemetry.aggregateFindings) to call
the actual mapping path used in production — e.g., invoke the tool-specific
mapper or exported helper (the function that produces metadata.findings from
tool output) instead of constructing findings manually, then assert against
metadata.findings and/or pass that into Telemetry.aggregateFindings to validate
the real mapping behavior (refer to the mapping helpers used by the SQL
analyzer, validator, semantics fixer, fix tool, equivalence checker, and
correction tool that produce metadata.findings).
---
Nitpick comments:
In `@packages/opencode/src/altimate/tools/altimate-core-validate.ts`:
- Around line 47-55: The classifyValidationError heuristic in function
classifyValidationError prioritizes "column not found" before "table not found"
which is fine; refactor this logic into a shared telemetry utility (e.g., export
a classifyValidationError function from a new/existing telemetry utils module)
and replace any duplicate implementations across the codebase with an import to
that single exported function; ensure the original function name
(classifyValidationError) and behavior are preserved, export it from the
telemetry module, update call sites to import it, and add a small unit test to
cover the classification cases (missing_column, missing_table, syntax_error,
type_mismatch, fallback validation_error).
🪄 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: 81b48319-f58f-4da3-b81c-3e258e6008e8
📒 Files selected for processing (9)
packages/opencode/src/altimate/telemetry/index.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/sql-analyze.tspackages/opencode/src/tool/tool.tspackages/opencode/test/altimate/sql-quality-telemetry.test.ts
packages/opencode/src/altimate/tools/altimate-core-semantics.ts
Outdated
Show resolved
Hide resolved
Comment on lines
+95
to
+204
| describe("tool finding extraction patterns", () => { | ||
| test("sql_analyze issues map to findings", () => { | ||
| const issues = [ | ||
| { type: "select_star", severity: "warning", message: "...", recommendation: "...", confidence: "high" }, | ||
| { type: "cartesian_product", severity: "error", message: "...", recommendation: "...", confidence: "high" }, | ||
| ] | ||
| const findings: Telemetry.Finding[] = issues.map((i) => ({ | ||
| category: i.type, | ||
| severity: i.severity, | ||
| })) | ||
| expect(findings).toEqual([ | ||
| { category: "select_star", severity: "warning" }, | ||
| { category: "cartesian_product", severity: "error" }, | ||
| ]) | ||
| }) | ||
|
|
||
| test("validate errors map to findings with classification", () => { | ||
| const errors = [ | ||
| { message: "Table 'users' not found in schema" }, | ||
| { message: "Column 'email' not found in table 'orders'" }, | ||
| { message: "Syntax error near 'SELCT'" }, | ||
| ] | ||
| // Simulates classifyValidationError logic (column check before table check) | ||
| function classify(msg: string): string { | ||
| const lower = msg.toLowerCase() | ||
| if (lower.includes("column") && lower.includes("not found")) return "missing_column" | ||
| if (lower.includes("table") && lower.includes("not found")) return "missing_table" | ||
| if (lower.includes("syntax")) return "syntax_error" | ||
| return "validation_error" | ||
| } | ||
| const findings: Telemetry.Finding[] = errors.map((e) => ({ | ||
| category: classify(e.message), | ||
| severity: "error", | ||
| })) | ||
| const { by_category } = Telemetry.aggregateFindings(findings) | ||
| expect(by_category).toEqual({ | ||
| missing_table: 1, | ||
| missing_column: 1, | ||
| syntax_error: 1, | ||
| }) | ||
| }) | ||
|
|
||
| test("semantics issues preserve rule/type as category", () => { | ||
| const issues = [ | ||
| { rule: "cartesian_product", severity: "error", message: "..." }, | ||
| { type: "null_misuse", severity: "warning", message: "..." }, | ||
| { severity: "warning", message: "..." }, // no rule or type | ||
| ] | ||
| const findings: Telemetry.Finding[] = issues.map((i: any) => ({ | ||
| category: i.rule ?? i.type ?? "semantic_issue", | ||
| severity: i.severity ?? "warning", | ||
| })) | ||
| expect(findings).toEqual([ | ||
| { category: "cartesian_product", severity: "error" }, | ||
| { category: "null_misuse", severity: "warning" }, | ||
| { category: "semantic_issue", severity: "warning" }, | ||
| ]) | ||
| }) | ||
|
|
||
| test("fix tool produces fix_applied and unfixable_error categories", () => { | ||
| const data = { | ||
| fixes_applied: [{ description: "Fixed typo" }, { description: "Fixed reference" }], | ||
| unfixable_errors: [{ error: { message: "Cannot resolve" } }], | ||
| } | ||
| const findings: Telemetry.Finding[] = [] | ||
| for (const _ of data.fixes_applied) { | ||
| findings.push({ category: "fix_applied", severity: "warning" }) | ||
| } | ||
| for (const _ of data.unfixable_errors) { | ||
| findings.push({ category: "unfixable_error", severity: "error" }) | ||
| } | ||
| const { by_category } = Telemetry.aggregateFindings(findings) | ||
| expect(by_category).toEqual({ fix_applied: 2, unfixable_error: 1 }) | ||
| }) | ||
|
|
||
| test("equivalence differences produce findings only when not equivalent", () => { | ||
| // Equivalent — no findings | ||
| const equivData = { equivalent: true, differences: [] } | ||
| const equivFindings: Telemetry.Finding[] = [] | ||
| if (!equivData.equivalent && equivData.differences?.length) { | ||
| for (const _ of equivData.differences) { | ||
| equivFindings.push({ category: "equivalence_difference", severity: "warning" }) | ||
| } | ||
| } | ||
| expect(equivFindings).toEqual([]) | ||
|
|
||
| // Different — findings | ||
| const diffData = { equivalent: false, differences: [{ description: "..." }, { description: "..." }] } | ||
| const diffFindings: Telemetry.Finding[] = [] | ||
| if (!diffData.equivalent && diffData.differences?.length) { | ||
| for (const _ of diffData.differences) { | ||
| diffFindings.push({ category: "equivalence_difference", severity: "warning" }) | ||
| } | ||
| } | ||
| expect(diffFindings.length).toBe(2) | ||
| const { by_category } = Telemetry.aggregateFindings(diffFindings) | ||
| expect(by_category).toEqual({ equivalence_difference: 2 }) | ||
| }) | ||
|
|
||
| test("correct tool changes produce findings", () => { | ||
| const data = { changes: [{ description: "a" }, { description: "b" }] } | ||
| const findings: Telemetry.Finding[] = data.changes.map(() => ({ | ||
| category: "correction_applied", | ||
| severity: "warning", | ||
| })) | ||
| expect(findings.length).toBe(2) | ||
| const { by_category } = Telemetry.aggregateFindings(findings) | ||
| expect(by_category).toEqual({ correction_applied: 2 }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
These tests re-implement mapping logic instead of testing production mapping paths.
Lines 101, 125, 143, 160, 174, 184, and 196 construct Telemetry.Finding[] inline and then assert that same local transformation. If tool mapping logic regresses, this suite can still pass and give false confidence.
Please assert through the real mapping path (tool output → metadata.findings) or shared exported mapping helpers used by the tools.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/altimate/sql-quality-telemetry.test.ts` around lines
95 - 204, Tests are building Telemetry.Finding[] locally instead of exercising
the real mapping logic, which can mask regressions; update each test (those
creating findings inline and calling Telemetry.aggregateFindings) to call the
actual mapping path used in production — e.g., invoke the tool-specific mapper
or exported helper (the function that produces metadata.findings from tool
output) instead of constructing findings manually, then assert against
metadata.findings and/or pass that into Telemetry.aggregateFindings to validate
the real mapping behavior (refer to the mapping helpers used by the SQL
analyzer, validator, semantics fixer, fix tool, equivalence checker, and
correction tool that produce metadata.findings).
Contributor
Author
There was a problem hiding this comment.
Acknowledged. These unit tests verify aggregation logic and category classification. Integration tests through the full tool→telemetry path would require mocking the Dispatcher and are tracked as a follow-up.
There was a problem hiding this comment.
@suryaiyer95, understood — unit-level coverage for aggregateFindings and the classification helpers is a valid scope for this PR. Mocking the Dispatcher for end-to-end tool→telemetry path tests is a meaningful separate effort.
Would you like me to open a follow-up GitHub issue to track the integration test work (tool output → metadata.findings → sql_quality emission) so it doesn't get lost?
Comment on lines
+209
to
+219
| describe("no findings = no sql_quality event", () => { | ||
| test("empty issues array produces empty findings", () => { | ||
| const issues: any[] = [] | ||
| const findings: Telemetry.Finding[] = issues.map((i: any) => ({ | ||
| category: i.type, | ||
| severity: i.severity, | ||
| })) | ||
| expect(findings.length).toBe(0) | ||
| // tool.ts checks: Array.isArray(findings) && findings.length > 0 | ||
| // So no event would be emitted | ||
| }) |
There was a problem hiding this comment.
“No event emitted” is stated but not actually verified.
Lines 217-219 only document expected behavior in comments. There is no assertion against the telemetry emitter call path, so regressions in tool.ts emission guards would be missed.
Add an assertion that the telemetry client/event emitter is not called when findings are empty.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/altimate/sql-quality-telemetry.test.ts` around lines
209 - 219, The test documents that no telemetry event should be emitted for
empty findings but doesn't assert it; add a mock/spy for the telemetry emitter
used by tool.ts (the telemetry client or event function that would be called
when findings exist) and in the "empty issues array produces empty findings"
test verify it was not invoked (e.g. use jest.spyOn or a mocked telemetry client
and expect(spy).not.toHaveBeenCalled()). Keep references to the existing
Telemetry.Finding/findings array and assert the emitter call count is zero in
this test.
Contributor
Author
There was a problem hiding this comment.
Acknowledged. Verifying Telemetry.track is NOT called requires mocking the track function. Current tests verify the data flow (empty findings → no event). Full emission tests are a follow-up.
There was a problem hiding this comment.
@suryaiyer95, understood — the current tests do validate the prerequisite condition (empty findings array), and the emission guard in tool.ts (findings.length > 0) is the piece that remains untested at the call-site level. That's a fair split.
Would you like me to open a follow-up GitHub issue to track adding mock-based emission guard tests (e.g., spy on Telemetry.track and assert it is not called when findings are empty, and is called with the expected shape when findings are present)? That way the gap is captured and doesn't get lost.
There was a problem hiding this comment.
Pull request overview
Adds first-class telemetry for SQL issue-prevention metrics by introducing a new sql_quality event and wiring multiple SQL-related tools to report lightweight findings that get aggregated and emitted centrally.
Changes:
- Adds
Telemetry.Finding+Telemetry.aggregateFindings()and a newsql_qualityevent type to the telemetry schema. - Emits
sql_qualityfrom the centralized tool wrapper (tool.ts) when tools returnmetadata.findings. - Updates several SQL tools to populate
metadata.findings(plusdialect/has_schema) and adds a focused unit test suite for aggregation/mapping behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/opencode/src/altimate/telemetry/index.ts | Adds sql_quality event type + Finding interface and aggregation helper. |
| packages/opencode/src/tool/tool.ts | Centralized emission of sql_quality based on result.metadata.findings. |
| packages/opencode/src/altimate/tools/sql-analyze.ts | Produces findings from analyze issues and adds dialect/schema metadata. |
| packages/opencode/src/altimate/tools/altimate-core-validate.ts | Produces findings from validation errors via classification helper. |
| packages/opencode/src/altimate/tools/altimate-core-semantics.ts | Produces findings from semantic issues. |
| packages/opencode/src/altimate/tools/altimate-core-fix.ts | Produces findings from applied fixes and unfixable errors. |
| packages/opencode/src/altimate/tools/altimate-core-correct.ts | Produces findings from correction changes. |
| packages/opencode/src/altimate/tools/altimate-core-equivalence.ts | Produces findings when queries are not equivalent. |
| packages/opencode/test/altimate/sql-quality-telemetry.test.ts | Adds unit tests for aggregation and mapping patterns (but not end-to-end emission). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+159
to
+176
| // altimate_change start — emit sql_quality when tools report findings | ||
| const findings = result.metadata?.findings as Telemetry.Finding[] | undefined | ||
| if (Array.isArray(findings) && findings.length > 0) { | ||
| const { by_severity, by_category } = Telemetry.aggregateFindings(findings) | ||
| Telemetry.track({ | ||
| type: "sql_quality", | ||
| timestamp: Date.now(), | ||
| session_id: ctx.sessionID, | ||
| tool_name: id, | ||
| tool_category: toolCategory, | ||
| finding_count: findings.length, | ||
| by_severity: JSON.stringify(by_severity), | ||
| by_category: JSON.stringify(by_category), | ||
| has_schema: result.metadata?.has_schema ?? false, | ||
| dialect: (result.metadata?.dialect as string) ?? "unknown", | ||
| duration_ms: durationMs, | ||
| }) | ||
| } |
There was a problem hiding this comment.
sql_quality is emitted whenever metadata.findings is non-empty, even when the tool is a soft failure (metadata.success === false). That can double-count failures (you already emit core_failure) and contradicts the intended “successfully detect issues” semantics. Consider gating this block on !isSoftFailure (or result.metadata?.success !== false) so only successful tool executions produce sql_quality events.
Contributor
Author
There was a problem hiding this comment.
Already gated. Line 164: if (!isSoftFailure && Array.isArray(findings) && findings.length > 0) — soft failures are excluded.
Comment on lines
10
to
+40
| parameters: z.object({ | ||
| sql: z.string().describe("SQL query to analyze"), | ||
| dialect: z | ||
| .string() | ||
| .optional() | ||
| .default("snowflake") | ||
| .describe("SQL dialect (snowflake, postgres, bigquery, duckdb, etc.)"), | ||
| }), | ||
| async execute(args, ctx) { | ||
| try { | ||
| const result = await Dispatcher.call("sql.analyze", { | ||
| sql: args.sql, | ||
| dialect: args.dialect, | ||
| }) | ||
|
|
||
| // altimate_change start — sql quality findings for telemetry | ||
| const findings: Telemetry.Finding[] = result.issues.map((issue) => ({ | ||
| category: issue.type, | ||
| severity: issue.severity, | ||
| })) | ||
| // altimate_change end | ||
| return { | ||
| title: `Analyze: ${result.error ? "PARSE ERROR" : `${result.issue_count} issue${result.issue_count !== 1 ? "s" : ""}`} [${result.confidence}]`, | ||
| metadata: { | ||
| success: result.success, | ||
| issueCount: result.issue_count, | ||
| confidence: result.confidence, | ||
| dialect: args.dialect, | ||
| has_schema: false, | ||
| ...(result.error && { error: result.error }), | ||
| ...(findings.length > 0 && { findings }), |
There was a problem hiding this comment.
sql_analyze doesn’t accept or forward schema_path / schema_context to the dispatcher (and hard-codes has_schema: false), even though the native sql.analyze handler supports schema inputs for better issue detection. This prevents the “schema availability” correlation described in the PR and likely reduces finding quality. Add optional schema inputs to the tool parameters, pass them to Dispatcher.call, and set has_schema based on whether schema input was provided.
Contributor
Author
There was a problem hiding this comment.
Separate concern. Tracked in #455 (add dialect/schema parameters to tools that don't have them yet).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/opencode/src/altimate/tools/altimate-core-validate.ts (1)
46-54: Use shared category constants to avoid typo-driven metric splits.These category strings are telemetry keys; a typo in any tool will silently create a new bucket. Centralizing constants here (or in telemetry module) reduces drift.
Proposed diff
+const ValidationFindingCategory = { + MissingColumn: "missing_column", + MissingTable: "missing_table", + SyntaxError: "syntax_error", + TypeMismatch: "type_mismatch", + ValidationError: "validation_error", +} as const + function classifyValidationError(message: string): string { const lower = message.toLowerCase() // Column check before table — "column not found in table" would match both - if (lower.includes("column") && lower.includes("not found")) return "missing_column" - if (lower.includes("table") && lower.includes("not found")) return "missing_table" - if (lower.includes("syntax")) return "syntax_error" - if (lower.includes("type")) return "type_mismatch" - return "validation_error" + if (lower.includes("column") && lower.includes("not found")) return ValidationFindingCategory.MissingColumn + if (lower.includes("table") && lower.includes("not found")) return ValidationFindingCategory.MissingTable + if (lower.includes("syntax")) return ValidationFindingCategory.SyntaxError + if (lower.includes("type")) return ValidationFindingCategory.TypeMismatch + return ValidationFindingCategory.ValidationError }🤖 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-validate.ts` around lines 46 - 54, classifyValidationError currently returns hard-coded telemetry category strings which can lead to typo-driven metric splits; replace those literals with shared constants exported from the telemetry/constants module (or create a shared VALIDATION_CATEGORIES object and export it) and import them into this file, then return VALIDATION_CATEGORIES.MISSING_COLUMN, .MISSING_TABLE, .SYNTAX_ERROR, .TYPE_MISMATCH, and .VALIDATION_ERROR from classifyValidationError instead of raw strings while keeping the existing column-before-table check and function signature intact.packages/opencode/src/tool/tool.ts (1)
163-171: Makeby_categoryserialization deterministic.Line 171 serializes an object whose key insertion order depends on input finding order. Identical category counts can produce different JSON strings, which increases custom-dimension cardinality and makes telemetry aggregation noisier.
Proposed diff
- const by_category = Telemetry.aggregateFindings(findings) + const by_category = Telemetry.aggregateFindings(findings) + const by_category_stable = Object.fromEntries( + Object.entries(by_category).sort(([a], [b]) => a.localeCompare(b)), + ) Telemetry.track({ type: "sql_quality", timestamp: Date.now(), session_id: ctx.sessionID, tool_name: id, tool_category: toolCategory, finding_count: findings.length, - by_category: JSON.stringify(by_category), + by_category: JSON.stringify(by_category_stable), has_schema: result.metadata?.has_schema ?? false, dialect: (result.metadata?.dialect as string) ?? "unknown", duration_ms: durationMs, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/tool/tool.ts` around lines 163 - 171, The by_category object produced by Telemetry.aggregateFindings is being JSON.stringified with non-deterministic key order, causing inconsistent telemetry; fix by producing a deterministic serialization before passing to Telemetry.track: create an ordered representation of by_category by sorting its keys (use Object.keys(by_category).sort() or similar) and building a new object/array in that key order, then JSON.stringify that ordered structure and pass it as by_category in the Telemetry.track call (references: variable by_category, Telemetry.aggregateFindings, and the Telemetry.track invocation).
🤖 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-validate.ts`:
- Around line 24-26: The code currently assumes data.errors is an array when
building findings; guard against non-array values by checking
Array.isArray(data?.errors) before mapping (or default to an empty array) so
classifyValidationError is only called for real error objects—update the
construction of findings (the const findings: Telemetry.Finding[] = ... code) to
use Array.isArray(data?.errors) ? data.errors : [] and map that result, ensuring
the mapped item uses err.message safely.
---
Nitpick comments:
In `@packages/opencode/src/altimate/tools/altimate-core-validate.ts`:
- Around line 46-54: classifyValidationError currently returns hard-coded
telemetry category strings which can lead to typo-driven metric splits; replace
those literals with shared constants exported from the telemetry/constants
module (or create a shared VALIDATION_CATEGORIES object and export it) and
import them into this file, then return VALIDATION_CATEGORIES.MISSING_COLUMN,
.MISSING_TABLE, .SYNTAX_ERROR, .TYPE_MISMATCH, and .VALIDATION_ERROR from
classifyValidationError instead of raw strings while keeping the existing
column-before-table check and function signature intact.
In `@packages/opencode/src/tool/tool.ts`:
- Around line 163-171: The by_category object produced by
Telemetry.aggregateFindings is being JSON.stringified with non-deterministic key
order, causing inconsistent telemetry; fix by producing a deterministic
serialization before passing to Telemetry.track: create an ordered
representation of by_category by sorting its keys (use
Object.keys(by_category).sort() or similar) and building a new object/array in
that key order, then JSON.stringify that ordered structure and pass it as
by_category in the Telemetry.track call (references: variable by_category,
Telemetry.aggregateFindings, and the Telemetry.track invocation).
🪄 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: cb7f37d1-ee6f-4f39-b993-895793893cfa
📒 Files selected for processing (9)
packages/opencode/src/altimate/telemetry/index.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/sql-analyze.tspackages/opencode/src/tool/tool.tspackages/opencode/test/altimate/sql-quality-telemetry.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/opencode/src/altimate/tools/altimate-core-fix.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/opencode/src/altimate/tools/sql-analyze.ts
- packages/opencode/src/altimate/tools/altimate-core-correct.ts
- packages/opencode/src/altimate/tools/altimate-core-semantics.ts
- packages/opencode/src/altimate/telemetry/index.ts
- packages/opencode/src/altimate/tools/altimate-core-equivalence.ts
- packages/opencode/test/altimate/sql-quality-telemetry.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/opencode/src/altimate/tools/altimate-core-policy.ts (1)
43-43: Consider breaking this line for readability.The error return is functionally correct but quite long. A minor formatting improvement would help maintainability.
✨ Suggested refactor
} catch (e) { const msg = e instanceof Error ? e.message : String(e) - return { title: "Policy: ERROR", metadata: { success: false, pass: false, has_schema: hasSchema, dialect: "snowflake" }, output: `Failed: ${msg}` } + return { + title: "Policy: ERROR", + metadata: { success: false, pass: false, has_schema: hasSchema, dialect: "snowflake" }, + 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-policy.ts` at line 43, The long single-line return in the policy error path should be reformatted for readability: break the object literal returned by the function into multiple lines so each property (title, metadata with its nested keys success/pass/has_schema/dialect, and output) sits on its own line and keep the template string for output as `Failed: ${msg}`; locate the return that currently returns { title: "Policy: ERROR", metadata: { success: false, pass: false, has_schema: hasSchema, dialect: "snowflake" }, output: `Failed: ${msg}` } (use the surrounding function in altimate-core-policy and the hasSchema/msg identifiers) and split it into a multi-line object literal with proper indentation.packages/opencode/src/altimate/tools/impact-analysis.ts (1)
133-144: Unused loop variablesdandt.The loops iterate solely to push a fixed category string per element, leaving the loop variables unused. Consider a more declarative approach:
♻️ Suggested refactor using Array.from
// altimate_change start — sql quality findings for telemetry const findings: Telemetry.Finding[] = [] if (totalAffected > 0) { findings.push({ category: `impact_${severity.toLowerCase()}` }) - for (const d of direct) { - findings.push({ category: "impact_direct_dependent" }) - } - for (const t of transitive) { - findings.push({ category: "impact_transitive_dependent" }) - } + findings.push( + ...Array.from({ length: direct.length }, () => ({ category: "impact_direct_dependent" as const })), + ...Array.from({ length: transitive.length }, () => ({ category: "impact_transitive_dependent" as const })), + ) } // altimate_change end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/tools/impact-analysis.ts` around lines 133 - 144, The loops in impact-analysis.ts create Telemetry.Finding entries but never use the loop variables `d` and `t`; replace the imperative for-loops that iterate `for (const d of direct)` and `for (const t of transitive)` with a declarative mapping or bulk push (e.g., use `direct.map` / `transitive.map` or generate arrays of the appropriate length) to append the repeated categories to the `findings` array when `totalAffected > 0`, keeping the existing `findings`, `severity`, `totalAffected`, `direct`, and `transitive` symbols.packages/opencode/src/altimate/tools/sql-optimize.ts (1)
60-60: Consider computinghas_schemaconsistently in the error path.In the success path,
hasSchemais computed fromargs.schema_context, but the error path hardcodeshas_schema: false. If an error occurs for reasons unrelated to schema (e.g., network issues), the telemetry would incorrectly indicate no schema was provided.♻️ Suggested fix
} catch (e) { const msg = e instanceof Error ? e.message : String(e) + const hasSchema = !!(args.schema_context && Object.keys(args.schema_context).length > 0) return { title: "Optimize: ERROR", - metadata: { success: false, suggestionCount: 0, antiPatternCount: 0, hasOptimizedSql: false, confidence: "unknown", has_schema: false, dialect: args.dialect }, + metadata: { success: false, suggestionCount: 0, antiPatternCount: 0, hasOptimizedSql: false, confidence: "unknown", has_schema: hasSchema, dialect: args.dialect }, output: `Failed to optimize SQL: ${msg}\n\nCheck your connection configuration and try again.`, } }🤖 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` at line 60, The error telemetry currently hardcodes metadata.has_schema: false instead of reflecting whether a schema was provided; update the error path in the metadata object to compute has_schema the same way the success path does (e.g., derive hasSchema from args.schema_context or the same helper/variable used in the success branch) so that metadata.has_schema accurately mirrors presence of schema even when an error occurs (look for the metadata object creation in sql-optimize.ts and the success-path computation of hasSchema/args.schema_context to reuse).packages/opencode/src/altimate/tools/schema-diff.ts (1)
35-39: Guard findings derivation to successful diffs only.Line 36 derives findings even when
result.successis false. Adding a success guard keepssql_qualitymetrics strictly tied to valid diff results and avoids future telemetry contamination if partial changes are ever returned on parse failures.Suggested patch
- const findings: Telemetry.Finding[] = result.changes.map((c) => ({ - category: c.change_type ?? (c.severity === "breaking" ? "breaking_change" : "schema_change"), - })) + const findings: Telemetry.Finding[] = result.success + ? result.changes.map((c) => ({ + category: c.change_type ?? (c.severity === "breaking" ? "breaking_change" : "schema_change"), + })) + : []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/tools/schema-diff.ts` around lines 35 - 39, The findings array is populated from result.changes even when result.success is false, which can pollute sql_quality telemetry; guard the derivation by checking result.success before mapping to Telemetry.Finding[] (i.e., only populate findings when result.success is true and result.changes exists), otherwise set findings to an empty array or skip emitting sql_quality metrics; update the logic around result.changes → findings and ensure any downstream use of findings respects this guarded value.
🤖 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-check.ts`:
- Around line 23-36: The telemetry mapper currently reads data.pii?.findings but
the altimate_core.check dispatcher only exposes top-level validation, lint, and
safety fields, so pii_detected is never reported; fix by either (A) updating the
native response in altimate-core.ts (the altimate_core.check dispatcher) to
include a top-level pii field mirroring where PII is produced, or (B) change the
mapper in altimate-core-check.ts (the loop that pushes { category:
"pii_detected" } from data.pii?.findings) to read PII from the actual location
the native response returns (e.g., move the mapping to data.safety or the
correct property name), and ensure the unique symbol data.pii?.findings or the
dispatcher export is updated consistently so pii_detected can be emitted.
---
Nitpick comments:
In `@packages/opencode/src/altimate/tools/altimate-core-policy.ts`:
- Line 43: The long single-line return in the policy error path should be
reformatted for readability: break the object literal returned by the function
into multiple lines so each property (title, metadata with its nested keys
success/pass/has_schema/dialect, and output) sits on its own line and keep the
template string for output as `Failed: ${msg}`; locate the return that currently
returns { title: "Policy: ERROR", metadata: { success: false, pass: false,
has_schema: hasSchema, dialect: "snowflake" }, output: `Failed: ${msg}` } (use
the surrounding function in altimate-core-policy and the hasSchema/msg
identifiers) and split it into a multi-line object literal with proper
indentation.
In `@packages/opencode/src/altimate/tools/impact-analysis.ts`:
- Around line 133-144: The loops in impact-analysis.ts create Telemetry.Finding
entries but never use the loop variables `d` and `t`; replace the imperative
for-loops that iterate `for (const d of direct)` and `for (const t of
transitive)` with a declarative mapping or bulk push (e.g., use `direct.map` /
`transitive.map` or generate arrays of the appropriate length) to append the
repeated categories to the `findings` array when `totalAffected > 0`, keeping
the existing `findings`, `severity`, `totalAffected`, `direct`, and `transitive`
symbols.
In `@packages/opencode/src/altimate/tools/schema-diff.ts`:
- Around line 35-39: The findings array is populated from result.changes even
when result.success is false, which can pollute sql_quality telemetry; guard the
derivation by checking result.success before mapping to Telemetry.Finding[]
(i.e., only populate findings when result.success is true and result.changes
exists), otherwise set findings to an empty array or skip emitting sql_quality
metrics; update the logic around result.changes → findings and ensure any
downstream use of findings respects this guarded value.
In `@packages/opencode/src/altimate/tools/sql-optimize.ts`:
- Line 60: The error telemetry currently hardcodes metadata.has_schema: false
instead of reflecting whether a schema was provided; update the error path in
the metadata object to compute has_schema the same way the success path does
(e.g., derive hasSchema from args.schema_context or the same helper/variable
used in the success branch) so that metadata.has_schema accurately mirrors
presence of schema even when an error occurs (look for the metadata object
creation in sql-optimize.ts and the success-path computation of
hasSchema/args.schema_context to reuse).
🪄 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: 7cb26ac9-0934-4cbc-adf1-673f370d9865
📒 Files selected for processing (9)
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-policy.tspackages/opencode/src/altimate/tools/impact-analysis.tspackages/opencode/src/altimate/tools/schema-diff.tspackages/opencode/src/altimate/tools/sql-analyze.tspackages/opencode/src/altimate/tools/sql-optimize.tspackages/opencode/test/altimate/sql-quality-telemetry.test.ts
✅ Files skipped from review due to trivial changes (2)
- packages/opencode/src/altimate/native/types.ts
- packages/opencode/test/altimate/sql-quality-telemetry.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/src/altimate/tools/sql-analyze.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-fix.ts`:
- Around line 26-32: The findings currently created in the loop lack a severity
field required by the sql_quality pipeline; update the objects pushed into the
findings array so each Telemetry.Finding includes a severity (e.g., set
severity: "low" for successful fixes generated from data.fixes_applied or
data.changes, and severity: "high" for items from data.unfixable_errors) so
aggregation by severity works; modify the two push sites where findings.push({
category: "fix_applied" }) and findings.push({ category: "unfixable_error" })
are created to include the appropriate severity property.
🪄 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: 1d954c1d-f35d-4a8e-a24c-409947cbb547
📒 Files selected for processing (5)
packages/opencode/src/altimate/tools/altimate-core-equivalence.tspackages/opencode/src/altimate/tools/altimate-core-fix.tspackages/opencode/src/altimate/tools/altimate-core-policy.tspackages/opencode/src/altimate/tools/altimate-core-semantics.tspackages/opencode/src/altimate/tools/altimate-core-validate.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/opencode/src/altimate/tools/altimate-core-semantics.ts
- packages/opencode/src/altimate/tools/altimate-core-validate.ts
- packages/opencode/src/altimate/tools/altimate-core-policy.ts
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>
…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>
…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>
…nding 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>
suryaiyer95
changed the base branch from
main
to
fix/ai-5975-tool-error-propagation
suryaiyer95
force-pushed
the
feat/ai-5975-sql-quality-telemetry
branch
from
faab17f to
38914b5
Compare
… 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>
…e 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>
suryaiyer95
merged commit fe1d8a1
into
fix/ai-5975-tool-error-propagation
4 checks passed
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
sql_qualitytelemetry event to App Insights, emitted whenever tools successfully detect SQL issuesmissing_table,cartesian_product,select_star) and severity (error,warning,info)sql_analyze,altimate_core_validate,altimate_core_semantics,altimate_core_fix,altimate_core_correct,altimate_core_equivalenceWhy
These findings are a goldmine of "issues prevented before production" data. AI-generated SQL can have anti-patterns, missing references, and semantic issues — our tools catch them, but we had no visibility into what was being caught or how often. This enables:
Architecture
Centralized: tools only populate
metadata.findings— emission logic lives in one place (tool.ts).PII-safe: only category names and severity strings reach telemetry, never SQL content.
KQL examples
Test plan
sqlserver.tsandtracing-viewer-e2e.test.tserrors unchanged)sql_qualityevents appear in App Insights after deploymentCompanion PRs
successsemantics so findings aren't misreported as failuresThis PR is independent — can be merged in any order relative to the companions.
🤖 Generated with Claude Code
Summary by CodeRabbit