fix(ci): count a base-side benchmark failure, and stop calling the run's spread a noise floor

[marius-bughiu]

Follow-up to #353, which merged while its review loop was still running. Copilot's last pass raised two findings as suppressed comments — so they had no thread to reply on — and both are real. Neither changes the 3σ rule itself.

Closes nothing on its own; it finishes #351.

A base-side failure was never counted

baseMap.set(b.FullName, b.Statistics) stores null when the base side of a pair produced no statistics. The row then rendered ⚠️ base errored, but errored was never incremented — so the subtitle could read "No significant change vs main" over a pair that could not be compared at all.

That is the same false reassurance a too-permissive guard gives, which is the whole subject of #351. The branch is counted now, and the self-test fixture gained a base-null benchmark so the count and the subtitle are both pinned.

"Measured noise floor" overclaimed, in the dangerous direction

The published percentiles are computed over every paired delta, so they contain the PR's own effects, not just the runner's drift. On a typical PR — a handful of ~750 rows touched — that distinction doesn't matter. On a change to a shared primitive it moves many rows at once and raises the very figures the footer invites you to measure the flag against.

So a reviewer looking at a real, broad regression would have been told by the tool that their p95 was 20% and their flag didn't stand out. That is the opposite failure to the one #353 fixed, and arguably the worse of the two: a false positive wastes an hour, a talked-away true positive ships.

Renamed to "Observed spread of this run", and the footer now states what is in the sample:

Observed spread of this run — |Δ| across all 757 paired rows: p50 0.7%, p90 6.2%, p95 10.3% (nearest-rank). A pull request usually touches a handful of these, so this is mostly the runner's own drift between the two slices — but a change to a shared primitive moves many rows at once and would raise these figures itself, so read it as the run's spread rather than as a floor the PR cannot have caused. A flag that does not stand out against it is worth re-running before acting on.

CONTRIBUTING.md and CHANGELOG.md carry the same correction, and a self-test check asserts the phrase "noise floor" never reaches the comment body again.

Why not derive the floor from control rows

The reviewer's other suggestion was to compute the floor from demonstrably unchanged benchmarks. The obvious candidates are the BCL baseline arms (Dictionary_*, HashSet_*) — identical code on both sides by construction, and four of the original 13 false positives were exactly those. But the residual noise this rule cannot reach is layout noise, which moves those arms too, so a control group would understate the tail while looking more authoritative than the number it replaced. Reporting the full distribution and being explicit about what is in it is the honest version.

Verification

• node scripts/benchmark_comment.js --self-test — 64 checks.
• Replayed against the real 757-row reports from run 31145509269 (the zero-IL pair from Benchmark comment flags ~13 rows on a zero-IL diff: the std-dev guard measures within-run spread, not between-run drift #351): still 2 regressions / 0 improvements, p50/p90/p95 of 0.7% / 6.2% / 10.3%.
• Both workflows still parse.

[Copilot]

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Fixes benchmark comment reporting to correctly count base-side benchmark failures as “errored” and clarifies the footer/Docs wording to avoid overclaiming a “noise floor” when the distribution includes PR-caused deltas.

Changes:

• Count “base-side errored” benchmarks in the errored total so the subtitle can’t claim “No significant change” when comparisons are missing.
• Rename “noise floor” wording to “observed spread” and expand the footer text to explain what the percentiles represent.
• Update self-test fixture and docs (CONTRIBUTING/CHANGELOG) to reflect the corrected semantics.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
scripts/benchmark_comment.js	Counts base-side failures in errored, updates footer wording, and extends self-tests to pin behavior and prevent “noise floor” terminology in output.
CONTRIBUTING.md	Updates contributor-facing explanation from “measured noise floor” to “observed spread”, clarifying interpretation.
CHANGELOG.md	Updates changelog entry to match the new “observed spread” terminology.

Suppressed comments (1)

scripts/benchmark_comment.js:145

• This PR intentionally stops calling the published percentiles a “noise floor”, but the internal API still uses noiseProfile / noise. That mismatch is likely to reintroduce confusion during future edits (and it’s easy to accidentally resurrect “noise floor” wording when the variable itself is noise). Consider renaming noiseProfile → spreadProfile (or similar) and noise → spread (including the returned object key from buildComment) to align code terminology with the corrected semantics.

function noiseProfile(deltaPercents) {
  if (deltaPercents.length === 0) return null;
  const sorted = [...deltaPercents].sort((a, b) => a - b);
  return {
    n: sorted.length,
    p50: percentile(sorted, 50),
    p90: percentile(sorted, 90),
    p95: percentile(sorted, 95),
  };
}

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

CHANGELOG.md:30

• The changelog still omits the other user-visible fix in this PR: a base-side benchmark failure now contributes to the warning count instead of allowing a falsely clean subtitle. Add a terse entry under Fixed so the release notes cover both shipped behavior changes.

- That comment now publishes the run's own **observed spread** — the p50, p90 and p95 of |Δ| across every paired row — so a flag can be read against the run it arrived in instead of an assumed floor. Closes [#351](https://github.com/marius-bughiu/Celerity/issues/351).

[marius-bughiu]

Picking up the suppressed comment from the latest pass — right, and fixed in 6192c54.

The changelog covered the spread rename but not the other user-visible change here, which is arguably the more consequential of the two: a row whose main side failed to measure, or whose two means cannot be compared at all, now reaches the comment's warning total instead of sitting quietly in the table under a summary line reading "No significant change vs main". Added under Fixed:

A benchmark whose main side failed to measure, or whose two means cannot be compared at all, is now counted in that comment's warning total. Previously such a row appeared in the table while the summary line still read "No significant change vs main". Closes #351.

Kept to two sentences per the repo's changelog guidance, and it ends with Closes to match the surrounding entries. check_doc_anchors.js still resolves all 569 links.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.