Skip to content

ci(perf-repeat): post the shadow-check report as a sticky PR comment - #708

Open
mparrett wants to merge 2 commits into
mainfrom
wt/perf-repeat-comment
Open

ci(perf-repeat): post the shadow-check report as a sticky PR comment#708
mparrett wants to merge 2 commits into
mainfrom
wt/perf-repeat-comment

Conversation

@mparrett

@mparrett mparrett commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

perf-pr-repeat.yml writes a job summary and uploads its evidence, but has no comment step, so a 65-minute run leaves nothing on the PR. On #706 I had to download the run artifact and read aggregate.json to find out what the lane had concluded. The shadow phase is supposed to be collecting false-positive and provenance data, and output nobody sees does not get compared.

This adds the comment job, mirroring the one already in perf-pr.yml rather than inventing a second pattern: separate job so the elevated token is scoped away from the one that checks out and builds PR code, sticky upsert by marker so repeated pushes update in place, continue-on-error because fork PRs get a read-only token and that should not redden the run.

The comment carries the aggregate — provenance header, family table, would-gate verdict — and points at the job summary for the per-cycle logs. The first self-test posted the whole report instead: 120 lines with seven literal ::group:: markers, which are Actions log-folding directives that do not render in a comment. Trimming to the text after the last ::endgroup:: takes it to 36 lines.

Two choices worth surfacing

The marker is perf-pr-repeat-report, deliberately not perf-pr-report. Both lanes run on the same PR during the shadow phase, and the header of this workflow says comparing them is the point, so neither may overwrite the other.

The comment is gated on bench success. The bench job exits nonzero when families are not comparable across cycles. Posting a delta table built from a measurement already known to be broken seems worse than posting nothing, and the job summary still carries the report in that case. Easy to relax if you would rather see the integrity failure on the PR too.

Verification

actionlint clean, including its shellcheck pass — the file was clean before and still is. The change is inert unless the perf-repeat label is applied, and since .github/workflows/perf-pr-repeat.yml is inside the workflow's own paths filter and same-repo PRs run it from the PR branch, labeling this PR exercises the new job on itself.

@mparrett mparrett added the perf-repeat Run the repeat A/B (variance-reduced) perf check label Aug 9, 2026
@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown

Repeat A/B (variance-reduced) — informational shadow check


=== interleaved repeat A/B — N=7, profile=pr-fast (INFORMATIONAL, never gates) ===
runner: AMD EPYC 7763 64-Core Processor  4 CPU  go1.26.5
base 23f3ab15b02f  head 3f22fe9ea166  order BH HB BH HB BH HB BH
anchor stability per cycle (base/head ns/op, should stay flat — it measures compute, not memory):
  cycle 1: base 1.246  head 1.246  Δ+0.0%
  cycle 2: base 1.245  head 1.245  Δ+0.0%
  cycle 3: base 1.245  head 1.246  Δ+0.1%
  cycle 4: base 1.245  head 1.246  Δ+0.1%
  cycle 5: base 1.245  head 1.246  Δ+0.1%
  cycle 6: base 1.245  head 1.247  Δ+0.2%
  cycle 7: base 1.247  head 1.246  Δ-0.1%

family                                    median%  worst% >prim
----------------------------------------------------------------
VectorConj/PersistentVector/10              +1.54   +7.94     0
VariadicInvoke/1Arg                         +1.37   -8.57     0
VectorConj/ArrayVector/100                  +1.36   +7.79     0
MapAssoc/GoMap-Lookup/01000                 +0.78  +44.21     1
VectorCreation/PersistentVector/10000       +0.44   +1.96     0
MapAssoc/HAMT-Dissoc/01000                  +0.35   -3.26     0
VectorCreation/ArrayVector/100              +0.35   -5.80     0
VectorConj/PersistentVector/100             +0.34   +6.82     0
FrameDispatch                               +0.33   +0.93     0
MapAssoc/HAMT-Dissoc/10                     +0.33   -2.31     0
VectorCreation/ArrayVector/10000            +0.28   +6.41     0
VectorConj/ArrayVector/1000                 +0.26   +4.09     0
... 41 more (full table in aggregate.json)

