Overhaul workflows: dedup, supply-chain hardening, bug fixes, e2e tests#52
Merged
Merged
Conversation
…de-safety and broadcast parsing bugs Bugs: - _foundry-cicd.yml no longer fails for consumers without upgrades.json: upgrade safety is skipped when the config file is absent, and deploys accept a skipped upgrade-safety job (fixes the documented minimum setup) - CREATE2 deployments are now included in summaries, deployment.json, and Blockscout verification; unnamed nested deployments (additionalContracts) are surfaced as a warning instead of silently dropped Structure: - new shared composite action (.github/actions/setup) replaces 9 copies of the Foundry/Node/deps setup preamble, adds Foundry compile caching - _foundry-cicd.yml is now a thin orchestrator calling _ci.yml, _upgrade-safety.yml and _deploy-testnet.yml as nested reusable workflows (581 -> 290 lines), eliminating the drift between copies - unified deploy behavior: chain-id-based network resolution, deployment.json artifact, and --private-key are now consistent (network-index removed) - main-branch input renamed to base-branch to match _upgrade-safety.yml Hardening: - all actions pinned to commit SHAs; foundry-version, halmos-version and oz-upgrades-core-version inputs added; Node default bumped to 22 - workflow inputs routed through env vars instead of run-block interpolation; test-verbosity validated against an allowlist - deploys restricted to same-repo PRs (fork PRs have no secrets) and serialized per ref via job-level concurrency, never cancelled mid-flight - scripts checkout defaults to job.workflow_sha, so the bash scripts always match the workflow commit (fixes the etherform-ref split-brain) - optional protected GitHub Environment support for mainnet deploys Robustness: - parse-broadcast: chain id read from the artifact, DEPLOY_SCRIPT scoping, hard error on ambiguous multiple broadcasts (was: silent head -1) - extract-summary: POSIX-portable parsing (no GNU grep -P; tests now pass on macOS), handles the forge >= 1.7 table format - validate.sh: OZ CLI installed once instead of npx per contract; per-contract "options" passthrough for --unsafeAllow* flags - upgrades.json removal guard unchanged; deployment artifact logic extracted to write-deployment-json.sh with tests Testing & docs: - new e2e workflow runs the real _foundry-cicd.yml against a dependency-free fixture Foundry project on every PR; actionlint added to repo CI; shellcheck now covers tests/ - README updated (mainnet deploys, concurrency guidance, explicit secrets, new inputs, ref-pinning advice); original spec marked partially superseded
Contributor
There was a problem hiding this comment.
Pull request overview
This PR significantly refactors Etherform’s CI/CD workflow structure by deduplicating repeated setup logic into a shared composite action, tightening supply-chain/injection posture, fixing deployment/broadcast parsing (including CREATE2 handling), and adding unit + end-to-end tests to validate the reusable workflow behavior.
Changes:
- Introduces a shared
.github/actions/setupcomposite action and rewires reusable workflows to use it (plus SHA pinning and workflow hardening). - Fixes deployment parsing/artifacts to include CREATE2 and avoid silently dropping unnamed nested deployments; improves broadcast selection/scoping.
- Expands the test suite with new unit tests, fixtures, and a full E2E workflow that runs
_foundry-cicd.ymlagainst a minimal Foundry fixture project.
Reviewed changes
Copilot reviewed 23 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test-write-deployment-json.sh | Adds unit coverage for new write-deployment-json.sh artifact writer. |
| tests/test-parse-broadcast.sh | Updates tests for chain-id parsing, CREATE2 inclusion, and ambiguous broadcast handling. |
| tests/test-extract-summary.sh | Adds regression test for forge ≥ 1.7 coverage table parsing. |
| tests/fixtures/project/test/Counter.t.sol | Adds dependency-free Foundry test fixture for E2E workflow. |
| tests/fixtures/project/src/Counter.sol | Adds minimal contract fixture used by E2E runs. |
| tests/fixtures/project/foundry.toml | Configures the fixture Foundry project. |
| tests/fixtures/coverage-raw-forge17.txt | Adds forge 1.7-style coverage output fixture. |
| tests/fixtures/broadcast/Deploy.s.sol/31337/run-latest.json | Extends broadcast fixture with CREATE2, additionalContracts, and chain field. |
| scripts/upgrade-safety/validate.sh | Improves upgrade validation by installing OZ CLI once and supporting per-contract options. |
| scripts/deploy/write-deployment-json.sh | New script to produce deployments/<network>/deployment.json including CREATE2. |
| scripts/deploy/verify-blockscout.sh | Expands verification to CREATE2 and adds a “nothing to verify” warning exit. |
| scripts/deploy/parse-broadcast.sh | Hardens broadcast discovery/scoping and includes CREATE2 + warning for unnamed nested deployments. |
| scripts/coverage/extract-summary.sh | Makes parsing POSIX-portable and compatible with forge ≥ 1.7 output format. |
| README.md | Updates usage/config docs for new inputs, hardening guidance, and mainnet deploy pattern. |
| docs/specs/ci-cd-automated-deployments.md | Marks original spec as partially superseded and points readers to README. |
| .gitignore | Ignores fixture Foundry build artifacts. |
| .github/workflows/test.yml | Adds actionlint job and expands lint/test execution coverage. |
| .github/workflows/e2e.yml | Adds E2E workflow running _foundry-cicd.yml against the fixture project. |
| .github/workflows/_upgrade-safety.yml | Adds new inputs/pins and switches to shared setup action + workflow_sha pinning. |
| .github/workflows/_foundry-cicd.yml | Refactors into an orchestrator calling _ci.yml, _upgrade-safety.yml, _deploy-testnet.yml. |
| .github/workflows/_deploy-testnet.yml | Generalizes deploy workflow, adds environment/concurrency, and writes deployment artifact. |
| .github/workflows/_ci.yml | Adds working-directory/foundry-version/halmos-version, validates verbosity, and uses shared setup. |
| .github/dependabot.yml | Enables Dependabot updates for SHA-pinned actions (including composite action directory). |
| .github/actions/setup/action.yml | New shared toolchain setup composite action (Foundry/Node/deps/cache). |
| .github/actionlint.yaml | Configures actionlint ignores for newer job.workflow_sha/workflow_ref context fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+21
to
+23
| run: | | ||
| bash <(curl -sSfL https://raw.githubusercontent.com/rhysd/actionlint/v1.7.7/scripts/download-actionlint.bash) 1.7.7 | ||
| ./actionlint -color |
Contributor
Author
There was a problem hiding this comment.
Fixed in 1bec5c4: the workflow now downloads the release tarball directly and verifies its sha256 against the value from the published actionlint_1.7.7_checksums.txt before executing, instead of piping the tag-addressed install script into bash.
…ignore-scripts - test.yml now downloads the actionlint release tarball directly and verifies its sha256 against the published checksum, instead of piping a tag-addressed script from raw.githubusercontent.com into bash - validate.sh installs @openzeppelin/upgrades-core with --ignore-scripts so no install-time lifecycle scripts run in CI
This was referenced Jul 3, 2026
Renaming main-branch -> base-branch was gratuitously breaking for existing consumers (safety-net, saving-circles). Restore main-branch as an optional deprecated alias on _foundry-cicd.yml: when set it overrides base-branch and emits a deprecation warning, so existing callers keep working unchanged.
Contributor
Author
Follow-up:
|
| Consumer | Before this commit | Now |
|---|---|---|
safety-net (@main, main-branch: dev) |
❌ broke on merge | ✅ keeps working |
saving-circles (pinned, main-branch: dev) |
latent break on pin bump | ✅ no break on pin bump |
| crowdstake.fun (script sparse-checkout) | ✅ fine | ✅ fine |
Verified live cross-repo against safety-net (now-closed test PR #88): with the unchanged main-branch: dev pointed at this branch, Build & Test passed, the deprecation warning fired, main-branch correctly routed into the base-branch comparison (Building base branch (dev)...), and upgrade-safety still failed on an injected storage-layout violation.
The remaining breaking changes in the PR description stand (they affect no current org consumer): network-index removed from _deploy-testnet.yml (no direct callers), and nested check-name changes (safety-net's dev has no branch protection). The main-branch breaking-change note is now superseded by this deprecation.
This was referenced Jul 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the full codebase improvement review: two user-facing bugs, the structural dedup refactor, a supply-chain/injection hardening pass, robustness fixes, missing features from the spec, and self-testing CI.
Bug fixes
_foundry-cicd.yml's upgrade-safety job hard-errored for any consumer without.github/upgrades.json(and blockeddeploy-on-prwith it). It is now skipped when the config file is absent, and the deploy gate accepts a skipped upgrade-safety job. The standalone_upgrade-safety.ymlkeeps the hard error, since calling it is an explicit opt-in.deployment.json, and Blockscout verification. Unnamed nested deployments (additionalContracts) are reported as a warning instead of vanishing.Structure
.github/actions/setupreplaces nine copies of the checkout/Foundry/Node/install preamble and adds Foundry compile caching. Jobs get it via the existing sparse checkout of etherform, pinned tojob.workflow_sha— so the scripts, setup action, and workflow are always the same commit (fixes theetherform-refsplit-brain pitfall; the input remains as an override)._foundry-cicd.ymlis now a thin orchestrator (581 → 290 lines) calling_ci.yml,_upgrade-safety.yml, and_deploy-testnet.ymlas nested reusable workflows. The behavioral drift between the copies is resolved by unifying on: chain-id-based network resolution,deployment.jsonartifact output, and passing--private-key(all previously inconsistent between_foundry-cicd.ymland_deploy-testnet.yml).Hardening
env:instead of interpolated intorun:blocks;test-verbosityvalidated against an allowlist.cancel-in-progress: false.foundry-version(defaultstable),halmos-version,oz-upgrades-core-version; Node default bumped 20 → 22 (20 is past EOL).secrets: inherit, a caller-sideconcurrencyblock, and pinning the workflow ref.Robustness
parse-broadcast.sh: chain id from the artifact'schainfield (path fallback),DEPLOY_SCRIPTscoping, and a hard error on ambiguous multiple broadcasts (was: arbitraryhead -1).extract-summary.sh: POSIX-portable parsing — no GNUgrep -P, so the test suite now passes on macOS — and a regression test for the forge ≥ 1.7 table format.validate.sh: installs the OZ CLI once instead ofnpxresolution per contract; supports per-contract"options": ["--unsafeAllowRenames"]passthrough.write-deployment-json.shwith unit tests.Features
_deploy-testnet.ymlgains anenvironmentinput; README Step 6 documents the on-merge mainnet pattern.working-directoryinput on_ci.yml/_foundry-cicd.ymlfor monorepos (upgrade safety and deploy still assume repo root — documented).Testing
e2e.ymlruns the real_foundry-cicd.ymlagainst a dependency-free fixture Foundry project (tests/fixtures/project) on every PR: change detection, nested_ci.yml(fmt/build/test), coverage extraction + 90% threshold, and the graceful upgrade-safety skip. This would have caught the missing-upgrades.jsonbug.test.ymladds actionlint, extends ShellCheck totests/, and auto-discovers test files.shellcheckclean,actionlintclean, and the fixture project passesforge fmt --check,forge test, andforge coverage(100%) on forge 1.7.1.Breaking changes for consumers
_foundry-cicd.yml:main-branchrenamed tobase-branch(now consistent with_upgrade-safety.yml)._deploy-testnet.yml:network-indexremoved — the network is resolved from the deployed chain ID; the deploy job is namedDeploy Contracts(wasDeploy to Testnet).CI/CD / ci / Build & Test) — required status checks in consumer branch-protection rules need updating.header: coverage, so the first run on an existing PR posts a fresh comment._foundry-cicd.ymlis now gated on the whole CI workflow (including Slither/Halmos when enabled), not just Build & Test — stricter, intentional.Things to watch on first downstream run
./workflow calls andjob.workflow_shawhen caller and callee are the same repo; the cross-repo case (a consumer calling@main) can't be exercised from this repo's CI, so a smoke test on one downstream repo after merge is recommended.empty-environment-guardjob in e2e pins the assumption thatenvironment: ''means "no environment"; if GitHub changes that, e2e goes red here rather than in consumers.