Merge 17b6d59338 into f6656c1256

skills/skill-creator/scripts/aggregate_benchmark.py

@@ -177,7 +177,8 @@ def aggregate_results(results: dict) -> dict:
     """
     Aggregate run results into summary statistics.
 
-    Returns run_summary with stats for each configuration and delta.
+    Returns run_summary with stats for each configuration and, when two or
+    more configurations exist, a delta between the first two.
     """
     run_summary = {}
     configs = list(results.keys())
@@ -203,23 +204,22 @@ def aggregate_results(results: dict) -> dict:
             "tokens": calculate_stats(tokens)
         }
 
-    # Calculate delta between the first two configs (if two exist)
+    # A delta only exists between two configurations; with a single config
+    # there is no baseline to compare against, and emitting one against an
+    # invented zero baseline would fabricate a "+X" improvement.
     if len(configs) >= 2:
         primary = run_summary.get(configs[0], {})
         baseline = run_summary.get(configs[1], {})
-    else:
-        primary = run_summary.get(configs[0], {}) if configs else {}
-        baseline = {}
 
-    delta_pass_rate = primary.get("pass_rate", {}).get("mean", 0) - baseline.get("pass_rate", {}).get("mean", 0)
-    delta_time = primary.get("time_seconds", {}).get("mean", 0) - baseline.get("time_seconds", {}).get("mean", 0)
-    delta_tokens = primary.get("tokens", {}).get("mean", 0) - baseline.get("tokens", {}).get("mean", 0)
+        delta_pass_rate = primary.get("pass_rate", {}).get("mean", 0) - baseline.get("pass_rate", {}).get("mean", 0)
+        delta_time = primary.get("time_seconds", {}).get("mean", 0) - baseline.get("time_seconds", {}).get("mean", 0)
+        delta_tokens = primary.get("tokens", {}).get("mean", 0) - baseline.get("tokens", {}).get("mean", 0)
 
-    run_summary["delta"] = {
-        "pass_rate": f"{delta_pass_rate:+.2f}",
-        "time_seconds": f"{delta_time:+.1f}",
-        "tokens": f"{delta_tokens:+.0f}"
-    }
+        run_summary["delta"] = {
+            "pass_rate": f"{delta_pass_rate:+.2f}",
+            "time_seconds": f"{delta_time:+.1f}",
+            "tokens": f"{delta_tokens:+.0f}"
+        }
 
     return run_summary
 
@@ -285,10 +285,7 @@ def generate_markdown(benchmark: dict) -> str:
 
     # Determine config names (excluding "delta")
     configs = [k for k in run_summary if k != "delta"]
-    config_a = configs[0] if len(configs) >= 1 else "config_a"
-    config_b = configs[1] if len(configs) >= 2 else "config_b"
-    label_a = config_a.replace("_", " ").title()
-    label_b = config_b.replace("_", " ").title()
+    label_a = configs[0].replace("_", " ").title() if configs else "config_a"
 
     lines = [
         f"# Skill Benchmark: {metadata['skill_name']}",
@@ -299,28 +296,42 @@ def generate_markdown(benchmark: dict) -> str:
         "",
         "## Summary",
         "",
-        f"| Metric | {label_a} | {label_b} | Delta |",
-        "|--------|------------|---------------|-------|",
     ]
 
-    a_summary = run_summary.get(config_a, {})
-    b_summary = run_summary.get(config_b, {})
-    delta = run_summary.get("delta", {})
+    a_summary = run_summary.get(configs[0], {}) if configs else {}
 
     # Format pass rate
     a_pr = a_summary.get("pass_rate", {})
-    b_pr = b_summary.get("pass_rate", {})
-    lines.append(f"| Pass Rate | {a_pr.get('mean', 0)*100:.0f}% ± {a_pr.get('stddev', 0)*100:.0f}% | {b_pr.get('mean', 0)*100:.0f}% ± {b_pr.get('stddev', 0)*100:.0f}% | {delta.get('pass_rate', '—')} |")
+    a_pass_rate = f"{a_pr.get('mean', 0)*100:.0f}% ± {a_pr.get('stddev', 0)*100:.0f}%"
 
     # Format time
     a_time = a_summary.get("time_seconds", {})
-    b_time = b_summary.get("time_seconds", {})
-    lines.append(f"| Time | {a_time.get('mean', 0):.1f}s ± {a_time.get('stddev', 0):.1f}s | {b_time.get('mean', 0):.1f}s ± {b_time.get('stddev', 0):.1f}s | {delta.get('time_seconds', '—')}s |")
+    a_time_s = f"{a_time.get('mean', 0):.1f}s ± {a_time.get('stddev', 0):.1f}s"
 
     # Format tokens
     a_tokens = a_summary.get("tokens", {})
