Report incomplete coverage on the PR comment, not just the commit status

The previous commit taught _verdict_outputs about missing buckets but not
_build_summary_markdown, which is what produces verdict_summary.md -- and that
one file is both the job summary and the sticky PR comment. The two surfaces
are computed by different code paths, so a run where 8 of 9 buckets reported
put a red status reading "only 8 of 9 buckets reported a result" directly above
a comment headlined "No meaningful performance regressions detected ... 0
benchmark failures". A reviewer reading the comment would conclude the red
check was gate noise and merge a change whose one regressing task was never
measured. Reproduced end to end against the real nine-bucket matrix.

Coverage is now computed once, in _coverage(), and passed to both surfaces, so
they cannot disagree by construction. The comment headline ranks a shortfall
above skew, BLOCK and WARN -- the rows that did arrive may all be clean, but the
change is not covered, so no all-clear may be printed -- and the count line
names the buckets that never reported rather than only counting them. Counts
are over distinct buckets, so a duplicated artifact cannot inflate the total,
and an unreadable tasks.json yields an empty result that can never invent a
failure.

The advisory banner claimed a red status always shows up "in this table", which
was untrue for exactly this case; it now points at the overall result instead.

The new tests drive the real aggregate.main() over the real matrix and compare
both surfaces, which is the check that was missing: the previous tests
exercised _verdict_outputs in isolation and could not have caught a divergence.

Author: fatimaanes

tools/perf_smoke_test/aggregate.py

@@ -329,8 +329,40 @@ def _build_stale_image_section(skewed: list[tuple[str, str, DependencySkew]]) ->
     )
 
 
-def _build_summary_markdown(rows: list[tuple], *, blocking: bool) -> str:
-    """Build reviewer-first Markdown for the sticky PR comment."""
+def _coverage(rows: list[tuple]) -> tuple[list[str], int]:
+    """Return ``(missing_labels, expected_total)`` for the configured matrix.
+
+    A bucket whose job died before ``build_bench_result`` uploads nothing, so it
+    contributes no row. Grading only the rows that arrived would report an
+    all-clear over a bucket that was never measured, which is exactly the class
+    of silent green this gate exists to catch. Returns ``([], 0)`` when the
+    matrix cannot be read, so this can never invent a failure.
+
+    Counts are over distinct buckets, not rows, so a duplicated artifact cannot
+    inflate the total.
+    """
+    try:
+        expected = {(task.task_id, task.backend_key) for task in load_tasks()}
+    except Exception:
+        return [], 0
+    reported = {(result.task_id, result.backend) for result, _ in rows}
+    missing = sorted(f"{task_id}/{backend}" for task_id, backend in expected - reported)
+    return missing, len(expected)
+
+
+def _build_summary_markdown(
+    rows: list[tuple], *, blocking: bool, missing: list[str] | None = None, expected_total: int = 0
+) -> str:
+    """Build reviewer-first Markdown for the sticky PR comment.
+
+    Args:
+        rows: ``(OracleResult, BenchResult)`` pairs for every scored bucket.
+        blocking: Whether the gate is in blocking mode, for the mode banner.
+        missing: ``task/backend`` labels that produced no result at all. Kept in
+            the headline so the comment cannot disagree with the commit status.
+        expected_total: Size of the configured matrix, for the "N of M" phrasing.
+    """
+    missing = missing or []
     counts = {verdict: 0 for verdict in OracleVerdict}
     for result, _ in rows:
         counts[result.verdict] += 1
@@ -347,6 +379,11 @@ def _build_summary_markdown(rows: list[tuple], *, blocking: bool) -> str:
         overall = "❌ No benchmark results were produced"
     elif unexplained_failures:
         overall = "❌ One or more benchmarks failed before producing usable performance data"
+    elif missing:
+        # Ranked above skew/BLOCK/WARN: the rows that did arrive may all be clean,
+        # but the change is not covered, so no all-clear may be printed.
+        reported_n = expected_total - len(missing)
+        overall = f"❌ Only {reported_n} of {expected_total} benchmark buckets reported — coverage is incomplete"
     elif skewed:
         overall = "⚠️ The CI image is stale for this PR, so some tasks could not be measured"
     elif counts[OracleVerdict.BLOCK]:
@@ -359,20 +396,25 @@ def _build_summary_markdown(rows: list[tuple], *, blocking: bool) -> str:
     warnings_label = "warning" if counts[OracleVerdict.WARN] == 1 else "warnings"
     blocks_label = "blocking signal" if counts[OracleVerdict.BLOCK] == 1 else "blocking signals"
     failures_label = "benchmark failure" if counts[OracleVerdict.HARD_FAILURE] == 1 else "benchmark failures"
-    count_summary = " · ".join(
-        (
-            f"✅ {counts[OracleVerdict.PASS]} passed",
-            f"⚠️ {counts[OracleVerdict.WARN]} {warnings_label}",
-            f"🚫 {counts[OracleVerdict.BLOCK]} {blocks_label}",
-            f"❌ {counts[OracleVerdict.HARD_FAILURE]} {failures_label}",
-        )
-    )
+    count_parts = [
+        f"✅ {counts[OracleVerdict.PASS]} passed",
+        f"⚠️ {counts[OracleVerdict.WARN]} {warnings_label}",
+        f"🚫 {counts[OracleVerdict.BLOCK]} {blocks_label}",
+        f"❌ {counts[OracleVerdict.HARD_FAILURE]} {failures_label}",
+    ]
+    if missing:
+        # Name them: "8 of 9" alone leaves the reviewer guessing which task is
+        # unmeasured, and a missing bucket is the one a regression can hide in.
+        count_parts.append(f"🚫 {len(missing)} did not report ({', '.join(missing)})")
+    count_summary = " · ".join(count_parts)
     mode = (
         "**Blocking:** BLOCK and HARD FAILURE results fail the check."
         if blocking
         else (
-            "**Advisory:** this job stays green whatever the verdict. A BLOCK or HARD FAILURE still shows up"
-            " as a red `perf-smoke-test` commit status and in this table -- it just does not fail the PR."
+            "**Advisory:** this job stays green whatever the verdict. Anything that is not a clean pass --"
+            " a BLOCK, a HARD FAILURE, or a bucket that never reported -- still shows up as a red"
+            " `perf-smoke-test` commit status, and the reason is in the overall result above."
+            " It just does not fail the PR."
         )
     )
 
@@ -425,7 +467,13 @@ def _write_github_output(**values) -> None:
 
 
 def _verdict_outputs(
-    rows: list[tuple], *, has_block: bool, has_hard_failure: bool, blocking: bool, expected_buckets: int | None = None
+    rows: list[tuple],
+    *,
+    has_block: bool,
+    has_hard_failure: bool,
+    blocking: bool,
+    missing: list[str] | None = None,
+    expected_total: int = 0,
 ) -> dict[str, str]:
     """Derive the reported verdict and the `perf-smoke-test` commit status from the rows.
 
@@ -440,8 +488,9 @@ def _verdict_outputs(
         has_hard_failure: Whether any bucket failed to produce a usable measurement,
             excluding failures already excused as a stale CI image.
         blocking: The gate's ``blocking`` setting, used only to label the status.
-        expected_buckets: How many buckets the matrix should have produced. When
-            fewer reported, the gate says so instead of grading the survivors.
+        missing: ``task/backend`` labels that produced no result. Shared with
+            :func:`_build_summary_markdown` so the commit status and the PR
+            comment cannot disagree about coverage.
 
     Returns:
         The ``overall_verdict`` / ``status_state`` / ``status_description`` /
@@ -453,7 +502,7 @@ def _verdict_outputs(
     # most bench jobs never uploaded, would otherwise fall through to an
     # affirmative "no regression detected" over measurements that never happened.
     unmeasured = [result for result, _ in rows if result.verdict == OracleVerdict.HARD_FAILURE]
-    missing = max(0, (expected_buckets or 0) - len(rows))
+    missing = missing or []
 
     if has_hard_failure:
         verdict = OracleVerdict.HARD_FAILURE
@@ -465,7 +514,7 @@ def _verdict_outputs(
         description = f"perf-smoke: no usable measurement for {len(unmeasured)} bucket(s); CI image looks stale"
     elif missing or not rows:
         verdict = OracleVerdict.HARD_FAILURE
-        description = f"perf-smoke: only {len(rows)} of {expected_buckets or '?'} buckets reported a result"
+        description = f"perf-smoke: only {expected_total - len(missing)} of {expected_total} buckets reported a result"
     elif has_block:
         verdict = OracleVerdict.BLOCK
         description = "perf-smoke: blocking-level performance regression detected"
@@ -703,7 +752,10 @@ def main() -> int:
             baseline_update_failed = True
             print(f"::error::Baseline push failed: {exc}")
 
-    summary = _build_summary_markdown(rows, blocking=blocking)
+    # One source of truth for coverage, shared by the summary/comment and the
+    # commit status so the two surfaces cannot contradict each other.
+    missing_buckets, expected_total = _coverage(rows)
+    summary = _build_summary_markdown(rows, blocking=blocking, missing=missing_buckets, expected_total=expected_total)
     print("\n## Performance Smoke Results\n")
     print(summary)
     print()
@@ -729,25 +781,15 @@ def main() -> int:
             app_config=args.omni_app_config,
         )
 
-    # How many buckets the matrix should have produced. The gate always runs the
-    # full matrix, so a shortfall means bench jobs died without uploading -- which
-    # must not be graded as a pass over the survivors. Left as None (check
-    # disabled) if tasks.json cannot be read, so this can never fail a run by
-    # itself.
-    try:
-        expected_buckets = len(load_tasks())
-    except Exception as exc:  # pragma: no cover - defensive
-        print(f"[aggregate] Warning: could not determine expected bucket count: {exc}")
-        expected_buckets = None
-
     output_values = {
         "baseline_read_sha": baseline_read_sha,
         **_verdict_outputs(
             rows,
             has_block=has_block,
             has_hard_failure=has_hard_failure,
             blocking=blocking,
-            expected_buckets=expected_buckets,
+            missing=missing_buckets,
+            expected_total=expected_total,
         ),
     }
     if baseline_push_result:

tools/perf_smoke_test/test/test_advisory_exit.py

@@ -237,7 +237,8 @@ def test_skew_excused_crashes_do_not_report_pass() -> None:
         has_block=False,
         has_hard_failure=False,  # cleared by detect_dependency_skew
         blocking=False,
-        expected_buckets=9,
+        missing=[],
+        expected_total=9,
     )
 
     assert out["overall_verdict"] == "HARD_FAILURE"
@@ -249,7 +250,12 @@ def test_skew_excused_crashes_do_not_report_pass() -> None:
 def test_missing_buckets_are_not_graded_on_the_survivors() -> None:
     """One passing bucket out of nine is not a pass."""
     out = aggregate._verdict_outputs(
-        _rows(OracleVerdict.PASS), has_block=False, has_hard_failure=False, blocking=False, expected_buckets=9
+        _rows(OracleVerdict.PASS),
+        has_block=False,
+        has_hard_failure=False,
+        blocking=False,
+        missing=[f"Task-{i}/physx" for i in range(8)],
+        expected_total=9,
     )
 
     assert out["overall_verdict"] == "HARD_FAILURE"
@@ -264,22 +270,100 @@ def test_a_complete_clean_run_still_passes() -> None:
         has_block=False,
         has_hard_failure=False,
         blocking=False,
-        expected_buckets=9,
+        missing=[],
+        expected_total=9,
     )
 
     assert out["overall_verdict"] == "PASS"
     assert out["status_state"] == "success"
     assert "9 buckets" in out["status_description"]
 
 
-def test_expected_bucket_count_is_optional() -> None:
-    """An unreadable tasks.json disables the completeness check, never fails a run."""
+def test_unreadable_matrix_disables_the_completeness_check() -> None:
+    """An unreadable tasks.json must never invent a failure."""
     out = aggregate._verdict_outputs(
         _rows(*([OracleVerdict.PASS] * 3)),
         has_block=False,
         has_hard_failure=False,
         blocking=False,
-        expected_buckets=None,
+        missing=[],
+        expected_total=0,
     )
 
     assert out["overall_verdict"] == "PASS"
+
+
+# --- the comment and the commit status must never disagree ----------------
+#
+# They are produced by different code paths: the sticky comment and job summary
+# come from _build_summary_markdown, the status from _verdict_outputs. An
+# earlier fix taught only the latter about missing buckets, so a run with 8 of 9
+# buckets reported showed a red status reading "only 8 of 9 buckets reported"
+# directly above a comment headlined "No meaningful performance regressions
+# detected -- 0 benchmark failures". These drive the real aggregate.main() over
+# the real tasks.json and compare both surfaces.
+
+import dataclasses  # noqa: E402
+
+from task_config import load_tasks  # noqa: E402
+
+
+def _run_real_matrix(tmp_path: Path, monkeypatch, *, reported: int):
+    """Run aggregate over `reported` of the 9 real matrix buckets."""
+    buckets = [(t.task_id, t.backend_key) for t in load_tasks()]
+    artifacts_dir = tmp_path / "artifacts"
+    artifacts_dir.mkdir(parents=True, exist_ok=True)
+    (tmp_path / "baselines").mkdir(parents=True, exist_ok=True)
+    summary_file = tmp_path / "verdict_summary.md"
+    output_file = tmp_path / "gh_output.txt"
+    gate_config = tmp_path / "gate_config.json"
+    gate_config.write_text(json.dumps({"blocking": False}), encoding="utf-8")
+
+    for i, (task_id, backend_key) in enumerate(buckets[:reported]):
+        bench = dataclasses.replace(
+            _bench_result(fps=100.0, info_present=True), task_id=task_id, backend=backend_key, backend_key=backend_key
+        )
+        task_dir = artifacts_dir / f"bench-{i}"
+        task_dir.mkdir(parents=True, exist_ok=True)
+        (task_dir / "perf_smoke_test_result.json").write_text(json.dumps(bench.to_dict()))
+
+    monkeypatch.setenv("GITHUB_OUTPUT", str(output_file))
+    monkeypatch.setattr(
+        sys,
+        "argv",
+        [
+            "aggregate.py",
+            "--artifacts_dir", str(artifacts_dir),
+            "--gpu_model", "L40S",
+            "--baselines_dir", str(tmp_path / "baselines"),
+            "--allow_baseline_update", "false",
+            "--gate_config", str(gate_config),
+            "--summary_file", str(summary_file),
+        ],
+    )  # fmt: skip
+    aggregate.main()
+    outputs = dict(line.split("=", 1) for line in output_file.read_text().splitlines() if "=" in line)
+    return outputs, summary_file.read_text(encoding="utf-8"), len(buckets)
+
+
+def test_partial_matrix_is_flagged_on_both_surfaces(tmp_path, monkeypatch) -> None:
+    """A red status must never sit above an all-clear comment."""
+    total_reported = 8
+    outputs, summary, total = _run_real_matrix(tmp_path, monkeypatch, reported=total_reported)
+
+    assert outputs["status_state"] == "failure"
+    assert f"only {total_reported} of {total}" in outputs["status_description"]
+    # The comment is what a reviewer reads, so it must carry the same message.
+    assert f"Only {total_reported} of {total}" in summary
+    assert "No meaningful performance regressions detected" not in summary
+    # And it must name the bucket that vanished, not just count it.
+    assert "did not report" in summary
+
+
+def test_complete_matrix_is_clean_on_both_surfaces(tmp_path, monkeypatch) -> None:
+    """The coverage guard must not fire when every bucket reported."""
+    outputs, summary, total = _run_real_matrix(tmp_path, monkeypatch, reported=9)
+
+    assert outputs["status_state"] == "success"
+    assert "did not report" not in summary
+    assert "coverage is incomplete" not in summary