fix: v1.2.0 pre-release review findings

README.md

@@ -49,7 +49,7 @@ composition.)
 | Build | `ptxas` | `ptxas-verbose` | kernel_codegen | `nvcc -Xptxas -v` (**no GPU needed**) | ✅ pure Python |
 | Build | `clang_time_trace` | `clang-time-trace`, `ftime-trace` | build_phase | `clang -ftime-trace` | ✅ pure Python |
 | Build | `cmake_profile` | `cmake-profile`, `cmake-trace` | build_phase | `cmake --profiling-format=google-trace` (≥3.18) | ✅ pure Python |
-| Build | `ninja_log` | `ninja-log` | build_step | `.ninja_log` (byproduct of any `ninja` build) | ✅ pure Python |
+| Build | `ninja_log` | `ninja-log`, `ninja_log` | build_step | `.ninja_log` (byproduct of any `ninja` build) | ✅ pure Python |
 | Build | `cargo_diag` | `cargo-diag`, `cargo-json` | build_diag | `cargo build --message-format=json` | ✅ pure Python |
 | Build | `criterion` | `criterion`, `criterion-json` | benchmark | `cargo bench` (`target/criterion` dir) | ✅ pure Python |
 | CI | `gha_log` | `gha-log`, `gh-run-log` | ci_step | saved `gh run view <id> --log` | ✅ pure Python |

src/perfdigest/adapters/chrome_trace/backend.py

@@ -17,7 +17,11 @@
 _PLATFORMS = frozenset({"linux", "darwin", "win32"})  # torch captures everywhere
 
 CHROME_TRACE_USAGE = (
-    "Chrome-trace digest (torch/Kineto, JAX, clang -ftime-trace...). Capture in "
+    "Chrome-trace digest (torch/Kineto, JAX, and other Chrome-trace emitters). "
+    "NOT for compiler/build traces that have their own backend: clang "
+    "-ftime-trace files belong to format clang-time-trace (its [total] tagging "
+    "prevents double-counting aggregate phases) and cmake profiling output to "
+    "format cmake-profile (B/E pair folding). Capture in "
     "code, exporting to a FILE (never print the profiler table):\n"
     "  with torch.profiler.profile(activities=[ProfilerActivity.CPU, "
     "ProfilerActivity.CUDA]) as prof:\n"

src/perfdigest/adapters/chrome_trace/trace_reader.py

@@ -1,15 +1,18 @@
 """Read Chrome-trace JSON (torch/Kineto & friends) -> NormalizedUnit.
 
 The FRAMEWORK layer. ``torch.profiler`` (Kineto) exports the Chrome Trace
-Format — and so do JAX, clang ``-ftime-trace``, Bazel and others — so this one
-pure-Python reader digests them all. We bind to the exported ARTIFACT, never to
-the framework's profiler API: the format has stayed stable across torch
-releases while the internals churned (fragility stays on the other side of the
-file boundary).
+Format — and so do JAX, Bazel and others — so this one pure-Python reader
+digests them all. We bind to the exported ARTIFACT, never to the framework's
+profiler API: the format has stayed stable across torch releases while the
+internals churned (fragility stays on the other side of the file boundary).
+Compiler/build traces that share the format have DEDICATED backends built on
+these same helpers — clang ``-ftime-trace`` -> ``clang_time_trace`` (adds
+``[total]`` aggregate tagging), cmake profiling -> ``cmake_profile`` (adds
+B/E pair folding) — route those formats there, not here.
 
 Two top-level shapes, both handled:
 
-  * dict with a ``traceEvents`` list (Kineto, ``-ftime-trace``)
+  * dict with a ``traceEvents`` list (Kineto and friends)
   * bare JSON array of events (legacy exporters)
 
 Only complete events (``"ph": "X"``) with a numeric ``dur`` are aggregated —
@@ -92,6 +95,11 @@ def _fold_begin_end_pairs(events: list) -> list:
                     dur = float(e["ts"]) - float(b["ts"])
                 except (KeyError, TypeError, ValueError):
                     continue  # unpaired timing info: skip, never fabricate
+                if dur < 0:
+                    # E before its own B on one thread is emitter/clock
+                    # corruption — a negative elapsed is physically impossible,
+                    # so drop the pair like an unparseable one.
+                    continue
                 out = dict(b)
                 out["ph"] = "X"
                 out["dur"] = dur
@@ -127,7 +135,7 @@ def _complete_events(report_path: str, *, fold_be_pairs: bool = False) -> list[d
         )
     if fold_be_pairs:
         events = _fold_begin_end_pairs(events)
-    return [
+    complete = [
         e
         for e in events
         if isinstance(e, dict)
@@ -137,6 +145,23 @@ def _complete_events(report_path: str, *, fold_be_pairs: bool = False) -> list[d
         and e.get("name") is not None
         and str(e["name"]).strip() != ""
     ]
+    if not complete and events:
+        # A valid trace with zero digestible events must not become a silent
+        # empty report ("the run did nothing" is a conclusion, not a default).
+        phases = {e.get("ph") for e in events if isinstance(e, dict)}
+        if not fold_be_pairs and ("B" in phases or "E" in phases):
+            raise ValueError(
+                f"{report_path} contains only begin/end (ph B/E) pair events and "
+                "no complete (ph 'X') events — this looks like a cmake "
+                "--profiling-format=google-trace capture; digest it with format "
+                "cmake-profile, which folds the pairs."
+            )
+        raise ValueError(
+            f"{report_path} is a valid trace container but has no complete "
+            "(ph=='X') events with a numeric dur — nothing to digest. Check the "
+            "emitter and the format argument."
+        )
+    return complete
 
 
 def _grouped(
@@ -199,7 +224,13 @@ def _grouped(
         forced = tag_override(rec) if tag_override is not None else None
         collide = len(cats_per_name[rec["name"]]) > 1 and rec["cat"]
         if forced:
-            rec["unit_name"] = f"{rec['name']} [{forced}]"
+            # A forced tag must not defeat the uniqueness invariant: if the
+            # name ALSO collides across categories, keep the cat tag too.
+            rec["unit_name"] = (
+                f"{rec['name']} [{forced}] [{rec['cat']}]"
+                if collide
+                else f"{rec['name']} [{forced}]"
+            )
         elif collide or rec["cat"] in _BOOKKEEPING_CATS:
             rec["unit_name"] = f"{rec['name']} [{rec['cat']}]"
         else:

src/perfdigest/adapters/criterion/criterion_reader.py

@@ -49,11 +49,11 @@ def _bench_dirs(root: Path) -> list[tuple[str, Path]]:
     found: list[tuple[str, Path]] = []
     for est in root.glob("**/new/estimates.json"):
         rel_parts = est.parent.parent.relative_to(root).parts
-        # criterion's own HTML lives under report/ dirs (the root's and each
-        # benchmark's); no estimates.json is written there, but guard anyway so
-        # a bookkeeping dir can never masquerade as a benchmark.
-        if "report" in rel_parts:
-            continue
+        # criterion's own HTML bookkeeping lives under report/ dirs, but those
+        # never contain a new/estimates.json, so the glob alone excludes them.
+        # No name-based guard on top: a user CAN name a bench "report" (it
+        # collides with criterion's HTML dir — criterion's wart, not ours), and
+        # suppressing its genuinely-written estimates would fabricate absence.
         found.append(("/".join(rel_parts), est))
     if not found:
         raise ValueError(

src/perfdigest/adapters/gha_log/backend.py

@@ -11,6 +11,7 @@
 from __future__ import annotations
 
 import subprocess
+import time
 
 from perfdigest.adapters import registry
 from perfdigest.adapters.gha_log import gha_log_reader, mapping
@@ -57,6 +58,49 @@
 )
 
 
+# `gh auth status` is the fleet's only probe doing I/O beyond shutil.which —
+# it can hit the network. capability_summary() probes every backend, so the
+# auth result is memoized per process with a TTL: platform_capabilities stays
+# fast/offline-safe, while a mid-session `gh auth login` is still picked up
+# within a few minutes (pre-release review finding 5).
+_AUTH_TTL_S = 300.0
+_auth_memo: tuple[float, CapabilityReport] | None = None
+
+
+def _clear_auth_memo() -> None:
+    """Testing hook: forget the memoized auth probe (mirrors cache.clear())."""
+    global _auth_memo
+    _auth_memo = None
+
+
+def _auth_probe(exe: str) -> CapabilityReport:
+    global _auth_memo
+    now = time.monotonic()
+    if _auth_memo is not None and now - _auth_memo[0] < _AUTH_TTL_S:
+        return _auth_memo[1]
+    try:
+        result = subprocess.run(
+            [exe, "auth", "status"], capture_output=True, text=True, timeout=10
+        )
+    except Exception as exc:  # noqa: BLE001 — surfaced as a diagnosable reason
+        report = CapabilityReport(
+            False, f"gh present but 'gh auth status' failed to run: {exc}", exe
+        )
+    else:
+        if result.returncode != 0:
+            report = CapabilityReport(
+                False,
+                "gh present but not authenticated (gh auth status failed); run "
+                "`gh auth login` to enable capture. Digesting an already-saved "
+                ".gha.log never needs authentication.",
+                exe,
+            )
+        else:
+            report = CapabilityReport(True, "gh present and authenticated", exe)
+    _auth_memo = (now, report)
+    return report
+
+
 def _probe() -> CapabilityReport:
     info = detect()
     exe = info.profilers_on_path.get("gha_log")
@@ -65,23 +109,7 @@ def _probe() -> CapabilityReport:
     # `gh` present is necessary but not sufficient for CAPTURE: pulling a real
     # run's log also needs an authenticated session. Digesting an already-saved
     # log needs neither check — that split is called out in the usage prompt.
-    try:
-        result = subprocess.run(
-            [exe, "auth", "status"], capture_output=True, text=True, timeout=10
-        )
-    except Exception as exc:  # noqa: BLE001 — surfaced as a diagnosable reason
-        return CapabilityReport(
-            False, f"gh present but 'gh auth status' failed to run: {exc}", exe
-        )
-    if result.returncode != 0:
-        return CapabilityReport(
-            False,
-            "gh present but not authenticated (gh auth status failed); run "
-            "`gh auth login` to enable capture. Digesting an already-saved "
-            ".gha.log never needs authentication.",
-            exe,
-        )
-    return CapabilityReport(True, "gh present and authenticated", exe)
+    return _auth_probe(exe)
 
 
 def _capture_command(target: str, info: PlatformInfo) -> str:

src/perfdigest/adapters/gha_log/gha_log_reader.py

@@ -198,6 +198,12 @@ def raw_metrics(report_path: str, kernel_index: int, name_filter: str) -> dict[s
         "error_lines": rec["error_lines"][:_MAX_RAW_ANNOTATION_LINES],
         "warning_lines": rec["warning_lines"][:_MAX_RAW_ANNOTATION_LINES],
     }
+    # Explicit truncation markers (only when truncation happened) — the caller
+    # must not have to infer it by comparing counts against list lengths.
+    if len(rec["error_lines"]) > _MAX_RAW_ANNOTATION_LINES:
+        out["error_lines_truncated_to"] = _MAX_RAW_ANNOTATION_LINES
+    if len(rec["warning_lines"]) > _MAX_RAW_ANNOTATION_LINES:
+        out["warning_lines_truncated_to"] = _MAX_RAW_ANNOTATION_LINES
     wanted = None if name_filter.lower() == "all" else name_filter.lower()
     if wanted is not None:
         out = {k: v for k, v in out.items() if wanted in k.lower()}

src/perfdigest/adapters/git_numstat/numstat_reader.py

@@ -34,6 +34,11 @@
   * Non-ASCII paths arrive C-quoted by git (``"docs/\\303\\266l\\303\\247..."``,
     core.quotePath default) and are kept VERBATIM as printed — the name is
     honest to the artifact; we do not unquote.
+  * Known ambiguity, inherent to the unquoted artifact: a plain file whose
+    NAME literally contains ``" => "`` (``notes about a => b.txt``) is
+    indistinguishable from a whole-path rename in numstat output — git prints
+    both identically — and parses as a rename here. git's own limitation;
+    ``expand`` shows ``path_as_printed`` so the raw line is always recoverable.
 
 ``duration_us`` is ``None`` for every unit: a change digest has no time
 dimension at all, so ``summarize_report`` falls back to file order and ranking

src/perfdigest/report_store/cache.py

@@ -48,7 +48,12 @@ def cached_units(
         # re-run and would serve stale units. Caching must stay behaviorally
         # invisible, so directory refs are parsed fresh every time (the trees
         # are small JSON files; the IO saving is not worth the staleness risk).
-        return loader(path)
+        # Same mutation-proofing as the cached path — the module invariant
+        # ("no caller can mutate shared state") holds for every return.
+        return [
+            replace(u, metrics=MappingProxyType(dict(u.metrics)))
+            for u in loader(path)
+        ]
     key = (backend_name, abspath, st.st_mtime_ns, st.st_size)
 
     with _LOCK:

src/perfdigest/server/prompts.py

@@ -12,10 +12,13 @@
 from perfdigest.server.app import mcp
 
 _HEADER = """\
-You have perfdigest: token-efficient access to performance-profiler reports across
-backends (NVIDIA, AMD HIP, CPU perf, Apple Metal, ptxas codegen, torch/Chrome
-traces). It is a translator, not a judge — it returns clean numbers; deciding
-"memory-bound?"/"occupancy-limited?" is YOUR job.
+You have perfdigest: token-efficient access to the development loop's report
+artifacts across four feedback channels — performance profilers (NVIDIA, AMD
+HIP, CPU perf, Apple Metal, torch/Chrome traces), build tools (ptxas codegen,
+clang -ftime-trace, cmake configure profiles, ninja logs, cargo diagnostics,
+criterion benchmarks), CI run logs (GitHub Actions), and repo change state
+(git numstat). It is a translator, not a judge — it returns clean numbers;
+deciding "memory-bound?"/"build-bottlenecked?"/"release-ready?" is YOUR job.
 
 perfdigest has TWO operations — keep them separate:
 

tests/test_criterion.py

@@ -113,18 +113,21 @@ def test_dir_without_estimates_raises_loudly_never_silent_empty(tmp_path):
     assert "no */new/estimates.json" in str(exc.value)
 
 
-def test_criterion_report_bookkeeping_dirs_are_skipped(tmp_path, fixtures_dir):
-    # criterion writes its own HTML under report/ dirs. Real trees put no
-    # estimates.json there, but the guard must hold even if one appeared
-    # (synthetic tree: a real estimates.json copied under a report/ path).
+def test_bench_named_report_is_digested_never_suppressed(tmp_path, fixtures_dir):
+    # criterion's own HTML bookkeeping lives under report/ dirs but NEVER
+    # contains new/estimates.json, so the glob alone excludes it. A user CAN
+    # name a bench "report" (criterion's known dir-collision wart) and its
+    # genuinely-written estimates must appear — suppressing them by path name
+    # would fabricate absence (pre-release review finding 2; synthetic tree
+    # built from a real estimates.json).
     root = tmp_path / "criterion"
     real = fixtures_dir / "criterion_sample" / "criterion" / "fib_plain" / "new" / "estimates.json"
-    for rel in ("mybench/new", "report/mybench/new"):
+    for rel in ("mybench/new", "report/new"):
         d = root / rel
         d.mkdir(parents=True)
         shutil.copy(real, d / "estimates.json")
     units = tools.list_kernels(str(root), FMT)
-    assert [u["name"] for u in units] == ["mybench"]  # report/ never a benchmark
+    assert [u["name"] for u in units] == ["mybench", "report"]
 
 
 def test_rerun_overwrite_is_seen_immediately(tmp_path, fixtures_dir):

tests/test_gha_log.py

@@ -296,6 +296,7 @@ class _Unauthed:
         returncode = 1
 
     monkeypatch.setattr(gha_backend.subprocess, "run", lambda *a, **k: _Unauthed())
+    gha_backend._clear_auth_memo()  # TTL memo (finding 5) must not leak stages
     report = gha_backend._probe()
     assert report.available is False
     assert "auth" in report.reason.lower()
@@ -305,6 +306,7 @@ class _Authed:
         returncode = 0
 
     monkeypatch.setattr(gha_backend.subprocess, "run", lambda *a, **k: _Authed())
+    gha_backend._clear_auth_memo()  # TTL memo (finding 5) must not leak stages
     report = gha_backend._probe()
     assert report.available is True
     assert report.tool == "/usr/bin/gh"