-    b_tokens = b_summary.get("tokens", {})
-    lines.append(f"| Tokens | {a_tokens.get('mean', 0):.0f} ± {a_tokens.get('stddev', 0):.0f} | {b_tokens.get('mean', 0):.0f} ± {b_tokens.get('stddev', 0):.0f} | {delta.get('tokens', '—')} |")
+    a_tokens_s = f"{a_tokens.get('mean', 0):.0f} ± {a_tokens.get('stddev', 0):.0f}"
+
+    if len(configs) >= 2:
+        label_b = configs[1].replace("_", " ").title()
+        b_summary = run_summary.get(configs[1], {})
+        delta = run_summary.get("delta", {})
+
+        b_pr = b_summary.get("pass_rate", {})
+        b_time = b_summary.get("time_seconds", {})
+        b_tokens = b_summary.get("tokens", {})
+
+        lines.append(f"| Metric | {label_a} | {label_b} | Delta |")
+        lines.append("|--------|------------|---------------|-------|")
+        lines.append(f"| Pass Rate | {a_pass_rate} | {b_pr.get('mean', 0)*100:.0f}% ± {b_pr.get('stddev', 0)*100:.0f}% | {delta.get('pass_rate', '—')} |")
+        lines.append(f"| Time | {a_time_s} | {b_time.get('mean', 0):.1f}s ± {b_time.get('stddev', 0):.1f}s | {delta.get('time_seconds', '—')}s |")
+        lines.append(f"| Tokens | {a_tokens_s} | {b_tokens.get('mean', 0):.0f} ± {b_tokens.get('stddev', 0):.0f} | {delta.get('tokens', '—')} |")
+    else:
+        lines.append(f"| Metric | {label_a} |")
+        lines.append("|--------|------------|")
+        lines.append(f"| Pass Rate | {a_pass_rate} |")
+        lines.append(f"| Time | {a_time_s} |")
+        lines.append(f"| Tokens | {a_tokens_s} |")
 
     # Notes section
     if benchmark.get("notes"):

skills/skill-creator/tests/test_aggregate_benchmark.py

@@ -0,0 +1,81 @@
+"""Regression tests for aggregate_benchmark.py.
+
+Covers the fabricated-delta bug: with only a single configuration of runs
+(e.g. only with_skill, which the loader supports), aggregate_results()
+previously emitted a "delta" against an invented zero baseline, so
+benchmark reports claimed a pass-rate/time/token improvement that no
+comparison supported.
+"""
+
+import sys
+from pathlib import Path
+
+import pytest
+
+SKILL_CREATOR = Path(__file__).resolve().parents[1]
+sys.path.insert(0, str(SKILL_CREATOR))
+
+from scripts.aggregate_benchmark import aggregate_results, generate_markdown  # noqa: E402
+
+
+def _run(pass_rate: float, time_seconds: float = 45.0, tokens: int = 3800) -> dict:
+    return {
+        "eval_id": 1,
+        "run_number": 1,
+        "pass_rate": pass_rate,
+        "passed": 8,
+        "failed": 2,
+        "total": 10,
+        "time_seconds": time_seconds,
+        "tokens": tokens,
+        "tool_calls": 18,
+        "errors": 0,
+        "expectations": [],
+        "notes": [],
+    }
+
+
+def _benchmark(run_summary: dict) -> dict:
+    return {
+        "metadata": {
+            "skill_name": "docx",
+            "executor_model": "m",
+            "timestamp": "t",
+            "evals_run": [1],
+            "runs_per_configuration": 1,
+        },
+        "run_summary": run_summary,
+        "notes": [],
+    }
+
+
+def test_single_config_has_no_delta() -> None:
+    summary = aggregate_results({"with_skill": [_run(0.80)]})
+    assert "delta" not in summary
+    assert summary["with_skill"]["pass_rate"]["mean"] == pytest.approx(0.80)
+
+
+def test_single_config_markdown_omits_delta_column() -> None:
+    summary = aggregate_results({"with_skill": [_run(0.80)]})
+    markdown = generate_markdown(_benchmark(summary))
+    assert "Delta" not in markdown
+    assert "Config B" not in markdown
+    assert "+0.80" not in markdown
+    assert "80% ± 0%" in markdown
+
+
+def test_two_configs_still_report_delta() -> None:
+    summary = aggregate_results(
+        {
+            "with_skill": [_run(0.85), _run(0.71)],
+            "without_skill": [_run(0.35, time_seconds=32.0, tokens=2100)],
+        }
+    )
+    assert summary["delta"] == {
+        "pass_rate": "+0.43",
+        "time_seconds": "+13.0",
+        "tokens": "+1700",
+    }
+    markdown = generate_markdown(_benchmark(summary))
+    assert "| Metric | With Skill | Without Skill | Delta |" in markdown
+    assert "+0.43" in markdown