Address PR #174 review: fix dimension scoring, TOOL_ERROR counting, and module constants

- H1: Skip parse errors and unknown categories in _compute_dimension_averages
  instead of scoring them as 0 (which inflated averages downward)
- H2: Default unknown categories to ❓ instead of ✅ in scorecard icons
- H3: Count TOOL_ERROR spans as tool attempts in _count_trace_metrics
- L3: Lift _SCORECARD_ICONS to module level (was duplicated in function)
- L7: Extract _PRIMARY_METRICS constant, replace 5 inline references
- M2: _compute_multiturn_stats returns stable shape on empty input
- Update tests: add parse_error attr to _FakeMetric, test TOOL_ERROR
  counting, test parse error/unknown category skipping, fix empty map
  assertion

Author: evekhm

scripts/quality_report.py

@@ -552,7 +552,7 @@ def _count_trace_metrics(trace):
   for span in trace.spans:
     if span.event_type == "USER_MESSAGE_RECEIVED":
       user_turns += 1
-    elif span.event_type == "TOOL_COMPLETED":
+    elif span.event_type in ("TOOL_COMPLETED", "TOOL_ERROR"):
       tool_calls += 1
   return user_turns, tool_calls
 
@@ -1767,7 +1767,26 @@ def _build_agent_stats(report, resolved_map):
     },
 }
 
