Updated Benchmark validation comment and error strategy

Signed-off-by: Effi-S <effi.szt@gmail.com>

Author: Effi-S

.github/workflows/token-validation-benchmark.yml

@@ -28,19 +28,24 @@ name: Token Validation Benchmark
 #   If a run is interrupted, clear leftover containers before retrying:
 #        docker rm -f $(docker ps -aq --filter ancestor=catthehacker/ubuntu:act-latest)
 on:
-  # pull_request_target runs in the BASE repo context, so GITHUB_TOKEN keeps
-  # write access even for fork PRs — which is what lets the compare job post its
-  # comment. The tradeoff is that this event is inherently more dangerous: the
-  # benchmark job below checks out and runs the PR HEAD (untrusted) code. To
-  # keep that safe, the default token here is read-only (see permissions), and
-  # pull-requests: write is granted ONLY to the trusted compare job that never
-  # runs PR code. Do not add secrets or write scopes to the benchmark job.
-  pull_request_target:
+  # The benchmark job below checks out and runs the PR HEAD (untrusted fork)
+  # code, so this workflow runs on `pull_request`, NOT `pull_request_target`:
+  # fork PRs run in the fork's untrusted context with a read-only GITHUB_TOKEN
+  # and no access to secrets, and `actions/checkout` pulls the fork's PR code
+  # directly (no `allow-unsafe-pr-checkout` needed). This workflow only produces
+  # artifacts; posting the PR comment happens in the companion
+  # `token-validation-benchmark-comment.yml`, which runs on `workflow_run` in the
+  # trusted base-repo context and therefore has `pull-requests: write` even for
+  # fork PRs.
+  pull_request:
   workflow_dispatch:
 
-# Read-only by default. The benchmark job runs untrusted PR head code, so it
-# must not have a write-capable token. Write access is scoped per-job on the
-# compare job (below), which only runs trusted base-repo code.
+# Read-only by default: the benchmark job runs untrusted fork code and must not
+# have a write-capable token. pull-requests: write is scoped per-job on the
+# compare job (below). Under `pull_request` that write scope is effective only
+# for same-repo PRs; for fork PRs the token stays read-only, so the compare
+# job's comment step fails harmlessly (it is continue-on-error) and the
+# companion workflow_run workflow posts the comment from the trusted context.
 permissions:
   contents: read
 
@@ -191,17 +196,18 @@ jobs:
     needs: benchmark
     runs-on: ubuntu-latest
     # Write scope lives HERE, not at the top level, because this job runs only
-    # trusted base-repo code (the checkout below takes the default base ref, and
-    # it never checks out or executes PR head code). The benchmark job keeps the
-    # read-only default token.
+    # trusted base-repo code (it never checks out or executes PR head code). The
+    # benchmark job keeps the read-only default token. On fork PRs the token is
+    # read-only regardless, so the comment step below cannot post — that is why
+    # the comment step itself is continue-on-error and the companion workflow_run
+    # workflow handles commenting for forks. The job as a whole is NOT
+    # continue-on-error: a detected regression fails it (see the final step).
     permissions:
       contents: read
       pull-requests: write
-    continue-on-error: true
     steps:
-      # No ref specified: under pull_request_target this checks out the BASE
-      # repo/branch (trusted), which is exactly what we want for the code that
-      # holds the write token.
+      # No ref specified: checks out this workflow's ref (the base repo), NOT the
+      # PR head, so the compare script we run is trusted.
       - name: Checkout code
         uses: actions/checkout@v4
 
@@ -221,15 +227,44 @@ jobs:
         run: pip install pandas
 
       - name: Build comparison report
+        id: report
         run: |
+          # Always build and print the report so it is visible in the job log,
+          # regardless of whether a regression is detected. The script exits 3
+          # when it detects a regression, 0 otherwise; any other non-zero code is
+          # a real failure and must propagate. Capture the code without letting
+          # `set -e` abort the step on the expected exit 3.
+          set +e
           python cmd/benchmarking/compare_benchmarks.py \
             --input-dir "$OUTPUT_DIR" \
             --base-tag '_base_' \
             --pr-tag '_pr_' \
             --delta "$DELTA" \
             --output comment.md
+          rc=$?
+          set -e
 
+          case "$rc" in
+            0) echo "degraded=false" >> "$GITHUB_OUTPUT" ;;
+            3) echo "degraded=true"  >> "$GITHUB_OUTPUT" ;;
+            *) echo "compare_benchmarks.py failed with exit code $rc" >&2
+               exit "$rc" ;;
+          esac
+
+      # Only comment when there is a regression to report. On fork PRs the token
+      # is read-only and this step cannot post, so it is continue-on-error; the
+      # companion workflow_run workflow handles commenting for forks.
       - name: Post comparison comment
+        if: steps.report.outputs.degraded == 'true'
+        continue-on-error: true
         env:
           GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
         run: gh pr comment "${{ github.event.pull_request.number }}" --body-file comment.md
+
+      # Fail the job on regression, after the comment has been posted, so the
+      # PR check turns red and the degradation is not silently accepted.
+      - name: Fail on regression
+        if: steps.report.outputs.degraded == 'true'
+        run: |
+          echo "::error::Token validation benchmark detected a performance regression (see report above)."
+          exit 1

cmd/benchmarking/compare_benchmarks.py

@@ -20,12 +20,18 @@
 from __future__ import annotations
 
 import argparse
+import sys
 from pathlib import Path
 
 import pandas as pd
 
 from bench_parse import simple_parser
 
+# Exit code used by ``main`` when the report contains at least one regression.
+# Distinct from 1 (used by argparse/uncaught errors) so callers can tell a
+# detected regression apart from the script failing to run at all.
+EXIT_REGRESSION = 3
+
 # Measurement/derived columns produced by the parser. These vary run-to-run
 # and must never be part of a row's identity key — only the benchmark's input
 # parameters (the ``key=value`` sub-tests) identify a row.
@@ -110,26 +116,38 @@ def _ranges_overlap(base_samples: list, pr_samples: list) -> bool:
     return min(base_samples) <= max(pr_samples) and min(pr_samples) <= max(base_samples)
 
 
-def _emoji(metric: str, pct: float, base_samples: list, pr_samples: list, delta: float = _DEFAULT_DELTA) -> str:
-    """Pick an indicator for a change, given whether lower is better.
+# Classification of a single metric change, independent of how it is rendered.
+_NEUTRAL, _IMPROVED, _REGRESSED = "neutral", "improved", "regressed"
+
+_STATUS_EMOJI = {_NEUTRAL: "➖", _IMPROVED: "🟢", _REGRESSED: "🔴"}
+
+
+def _classify(metric: str, pct: float, base_samples: list, pr_samples: list, delta: float = _DEFAULT_DELTA) -> str:
+    """Classify a metric change as neutral, improved, or regressed.
 
-    Returns the neutral marker when the change is within the ±1% noise band or
-    when the base/PR sample ranges overlap (i.e. the delta is not statistically
-    distinguishable from run-to-run jitter).
+    Returns ``_NEUTRAL`` when the change is within the ±delta% noise band or when
+    the base/PR sample ranges overlap (i.e. the delta is not statistically
+    distinguishable from run-to-run jitter). This is the single source of truth
+    for whether a row is a regression — the emoji is derived from it, never the
+    other way around.
     """
-    if abs(pct) < delta:  # treat sub-1% as noise
-        return "➖"
+    if abs(pct) < delta:  # treat sub-delta as noise
+        return _NEUTRAL
     if _ranges_overlap(base_samples, pr_samples):
-        return "➖"
+        return _NEUTRAL
     improved = (pct < 0) == _LOWER_IS_BETTER[metric]
-    return "🟢" if improved else "🔴"
+    return _IMPROVED if improved else _REGRESSED
 
 
-def build_report(base: pd.DataFrame, pr: pd.DataFrame, delta: float = _DEFAULT_DELTA) -> str:
-    """Render the Markdown comparison table for two parsed benchmark groups."""
+def build_report(base: pd.DataFrame, pr: pd.DataFrame, delta: float = _DEFAULT_DELTA) -> tuple[str, bool]:
+    """Render the Markdown comparison table for two parsed benchmark groups.
+
+    Returns ``(report, regressed)`` where ``regressed`` is ``True`` if any row was
+    classified as a regression. Missing results are not treated as a regression.
+    """
     if base.empty or pr.empty:
         missing = "base" if base.empty else "PR"
-        return f"⚠️ No benchmark results found for the **{missing}** branch."
+        return f"⚠️ No benchmark results found for the **{missing}** branch.", False
 
     # Parameter columns identify a row: everything except variant/bench/workers,
     # the measurements, and any derived latency column (``... (ms)``).
@@ -154,7 +172,7 @@ def by_key(df: pd.DataFrame) -> dict:
                         for r in rows if not pd.isna(r.get(metric))]
                 agg[metric] = sum(vals) / len(vals) if vals else float("nan")
                 # Retain the raw per-count samples so the significance guard in
-                # ``_emoji`` can compare the base/PR ranges, not just the means.
+                # ``_classify`` can compare the base/PR ranges, not just the means.
                 # A plain string key avoids pandas treating a tuple as a
                 # multi-index label on the Series.
                 agg[f"_samples_{metric}"] = vals
@@ -164,6 +182,8 @@ def by_key(df: pd.DataFrame) -> dict:
     base_by_key = by_key(base)
     pr_by_key = by_key(pr)
 
+    regressed = False
+
     lines = [
         "## 📊 Token Validation Benchmark",
         "",
@@ -192,9 +212,11 @@ def by_key(df: pd.DataFrame) -> dict:
             pct = _pct(bv, pv)
             base_samples = b.get(f"_samples_{metric}", [])
             pr_samples = p.get(f"_samples_{metric}", [])
+            status = _classify(metric, pct, base_samples, pr_samples, delta)
+            if status == _REGRESSED:
+                regressed = True
             cells.append(f"{bv:,.0f} → {pv:,.0f}")
-            cells.append(
-                f"{_emoji(metric, pct, base_samples, pr_samples)} {pct:+.1f}%")
+            cells.append(f"{_STATUS_EMOJI[status]} {pct:+.1f}%")
         lines.append("| " + " | ".join(cells) + " |")
 
     only_pr = pr_by_key.keys() - base_by_key.keys()
@@ -206,7 +228,7 @@ def by_key(df: pd.DataFrame) -> dict:
         lines += ["",
                   f"> ℹ️ {len(only_base)} benchmark(s) present only on the base branch (removed/renamed)."]
 
-    return "\n".join(lines) + "\n"
+    return "\n".join(lines) + "\n", regressed
 
 
 def main() -> None:
@@ -230,11 +252,17 @@ def main() -> None:
 
     base = _load_group(args.input_dir, args.base_tag)
     pr = _load_group(args.input_dir, args.pr_tag)
-    report = build_report(base, pr, delta=args.delta)
+    report, regressed = build_report(base, pr, delta=args.delta)
 
+    # Always write and print the report so it is visible regardless of outcome.
     args.output.write_text(report)
     print(report)
 
+    # Signal a detected regression via a distinct exit code so callers (e.g. CI)
+    # can gate on it without parsing the rendered Markdown/emoji.
+    if regressed:
+        sys.exit(EXIT_REGRESSION)
+
 
 if __name__ == "__main__":
     main()