Skip to content

fix: repair touchstone benchmark CI#2688

Merged
schochastics merged 2 commits into
mainfrom
fix-touchstone-ci
Jun 4, 2026
Merged

fix: repair touchstone benchmark CI#2688
schochastics merged 2 commits into
mainfrom
fix-touchstone-ci

Conversation

@schochastics

Copy link
Copy Markdown
Contributor

The touchstone benchmark CI added in #2685 failed on its first run (example). Three linked problems:

1. Receive workflow skipped every maintainer PR (root cause)

prepare gated on author_association == OWNER/MEMBER/COLLABORATOR. That value comes from the pull_request webhook payload, which reports CONTRIBUTOR/NONE (never MEMBER) for maintainers whose org membership is private — so the gate was false, both jobs skipped, and no "pr" artifact was produced.

→ Gate on the PR head being a same-repo branch instead. Creating a same-repo branch requires push access, so this is the real security boundary against untrusted fork code, and it's immune to membership visibility.

2. Comment workflow crashed instead of no-op'ing (the visible error)

It only checked workflow_run.event == 'pull_request', so it ran after the skipped receive run and died downloading the missing artifact: Cannot read properties of undefined (reading 'id').

→ Also require workflow_run.conclusion == 'success'.

3. Deprecated ::set-output (latent)

Would have left needs.prepare.outputs.config empty once prepare actually ran, breaking the fromJson build matrix.

→ Switch to $GITHUB_OUTPUT, compacting config.json with jq -c (drops the multi-line escaping hack).

This PR touches .github/workflows/touchstone-*.yaml, which is in the receive trigger's path filter, so it exercises the fixed workflow itself.

🤖 Generated with Claude Code

The two-workflow touchstone setup failed: the receive workflow skipped
every maintainer PR and the comment workflow then crashed on the missing
artifact.

- receive: gate `prepare` on the PR head being a same-repo branch instead
  of `author_association`. The association comes from the webhook payload,
  which reports CONTRIBUTOR/NONE (never MEMBER) for users with private org
  membership, so legitimate maintainer PRs were silently skipped. The
  same-repo check is the real security boundary (push access is required to
  create a branch) and is immune to membership visibility.
- receive: replace the deprecated `::set-output` command with $GITHUB_OUTPUT,
  compacting config.json via `jq -c` so no multi-line escaping is needed.
- comment: also require `workflow_run.conclusion == 'success'` so the job
  no-ops instead of crashing ("Cannot read properties of undefined
  (reading 'id')") when receive was skipped or failed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@schochastics schochastics enabled auto-merge (squash) June 4, 2026 11:26
@schochastics schochastics disabled auto-merge June 4, 2026 11:26
The released touchstone tags (v1, v1.0.1) still call actions/upload-artifact@v2
and actions/download-artifact@v2, which GitHub now automatically fails. Only
the upstream default branch has been updated to artifact@v4/v5 and
github-script@v7, but no release includes that fix yet. Pin both the receive
and comment actions to that default-branch commit until upstream tags a release.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@schochastics

Copy link
Copy Markdown
Contributor Author

Validation status

The receive workflow runs from the PR branch and is fully green here: it ran the benchmarks and uploaded the pr, results, and visual-benchmarks artifacts (run). This validates the three receive-side fixes (same-repo gate, $GITHUB_OUTPUT, SHA pin).

The comment workflow is workflow_run-triggered, which GitHub always runs from the default branch (main) — never from the PR branch. So the "Continuous Benchmarks (Comment)" check on this PR still executes the old main code (comment@v1 + github-script@v3) and fails with 401 InvalidAuthenticationInfo while downloading the artifact from Azure storage — the exact bug fixed by pinning to a commit that uses actions/download-artifact@v5.

That red check is expected and cannot turn green until this PR is merged. The pinned comment action is a verified drop-in (same GITHUB_TOKEN input; replaces the hand-rolled github-script download with actions/download-artifact@v5). After merge, the next PR touching R/** / touchstone/** will exercise the full receive → comment flow end-to-end.

@schochastics schochastics merged commit d08ece8 into main Jun 4, 2026
8 of 9 checks passed
@schochastics schochastics deleted the fix-touchstone-ci branch June 4, 2026 11:55
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.

1 participant