would-gate (any comparable family's median > budget):
  budget    6%:  clean  
  budget    8%:  clean  
  budget   10%:  clean  
confirm K=2/7 @ 10% (secondary): clean

(no-op PR: every would-gate above is a false positive; each is one sample toward P(any-gate).)

Per-cycle logs are in the job summary. Per-snapshot evidence — raw samples, cycle order, provenance — is in the perf-pr-repeat artifact on this run.

Informational only. This check never gates the PR — the workflow header says why.

@nnunley

nnunley commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

It self-tested, which is the nicest kind of evidence: the sticky comment is already rendered on the PR with the aggregate table, the would-gate verdict, the artifact link, and no leaked ::group:: noise.

Shell holds under set -euo pipefail — the grep -n … | tail -1 | cut … || true guard covers grep's exit 1, the no-groups path falls back to the whole report, and rc is captured and re-exited after comment.md is written, so an integrity failure stays red while the summary and artifact survive. The distinct marker (perf-pr-repeat-report vs perf-pr-report) means the two shadow lanes cannot overwrite each other, and keeping the elevated token in a separate job mirrors perf-pr.yml:173-210.

One nit that is worth fixing in both lanes rather than just this one. github.rest.issues.listComments({owner, repo, issue_number}) is unpaginated — 30 per page by default — so on a PR past 30 comments the marker is not found and the "sticky" comment silently duplicates on every run. I checked: perf-pr.yml:207 has the same call, so this is inherited rather than introduced. github.paginate(github.rest.issues.listComments, …) fixes both. This repo's PRs do get long enough to hit it.

Two smaller ones, neither blocking: the report text originates from PR-branch code and is interpolated inside a ``` fence, so a head branch could break out of the fence — same exposure as the existing lane, and the reason the token is in a separate job, but the artifact content is still head-controlled. And a stale sticky comment persists if the perf-repeat label is later removed; no cleanup path.

Worth noting what this unblocks: #706's author had to hand-post benchmark results because this step did not exist yet.

@mparrett
mparrett requested review from nnunley and nooga August 11, 2026 23:55
mparrett added a commit that referenced this pull request Aug 12, 2026
listComments defaults to 30 per page. Past that the marker is not found and
the "sticky" upsert silently creates a duplicate on every run. Flagged on
#708 for both lanes — this was inherited by the repeat comment job from
perf-pr.yml, so fix them together.

Co-authored-by: Cursor <cursoragent@cursor.com>
@mparrett
mparrett force-pushed the wt/perf-repeat-comment branch 2 times, most recently from afb4814 to 3f22fe9 Compare August 12, 2026 14:39
mparrett added a commit that referenced this pull request Aug 12, 2026
listComments defaults to 30 per page. Past that the marker is not found and
the "sticky" upsert silently creates a duplicate on every run. Flagged on
#708 for both lanes — this was inherited by the repeat comment job from
perf-pr.yml, so fix them together.

Co-authored-by: Cursor <cursoragent@cursor.com>
@mparrett mparrett removed the perf-repeat Run the repeat A/B (variance-reduced) perf check label Aug 13, 2026
mparrett and others added 2 commits August 13, 2026 11:48
The repeat A/B lane writes a job summary and uploads its evidence, but has no
comment step, so a 65-minute run leaves nothing on the PR. On #706 the results
had to be pulled out of the run artifact by hand to be read at all -- which
means in practice the shadow check's output is only seen by whoever remembers
to go looking.

Mirrors perf-pr.yml's existing comment job rather than inventing a second
pattern: separate job so the elevated token is scoped away from the one that
builds PR code, sticky upsert by marker, continue-on-error for fork PRs whose
token is read-only.

Two deliberate choices:

- The marker is `perf-pr-repeat-report`, distinct from perf-pr.yml's. Both
  lanes run on the same PR during the shadow phase and comparing them is the
  point, so neither may overwrite the other.
- Gated on bench success. The bench job exits nonzero when families are not
  comparable across cycles; posting a delta table from a measurement already
  known to be broken is worse than posting nothing, and the summary still
  carries it.

The comment body is the same text as the job summary, plus a link to the run
for the per-snapshot evidence artifact. actionlint clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The self-test on this PR posted the whole report: 120 lines, 9.6 KB, with
seven literal `::group::` markers. Those are Actions log-folding directives
and render as text in a comment body, so the useful part -- the family table
and the would-gate verdict -- sat under a hundred lines of per-cycle bench
output.

The comment now starts after the last `::endgroup::`, which is where the
aggregate begins: 36 lines, 1.8 KB, opening on the provenance header. The job
summary is unchanged and still carries the per-cycle logs, and the comment
points at it. Falls back to the full report if no group markers are found, so
a format change or an early driver death posts something rather than nothing.

Measured against the report this PR's own previous run produced.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@mparrett
mparrett force-pushed the wt/perf-repeat-comment branch from 3f22fe9 to 559b821 Compare August 13, 2026 18:48
mparrett added a commit that referenced this pull request Aug 13, 2026
listComments defaults to 30 per page. Past that the marker is not found and
the "sticky" upsert silently creates a duplicate on every run. Flagged on
#708 for both lanes — this was inherited by the repeat comment job from
perf-pr.yml, so fix them together.

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants