Skip to content

ci(benchmarks): split commit/push out of bench job#14480

Open
emmajam wants to merge 1 commit intofoundry-rs:masterfrom
emmajam:fix/benchmarks-publish-refactor
Open

ci(benchmarks): split commit/push out of bench job#14480
emmajam wants to merge 1 commit intofoundry-rs:masterfrom
emmajam:fix/benchmarks-publish-refactor

Conversation

@emmajam
Copy link
Copy Markdown
Collaborator

@emmajam emmajam commented Apr 28, 2026

Motivation

The Foundry Benchmarks workflow has been failing on the Commit and read benchmark results step (example run):

fatal: could not read Username for 'https://github.com': No such device or address
Error: Process completed with exit code 128.

The bench job (run-benchmarks) checks out with persist-credentials: false (correctly so — it then executes untrusted third-party builds via foundryup and forge against external repos like ithacaxyz/account and Vectorized/solady), so the in-script git push had no token and exited 128.

The minimal fix would be persist-credentials: true on that checkout, but that permanently parks a contents: write GitHub token on the self-hosted runner alongside untrusted code — not a tradeoff worth one line.

Solution

Split responsibilities cleanly between the two existing jobs:

  • run-benchmarks (self-hosted foundry-runner): runs benches, uploads artifact, exposes pr_comment as a job output. Permissions reduced to contents: read. No git writes.
  • publish-results (ubuntu-latest): downloads the artifact, commits benches/LATEST.md, pushes the branch, and (for workflow_dispatch) opens the results PR / comments on the target PR. This is the only job with contents: write.

commit-and-read-benchmarks.sh is split to match its new homes:

  • read-benchmark-results.sh — pure read, emits GitHub Actions outputs.
  • commit-benchmark-results.sh — branch creation, commit, push.

Also removes the duplicate Push branch for manual runs step in publish-results that was attempting to push a branch which had only ever existed on the bench runner.

Why this is better long term

  1. Token isolation. A compromised dependency in any benched repo (or in foundryup's install path) can no longer reach a write-scoped token, since the bench job has none. The blast radius shrinks to the lightweight ubuntu-latest job that only touches the artifact.
  2. Single source of truth for git. Both jobs were previously trying to push the results branch. Centralizing all git/PR logic in publish-results removes the duplication and the broken second push.
  3. Replayable. With the bench output captured as an artifact, publish-results can be re-run independently without spending another 30+ minutes regenerating numbers.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

The benchmarks workflow was failing on the 'Commit and read benchmark
results' step with:

    fatal: could not read Username for 'https://github.com'

The bench job checks out with persist-credentials: false (correctly,
since it executes untrusted third-party builds via foundryup and forge
against external repos), so 'git push' had no auth and exited 128.

Rather than re-enable persist-credentials on the self-hosted runner —
which would park a contents:write token alongside untrusted code — this
splits responsibilities cleanly:

* run-benchmarks (foundry-runner): runs benches, uploads artifact,
  exposes pr_comment as a job output. Permissions reduced to
  contents: read. No git writes.
* publish-results (ubuntu-latest): downloads artifact, commits
  benches/LATEST.md, pushes the branch, and (for workflow_dispatch)
  opens the results PR / comments on the target PR. This is the only
  job with contents: write.

The single commit-and-read-benchmarks.sh script is split to match:

* read-benchmark-results.sh — pure read, emits GitHub Actions outputs.
* commit-benchmark-results.sh — branch creation, commit, push.

Also removes the duplicate 'Push branch for manual runs' step in
publish-results that was attempting to push a branch which had never
been created on the publish runner.

Amp-Thread-ID: https://ampcode.com/threads/T-019dd30a-bf52-745c-a966-e06f0e6cf363
Co-authored-by: Amp <amp@ampcode.com>

ITHACAXYZ_ACCOUNT: "ithacaxyz/account:v0.3.2"
VECTORIZED_SOLADY: "Vectorized/solady:v0.1.22"
DEFAULT_REPOS: "ithacaxyz/account:v0.3.2,Vectorized/solady:v0.1.22"
Copy link
Copy Markdown
Member

@zerosnacks zerosnacks Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a regression, we need to retain the changes @mablr made

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

env:
# Repos to benchmark per step. Each comma-separated entry has the form
# org/repo[:rev][ <extra args>]
# where anything after the first whitespace is appended to every benchmark
# command for that repo (use this to skip a broken test contract via e.g.
# `--nmc BrokenTest`, so a single failing test does not fail the whole CI).
TEST_REPOS: >-
ithacaxyz/account:v0.5.7,
vectorized/solady:v0.1.26 --nmc 'LifebuoyTest|LibBitTest|Base58Test',
aave/aave-v4:af1f0f2ba323ac6fbaaee3abf6be060c78e22d35,
uniswap/v4-core:46c6834698c48bc4a463a86d8420f4eb1d7f3b75 --nmc 'TickMathTestTest',
sparkdotfi/spark-psm:v1.0.0 --nmc PSMInvariants_TimeBasedRateSetting_WithTransfers_WithPocketSetting
ISOLATE_TEST_REPOS: >-
ithacaxyz/account:v0.5.7 --nmc SimulateExecuteTest,
vectorized/solady:v0.1.26 --nmc 'SafeTransferLibTest|LifebuoyTest|LibBitTest|Base58Test',
aave/aave-v4:af1f0f2ba323ac6fbaaee3abf6be060c78e22d35,
uniswap/v4-core:46c6834698c48bc4a463a86d8420f4eb1d7f3b75 --nmc 'TickMathTestTest',
sparkdotfi/spark-psm:v1.0.0 --nmc PSMInvariants_TimeBasedRateSetting_WithTransfers_WithPocketSetting
BUILD_REPOS: >-
ithacaxyz/account:v0.5.7,
vectorized/solady:v0.1.26,
aave/aave-v4:af1f0f2ba323ac6fbaaee3abf6be060c78e22d35,
uniswap/v4-core:46c6834698c48bc4a463a86d8420f4eb1d7f3b75,
sparkdotfi/spark-psm:v1.0.0
COVERAGE_REPOS: >-
ithacaxyz/account:v0.5.7,
aave/aave-v4:af1f0f2ba323ac6fbaaee3abf6be060c78e22d35,
uniswap/v4-core:46c6834698c48bc4a463a86d8420f4eb1d7f3b75,
sparkdotfi/spark-psm:v1.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants