refactor: [AI-5975] drop by_severity from sql_quality telemetry, address PR review

suryaiyer95 · claude · suryaiyer95 · commit d930e3c17585 · 2026-03-24T15:05:18.000-07:00
- 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 &lt;noreply@anthropic.com&gt;
diff --git a/packages/opencode/src/altimate/telemetry/index.ts b/packages/opencode/src/altimate/telemetry/index.ts
@@ -413,8 +413,6 @@ export namespace Telemetry {
         tool_name: string
         tool_category: string
         finding_count: number
-        /** JSON-encoded Record<string, number> — count per severity level */
-        by_severity: string
         /** JSON-encoded Record<string, number> — count per issue category */
         by_category: string
         has_schema: boolean
@@ -792,24 +790,18 @@ export namespace Telemetry {
   }
 
   // altimate_change start — sql quality telemetry types
-  /** Lightweight finding record for quality telemetry. Only category/severity — never SQL content. */
+  /** Lightweight finding record for quality telemetry. Only category — never SQL content. */
   export interface Finding {
     category: string
-    severity: string
   }
 
-  /** Aggregate an array of findings into counts suitable for the sql_quality event. */
-  export function aggregateFindings(findings: Finding[]): {
-    by_severity: Record<string, number>
-    by_category: Record<string, number>
-  } {
-    const by_severity: Record<string, number> = {}
+  /** Aggregate an array of findings into category counts suitable for the sql_quality event. */
+  export function aggregateFindings(findings: Finding[]): Record<string, number> {
     const by_category: Record<string, number> = {}
     for (const f of findings) {
-      by_severity[f.severity] = (by_severity[f.severity] ?? 0) + 1
       by_category[f.category] = (by_category[f.category] ?? 0) + 1
     }
-    return { by_severity, by_category }
+    return by_category
   }
   // altimate_change end
 
diff --git a/packages/opencode/src/tool/tool.ts b/packages/opencode/src/tool/tool.ts
@@ -159,17 +159,17 @@ export namespace Tool {
               })
             }
             // altimate_change start — emit sql_quality when tools report findings
+            // Only emit for successful tool runs — soft failures already emit core_failure
             const findings = result.metadata?.findings as Telemetry.Finding[] | undefined
-            if (Array.isArray(findings) && findings.length > 0) {
-              const { by_severity, by_category } = Telemetry.aggregateFindings(findings)
+            if (!isSoftFailure && Array.isArray(findings) && findings.length > 0) {
+              const 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",
diff --git a/packages/opencode/test/altimate/sql-quality-telemetry.test.ts b/packages/opencode/test/altimate/sql-quality-telemetry.test.ts
@@ -1,8 +1,10 @@
 /**
  * SQL Quality Telemetry Tests
  *
- * Verifies that the `sql_quality` event is emitted with correct aggregations
- * when tools report findings, and is NOT emitted when there are no findings.
+ * Verifies the aggregation logic, event payload shape, and finding
+ * extraction patterns used for the `sql_quality` telemetry event, and
+ * that scenarios with no findings result in empty finding arrays (the
+ * condition used by tool.ts to decide not to emit the event).
  */
 
 import { describe, expect, test } from "bun:test"
@@ -12,99 +14,91 @@ import { Telemetry } from "../../src/altimate/telemetry"
 // 1. aggregateFindings
 // ---------------------------------------------------------------------------
 describe("Telemetry.aggregateFindings", () => {
-  test("aggregates findings by severity and category", () => {
+  test("aggregates findings by category", () => {
     const findings: Telemetry.Finding[] = [
-      { category: "missing_table", severity: "error" },
-      { category: "missing_column", severity: "error" },
-      { category: "cartesian_product", severity: "warning" },
-      { category: "missing_table", severity: "error" },
+      { category: "missing_table" },
+      { category: "missing_column" },
+      { category: "lint" },
+      { category: "missing_table" },
     ]
     const result = Telemetry.aggregateFindings(findings)
-    expect(result.by_severity).toEqual({ error: 3, warning: 1 })
-    expect(result.by_category).toEqual({
+    expect(result).toEqual({
       missing_table: 2,
       missing_column: 1,
-      cartesian_product: 1,
+      lint: 1,
     })
   })
 
-  test("returns empty objects for empty findings", () => {
+  test("returns empty object for empty findings", () => {
     const result = Telemetry.aggregateFindings([])
-    expect(result.by_severity).toEqual({})
-    expect(result.by_category).toEqual({})
+    expect(result).toEqual({})
   })
 
   test("handles single finding", () => {
     const findings: Telemetry.Finding[] = [
-      { category: "syntax_error", severity: "error" },
+      { category: "syntax_error" },
     ]
     const result = Telemetry.aggregateFindings(findings)
-    expect(result.by_severity).toEqual({ error: 1 })
-    expect(result.by_category).toEqual({ syntax_error: 1 })
+    expect(result).toEqual({ syntax_error: 1 })
   })
 
-  test("handles all same category different severities", () => {
+  test("handles all same category", () => {
     const findings: Telemetry.Finding[] = [
-      { category: "select_star", severity: "warning" },
-      { category: "select_star", severity: "info" },
-      { category: "select_star", severity: "error" },
+      { category: "lint" },
+      { category: "lint" },
+      { category: "lint" },
     ]
     const result = Telemetry.aggregateFindings(findings)
-    expect(result.by_severity).toEqual({ warning: 1, info: 1, error: 1 })
-    expect(result.by_category).toEqual({ select_star: 3 })
+    expect(result).toEqual({ lint: 3 })
   })
 })
 
 // ---------------------------------------------------------------------------
 // 2. sql_quality event shape validation
 // ---------------------------------------------------------------------------
 describe("sql_quality event shape", () => {
-  test("by_severity and by_category serialize to valid JSON strings", () => {
+  test("by_category serializes to valid JSON string", () => {
     const findings: Telemetry.Finding[] = [
-      { category: "anti_pattern", severity: "warning" },
-      { category: "anti_pattern", severity: "warning" },
-      { category: "performance_issue", severity: "info" },
+      { category: "lint" },
+      { category: "lint" },
+      { category: "safety" },
     ]
-    const { by_severity, by_category } = Telemetry.aggregateFindings(findings)
-    const severityJson = JSON.stringify(by_severity)
-    const categoryJson = JSON.stringify(by_category)
+    const by_category = Telemetry.aggregateFindings(findings)
+    const json = JSON.stringify(by_category)
 
     // Should round-trip through JSON
-    expect(JSON.parse(severityJson)).toEqual({ warning: 2, info: 1 })
-    expect(JSON.parse(categoryJson)).toEqual({ anti_pattern: 2, performance_issue: 1 })
+    expect(JSON.parse(json)).toEqual({ lint: 2, safety: 1 })
   })
 
   test("aggregated counts match finding_count", () => {
     const findings: Telemetry.Finding[] = [
-      { category: "a", severity: "error" },
-      { category: "b", severity: "warning" },
-      { category: "c", severity: "error" },
-      { category: "a", severity: "info" },
+      { category: "a" },
+      { category: "b" },
+      { category: "c" },
+      { category: "a" },
     ]
-    const { by_severity, by_category } = Telemetry.aggregateFindings(findings)
-    const totalBySeverity = Object.values(by_severity).reduce((a, b) => a + b, 0)
-    const totalByCategory = Object.values(by_category).reduce((a, b) => a + b, 0)
-    expect(totalBySeverity).toBe(findings.length)
-    expect(totalByCategory).toBe(findings.length)
+    const by_category = Telemetry.aggregateFindings(findings)
+    const total = Object.values(by_category).reduce((a, b) => a + b, 0)
+    expect(total).toBe(findings.length)
   })
 })
 
 // ---------------------------------------------------------------------------
 // 3. Finding extraction patterns (validates what tools produce)
 // ---------------------------------------------------------------------------
 describe("tool finding extraction patterns", () => {
-  test("sql_analyze issues map to findings", () => {
+  test("sql_analyze issues map to findings via issue.type", () => {
+    // issue.type is coarse: "lint", "semantic", "safety"
     const issues = [
-      { type: "select_star", severity: "warning", message: "...", recommendation: "...", confidence: "high" },
-      { type: "cartesian_product", severity: "error", message: "...", recommendation: "...", confidence: "high" },
+      { type: "lint", severity: "warning", message: "...", recommendation: "...", confidence: "high" },
+      { type: "safety", severity: "high", 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" },
+      { category: "lint" },
+      { category: "safety" },
     ])
   })
 
@@ -124,31 +118,32 @@ describe("tool finding extraction patterns", () => {
     }
     const findings: Telemetry.Finding[] = errors.map((e) => ({
       category: classify(e.message),
-      severity: "error",
     }))
-    const { by_category } = Telemetry.aggregateFindings(findings)
+    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", () => {
+  test("semantics issues all map to semantic_issue category", () => {
+    // Semantic findings don't have rule/type — always "semantic_issue"
     const issues = [
-      { rule: "cartesian_product", severity: "error", message: "..." },
-      { type: "null_misuse", severity: "warning", message: "..." },
-      { severity: "warning", message: "..." }, // no rule or type
+      { severity: "error", message: "..." },
+      { severity: "warning", message: "..." },
+      { severity: "warning", message: "..." },
     ]
-    const findings: Telemetry.Finding[] = issues.map((i: any) => ({
-      category: i.rule ?? i.type ?? "semantic_issue",
-      severity: i.severity ?? "warning",
+    const findings: Telemetry.Finding[] = issues.map(() => ({
+      category: "semantic_issue",
     }))
     expect(findings).toEqual([
-      { category: "cartesian_product", severity: "error" },
-      { category: "null_misuse", severity: "warning" },
-      { category: "semantic_issue", severity: "warning" },
+      { category: "semantic_issue" },
+      { category: "semantic_issue" },
+      { category: "semantic_issue" },
     ])
+    const by_category = Telemetry.aggregateFindings(findings)
+    expect(by_category).toEqual({ semantic_issue: 3 })
   })
 
   test("fix tool produces fix_applied and unfixable_error categories", () => {
@@ -158,12 +153,12 @@ describe("tool finding extraction patterns", () => {
     }
     const findings: Telemetry.Finding[] = []
     for (const _ of data.fixes_applied) {
-      findings.push({ category: "fix_applied", severity: "warning" })
+      findings.push({ category: "fix_applied" })
     }
     for (const _ of data.unfixable_errors) {
-      findings.push({ category: "unfixable_error", severity: "error" })
+      findings.push({ category: "unfixable_error" })
     }
-    const { by_category } = Telemetry.aggregateFindings(findings)
+    const by_category = Telemetry.aggregateFindings(findings)
     expect(by_category).toEqual({ fix_applied: 2, unfixable_error: 1 })
   })
 
@@ -173,7 +168,7 @@ describe("tool finding extraction patterns", () => {
     const equivFindings: Telemetry.Finding[] = []
     if (!equivData.equivalent && equivData.differences?.length) {
       for (const _ of equivData.differences) {
-        equivFindings.push({ category: "equivalence_difference", severity: "warning" })
+        equivFindings.push({ category: "equivalence_difference" })
       }
     }
     expect(equivFindings).toEqual([])
@@ -183,22 +178,21 @@ describe("tool finding extraction patterns", () => {
     const diffFindings: Telemetry.Finding[] = []
     if (!diffData.equivalent && diffData.differences?.length) {
       for (const _ of diffData.differences) {
-        diffFindings.push({ category: "equivalence_difference", severity: "warning" })
+        diffFindings.push({ category: "equivalence_difference" })
       }
     }
     expect(diffFindings.length).toBe(2)
-    const { by_category } = Telemetry.aggregateFindings(diffFindings)
+    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)
+    const by_category = Telemetry.aggregateFindings(findings)
     expect(by_category).toEqual({ correction_applied: 2 })
   })
 })
@@ -211,18 +205,16 @@ describe("no findings = no sql_quality event", () => {
     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
+    // tool.ts guards: !isSoftFailure && Array.isArray(findings) && findings.length > 0
     // So no event would be emitted
   })
 
   test("valid SQL with no errors produces no findings", () => {
     const data = { valid: true, errors: [] }
     const findings: Telemetry.Finding[] = (data.errors ?? []).map(() => ({
       category: "validation_error",
-      severity: "error",
     }))
     expect(findings.length).toBe(0)
   })