-_DIMENSION_NAMES = list(_DIMENSION_SCORES.keys())
+_DIMENSION_NAMES = list(_DIMENSION_SCORES.keys())  # order matters for rendering
+
+_PRIMARY_METRICS = {"response_usefulness", "task_grounding"}
+
+_SCORECARD_ICONS = {
+    "correct": "✅",
+    "mostly_correct": "⚠️",
+    "incorrect": "❌",
+    "proper": "✅",
+    "partial": "⚠️",
+    "none": "❌",
+    "specific": "✅",
+    "somewhat_specific": "⚠️",
+    "vague": "❌",
+    "compliant": "✅",
+    "partially_compliant": "⚠️",
+    "non_compliant": "❌",
+    "clarification_needed": "⚠️",
+    "correction_needed": "❌",
+}
 
 # Maps dimension → (lowest category, section title) for "Low X" report sections.
 _DIMENSION_LOW_CATEGORIES = {
@@ -1792,8 +1811,9 @@ def _compute_dimension_averages(report):
     for mr in sr.metrics:
       if mr.metric_name in _DIMENSION_SCORES:
         score_map = _DIMENSION_SCORES[mr.metric_name]
-        score = score_map.get(mr.category, 0)
-        dim_totals[mr.metric_name].append(score)
+        if mr.parse_error or mr.category not in score_map:
+          continue
+        dim_totals[mr.metric_name].append(score_map[mr.category])
   return {
       d: round(sum(scores) / len(scores), 2) if scores else 0
       for d, scores in dim_totals.items()
@@ -1808,7 +1828,11 @@ def _compute_multiturn_stats(resolved_map):
   verifications = [r.get("verifications", 0) for r in resolved_map.values()]
   total = len(user_turns)
   if not total:
-    return {}
+    return {
+        "avg_user_turns": 0,
+        "avg_tool_calls": 0,
+        "multi_turn_sessions": 0,
+    }
   mt_count = sum(1 for t in user_turns if t > 1)
   stats = {
       "avg_user_turns": round(sum(user_turns) / total, 1),
@@ -1876,7 +1900,7 @@ def _print_eval_results(
 
       # Primary metrics with justifications
       for mr in sr.metrics:
-        if mr.metric_name not in ("response_usefulness", "task_grounding"):
+        if mr.metric_name not in _PRIMARY_METRICS:
           continue
         mr_label = _category_label(mr.category)
         if mr.parse_error:
@@ -1892,7 +1916,7 @@ def _print_eval_results(
       # Compact scorecard for quality dimensions
       dim_parts = []
       for mr in sr.metrics:
-        if mr.metric_name in ("response_usefulness", "task_grounding"):
+        if mr.metric_name in _PRIMARY_METRICS:
           continue
         display_name = _METRIC_LABELS.get(mr.metric_name, mr.metric_name)
         mr_label = _category_label(mr.category)
@@ -2055,7 +2079,7 @@ def _print_eval_results(
 
   print("\n  Category Distributions:")
   for metric_name, dist in report.category_distributions.items():
-    if metric_name not in ("response_usefulness", "task_grounding"):
+    if metric_name not in _PRIMARY_METRICS:
       continue
     print(f"\n  [{metric_name}]")
     dist_total = sum(dist.values())
@@ -2378,28 +2402,12 @@ def _md_write_trajectory_section(w, trajectories, resolved_map):
 
 def _md_dimension_scorecard(sr):
   """Build a compact one-line scorecard for the 5 quality dimensions."""
-  _SCORECARD_ICONS = {
-      "correct": "\u2705",
-      "mostly_correct": "\u26a0\ufe0f",
-      "incorrect": "\u274c",
-      "proper": "\u2705",
-      "partial": "\u26a0\ufe0f",
-      "none": "\u274c",
-      "specific": "\u2705",
-      "somewhat_specific": "\u26a0\ufe0f",
-      "vague": "\u274c",
-      "compliant": "\u2705",
-      "partially_compliant": "\u26a0\ufe0f",
-      "non_compliant": "\u274c",
-      "clarification_needed": "\u26a0\ufe0f",
-      "correction_needed": "\u274c",
-  }
   parts = []
   for mr in sr.metrics:
-    if mr.metric_name in ("response_usefulness", "task_grounding"):
+    if mr.metric_name in _PRIMARY_METRICS:
       continue
     label = _METRIC_LABELS.get(mr.metric_name, mr.metric_name)
-    icon = _SCORECARD_ICONS.get(mr.category, "\u2705")
+    icon = _SCORECARD_ICONS.get(mr.category, "\u2753")
     parts.append(f"{label} {icon}")
   return " | ".join(parts)
 
@@ -2458,7 +2466,7 @@ def _md_write_session_section(
     w(f"- **Response:** {r_display}")
 
     for mr in sr.metrics:
-      if mr.metric_name not in ("response_usefulness", "task_grounding"):
+      if mr.metric_name not in _PRIMARY_METRICS:
         continue
       label = _category_label(mr.category)
       display = _METRIC_LABELS.get(mr.metric_name, mr.metric_name)
@@ -2868,7 +2876,6 @@ def _write_md_report(
     sessions = _md_find_low_dimension_sessions(report, dim, low_cat)
     if sessions:
       low_dims[dim] = sessions
-  _PRIMARY_METRICS = {"response_usefulness", "task_grounding"}
 
   # --- TOC ---
   w("# Quality Evaluation Report")

tests/test_quality_report_helpers.py

@@ -62,9 +62,10 @@ def __init__(self, spans):
 
 class _FakeMetric:
 
-  def __init__(self, metric_name, category):
+  def __init__(self, metric_name, category, parse_error=False):
     self.metric_name = metric_name
     self.category = category
+    self.parse_error = parse_error
 
 
 class _FakeSession:
@@ -655,6 +656,18 @@ def test_tool_starting_not_counted(self):
     _, tool_calls = _count_trace_metrics(trace)
     assert tool_calls == 1
 
+  def test_tool_error_counted(self):
+    trace = _FakeTrace(
+        [
+            _FakeSpan("TOOL_STARTING", {"tool": "search"}),
+            _FakeSpan("TOOL_ERROR", {"error": "timeout"}),
+            _FakeSpan("TOOL_STARTING", {"tool": "lookup"}),
+            _FakeSpan("TOOL_COMPLETED", {"tool": "lookup"}),
+        ]
+    )
+    _, tool_calls = _count_trace_metrics(trace)
+    assert tool_calls == 2
+
 
 # ================================================================== #
 # _compute_dimension_averages                                         #
@@ -726,6 +739,34 @@ def test_missing_dimensions(self):
     # Non-dimension metrics should not contribute
     assert avgs["correctness"] == 0
 
+  def test_parse_error_skipped(self):
+    sessions = [
+        _FakeSession(
+            "s1",
+            [
+                _FakeMetric("correctness", "correct"),
+                _FakeMetric("correctness", "incorrect", parse_error=True),
+            ],
+        ),
+    ]
+    report = _FakeReport(sessions)
+    avgs = _compute_dimension_averages(report)
+    assert avgs["correctness"] == 2.0
+
+  def test_unknown_category_skipped(self):
+    sessions = [
+        _FakeSession(
+            "s1",
+            [
+                _FakeMetric("correctness", "correct"),
+                _FakeMetric("correctness", "bogus_value"),
+            ],
+        ),
+    ]
+    report = _FakeReport(sessions)
+    avgs = _compute_dimension_averages(report)
+    assert avgs["correctness"] == 2.0
+
 
 # ================================================================== #
 # _compute_multiturn_stats                                            #
@@ -745,7 +786,12 @@ def test_basic_stats(self):
     assert stats["multi_turn_sessions"] == 1
 
   def test_empty_map(self):
-    assert _compute_multiturn_stats({}) == {}
+    result = _compute_multiturn_stats({})
+    assert result == {
+        "avg_user_turns": 0,
+        "avg_tool_calls": 0,
+        "multi_turn_sessions": 0,
+    }
 
   def test_all_single_turn(self):
     resolved = {