Skip to content

docs: generated-artifacts draft-PR CI policy in AGENTS.md - #2445

Open
kelemeno wants to merge 3 commits into
release/v0.33.0-atomic-interopfrom
kl/agents-md-draft-ci-policy
Open

kelemeno wants to merge 3 commits into
release/v0.33.0-atomic-interopfrom
kl/agents-md-draft-ci-policy

Conversation

@kelemeno

Copy link
Copy Markdown
Contributor

What

Adds a Generated Artifacts and Draft-PR CI Policy section to AGENTS.md, so coding agents stop wasting time on the regen-dependent checks during development:

  • Open PRs as drafts while iterating.
  • On a draft PR, red CI on check-hashes / check-selectors / check-zkstack-out / state-generation-check is expected and must NOT be investigated, fixed, or regenerated per commit.
  • Stale chain-state snapshots are named as the first suspect when interop tests fail right after a bytecode change (they load snapshot bytecode, so they fail — or silently test old code — until regenerated; ANVIL_INTEROP_FRESH_DEPLOY=1 tests the actual code locally). This is the failure mode that previously caused phantom debugging (e.g. around chore(verifier): regenerate ZKsyncOSVerifierPlonk for the zksync-os v0.5.3-private VK #2433).
  • When done: one Update All Generated Artifacts dispatch with the PR number, then mark ready for review. Never regenerate hashes/chain states locally (local zksolc diverges from CI).

The existing "zkstack-out out of date" / "selectors out of date" Common Issues entries now point at this policy and at the one-click dispatch.

Why

Docs-only, mergeable now: it lets us trial the "don't chase regen checks on drafts" behavior for a few days while #2437 (which additionally makes those checks skip on draft PRs) waits for review. The policy text is written to stay correct after #2437 merges.

🤖 Generated with Claude Code

Agents must not regenerate hashes/selectors/zkstack-out/chain-states per
commit or debug their red CI on draft PRs; one Update All Generated
Artifacts dispatch right before marking ready for review clears them all.
Also flags stale chain-state snapshots as the first suspect when interop
tests fail after a bytecode change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Auto-generated by CI with foundry v1.5.1 and FOUNDRY_PROFILE=anvil-interop.
kelemeno added a commit that referenced this pull request Sep 9, 2026
Dispatching the regen against #2445 (a docs-only PR) verified the local-action
fix from the previous commit, and surfaced this: the writer decided 'did
anything change?' with git diff, while the verifier gunzips and tolerates
block-number drift. Since the snapshots are gzipped dumps of a wall-clock
chain, every regeneration differs byte-for-byte, so the dispatch always pushed
a commit — it pushed six +0/-0 .json.gz files onto that PR. The writer now
asks compare-chain-states, the same comparator state-generation-check uses.

Also drops the temporary marker file used to exercise the draft-skip path.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kelemeno added a commit that referenced this pull request Sep 9, 2026
## What

Splits PR CI into two tiers so that checks requiring an artifact
regeneration only have to pass once, right before merging, instead of on
every push. The tiers are tied to the PR's **draft status**: on a draft
PR only per-commit CI runs; once the PR is marked ready for review, all
CI runs.

### Per-commit CI (runs on every push, drafts included)

- `l1-contracts-ci`: **removed** `check-hashes`, `check-selectors`,
`check-zkstack-out` (moved to pre-merge). Build, foundry tests,
zkfoundry tests, verifier-gen check, and coverage stay.
- `anvil-interop-ci`: the interop suite keeps running from the committed
chain-state snapshots (fast, parallel). If a change alters genesis state
it may run against stale bytecode (or fail) until a regen — accepted
trade-off; the pre-merge `state-generation-check` gates snapshot
freshness before merge. The v31→v33 upgrade test stays (it uses
historical v31 states, no regen needed).
- All other workflows (lint, slither, l2/system contracts, foundry-ci,
licenses, secrets) are untouched.

### Pre-merge CI (new `pre-merge-checks.yaml`, skips on draft PRs)

All checks that compare committed generated files against a fresh
regeneration:

- `check-hashes` (`AllContractsHashes.json`)
- `check-selectors` (`l1-contracts/selectors`)
- `check-zkstack-out` (`l1-contracts/zkstack-out`, reuses the build
artifacts for an incremental rebuild)
- `state-generation-check` (chain-state determinism gate)

They run when the PR is **not a draft** — on the `ready_for_review`
transition and every later push — and on `workflow_dispatch`.
`merge_group` is also wired up, but it is forward-looking only: the repo
has no merge queue today, and enabling one should not require a workflow
edit.

A `changes` job decides first whether the PR can affect the artifacts at
all, so a documentation-only PR does not pay a 9-minute state
regeneration. It is exclusion-based on purpose: an unrecognized path
counts as relevant, so a new directory runs the checks rather than
silently bypassing them. Only paths that cannot reach a compiler or a
generator belong in that list, which is why `protocol-ops/` came back
out of it after review: chain-state generation reaches only forge
scripts today, but code that *writes* a committed artifact qualifies
whatever language it is in — #2457's genesis check is exactly that
shape, and nothing structural keeps protocol-ops out of state setup
tomorrow.

### `pre-merge-verified` — the check to require

**This is the one context to add to the base branch's required checks,
and it needs an org-ruleset edit to take effect.** Today nothing
enforces the tier: ruleset 18020683 (org-sourced, `Hardened - Protected
Branches`) has no `required_status_checks` rule, classic protection is
off, and every check on this PR reports `isRequired: null` — so a PR can
be merged the moment it is marked ready, while the checks are still
queued.

Requiring the four validators individually would **not** fix that, which
is the trap: GitHub [counts a skipped required check as
satisfied](https://docs.github.com/en/pull-requests/how-tos/merge-and-close-pull-requests/troubleshooting-required-status-checks),
and the validators are skipped whenever their producer fails. That
combination already happened on this PR — [pre-merge attempt
1](https://github.com/matter-labs/era-contracts/actions/runs/33162317525/attempts/1)
failed downloading zksolc, skipped all three validators, while [L1
CI](https://github.com/matter-labs/era-contracts/actions/runs/33162317466)
passed.

So `pre-merge-verified` always runs (`if: always()`), inspects every
job's result, and fails on any failure, cancellation, or *unexpected*
skip. It passes only when the checks really passed, or when they were
skipped for a reason it can name: the PR is a draft, or nothing
artifact-affecting changed.

Note for whoever configures this: the build job is deliberately named
`build-artifacts` here, because both callers of the reusable build
otherwise report a check literally named `build / build`
(`l1-contracts-ci`'s and this workflow's), which is ambiguous as a
required context.

### One-click regen

**Update All Generated Artifacts** performs the complete pre-merge regen
in one dispatch: hashes + selectors + zkstack-out (via
`update-hashes-on-demand`), then chain states.

`regenerate-chain-states` needed two fixes to keep that dispatch
trustworthy.

First, `uses: ./…` resolves from the workspace, which the workflow
rewinds to the *target PR's* head — a branch that predates the composite
action this revision calls, so the chain-state stage could fail to find
it after the hashes stage had already committed. The dispatched
revision's actions are now stashed across that checkout and used to fill
in only what the target branch lacks — the PR's own copy wins wherever
it exists, so a PR that updates the build recipe regenerates with the
recipe its own `state-generation-check` will verify against. Verified on
exactly that shape: [dispatched from this branch against
#2445](https://github.com/matter-labs/era-contracts/actions/runs/34333262063),
whose head does not contain the action — stash, checkout, restore and
build all succeed.

That run also showed the dispatch pushing a chain-states commit onto
that docs-only PR — six `+0/-0` `.json.gz` files (since reverted). The
cause is benign: the writer commits on any byte difference, and these
gzipped dumps of a wall-clock-dependent chain always differ. Suppressing
it with `compare-chain-states` was tried and **reverted**, because that
comparator skips the *entire* storage of every chain diamond, so a
change confined to diamond storage compares as "up to date" — a writer
trusting that verdict would leave the committed snapshots stale while
reporting success. A redundant commit per dispatch is the cheaper
mistake.

Worth flagging for the same reason, since this PR makes
`state-generation-check` the merge gate: that exclusion is a **known
blind spot of the gate itself**, and it predates this PR. Narrowing it
to the genuinely volatile slots is follow-up work that would both let
the writer be smarter and make the gate strictly stronger. What this PR
does fix in the comparator is the version-directory scan: it iterated
only the committed tree, so a protocol bump adding a generated-only
directory compared as up to date; it now takes the union
(fixture-checked — identical trees pass, generated-only and
committed-only versions both fail).

### Shared build (dedup)

The all-projects foundry build + artifacts cache has a single
definition: `build-contract-artifacts.yaml` (`workflow_call`), used by
both `l1-contracts-ci` and `pre-merge-checks`, carrying the ci-speedups
build steps from #2441 (solc-cache warming, prune unit test). It now
declares `permissions: contents: read` instead of inheriting
`l1-contracts-ci`'s `pull-requests: write`.

Cache coordination is race-free by construction rather than by waiting.
The build takes the key it saves under — run-scoped for pre-merge, so no
other run ever writes it — and *outputs* the key consumers must restore:
the reused key when the reuse hit (already finalized, since the build
just downloaded it), otherwise its own. `reuse-key` is the cheap path:
when `pre-merge-checks` runs on a commit `l1-contracts-ci` has already
finished building, the build collapses to a cache download ([63 seconds,
measured](https://github.com/matter-labs/era-contracts/actions/runs/33162317525/job/98821679690))
— see the caveat under "The gating, demonstrated" for when it does not.

Similarly, the deterministic chain-state build (plain foundry,
`anvil-interop` profile, no warm cache) has a single definition in the
`build-anvil-interop-artifacts` composite action, used by both
`regenerate-chain-states` (the snapshot writer) and
`state-generation-check` (the verifier) so the two cannot drift apart.

## Why a separate workflow, rather than `if: !draft` on the four jobs in
place?

Gating the four jobs where they were — and adding `ready_for_review` to
`l1-contracts-ci` and `anvil-interop-ci` — is simpler, and it keeps
`state-generation-check` behind the anvil path filter. It was rejected
for one reason: `ready_for_review` re-runs those workflows *whole*. The
per-commit tier's ~20 minutes of builds, foundry tests, zkfoundry tests
and coverage would run a second time on a commit where they already
passed, and the PR's check rollup would be replaced by that second run.
Suppressing the tier-1 jobs on that event (`if: github.event.action !=
'ready_for_review'`) trades the waste for something worse:
`test-foundry` and friends re-reporting as *skipped*, which — per the
same GitHub behaviour described above — reads as satisfied.

A separate workflow re-runs only the four checks that need re-running,
and leaves tier-1's verdicts standing. The cost is the shared build
definition and the cache handshake above, which is why both are kept
small and why the build is skipped outright on a reuse hit.

## The gating, demonstrated

Reviewers noted the draft and `ready_for_review` paths had never
actually run. They have now, on this branch:

| Case | Run | Result |
| --- | --- | --- |
| Push while draft |
[34333147132](https://github.com/matter-labs/era-contracts/actions/runs/34333147132)
| `changes` green (the pushed path was *not* excluded, so the tier
wanted to run), `build-artifacts` and all four validators skipped,
`pre-merge-verified` green — skips recognised as deliberate |
| `ready_for_review` |
[34333263535](https://github.com/matter-labs/era-contracts/actions/runs/34333263535)
| full tier green on real results |
| Push while non-draft |
[34334332690](https://github.com/matter-labs/era-contracts/actions/runs/34334332690)
| tier runs again |

One honest caveat on the reuse path: in the `ready_for_review` run above
the build did *not* hit the cache, because the ready transition came ~2
minutes after the push while `l1-contracts-ci` was still building the
same commit — both missed and both built, which is the concurrent case
the run-scoped save key makes safe ([consumers restored
`artifacts-l1-pre-merge-<sha>-<run_id>`](https://github.com/matter-labs/era-contracts/actions/runs/34251002816)).
The reuse hit is demonstrated separately, by a rerun against an
already-built commit: [a 63-second
build](https://github.com/matter-labs/era-contracts/actions/runs/33162317525/job/98821679690)
that skipped the compile entirely. In practice the ready transition
usually lands after tier-1 has finished, which is when reuse pays.

## Merge flow

1. Open the PR as a **draft** and iterate; per-commit CI must be green.
2. When ready: dispatch **Update All Generated Artifacts** with the PR
number.
3. Mark the PR **ready for review**; merge once `pre-merge-verified` is
green. If review forces bytecode changes, regen again (or convert back
to draft until done).

Documented in `docs/ai-review/docs/ci-green.md` under "CI tiers" — the
real CI doc, whose job lists and v31→v32 references were stale.
`.github/workflows/README.md` (which is about calldata generation) keeps
a pointer instead of the section. #2445 writes the same expectations
into `AGENTS.md` for agents.

## Coordination with #2457

#2457 adds `check-zksync-os-genesis` to `l1-contracts-ci` with `needs:
[build]` and the restore action. It is a committed-generated-artifact
check by the same definition as the rest (it regenerates
`configs/genesis/zksync-os/latest.json` and fails on the diff), so under
this policy it belongs in the pre-merge tier, and **Update All Generated
Artifacts** needs a leg that regenerates it — otherwise the one-click
regen leaves it red. The two PRs also conflict in `l1-contracts-ci.yaml`
(this PR deletes the block #2457 inserts into). **Whichever lands second
adapts**: if #2457 lands first, this PR moves the job into
`pre-merge-checks.yaml` and adds the regen leg; if this lands first,
#2457 targets the new workflow.

> Note for the squash message: the first commit in this branch describes
the gate as a `pre-merge` label, which was replaced by draft-status
gating later in the branch. Prefer this description over the
concatenated commit messages.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
0xValera added a commit that referenced this pull request Sep 21, 2026
## What ❔

The regeneration workflows commit through the GitHub API instead of `git
commit && git push` as `zksync[bot]`, so their commits are signed.

- New composite action `.github/actions/signed-commit`: stages the given
paths and creates the commit with the GraphQL `createCommitOnBranch`
mutation, which GitHub signs. It refuses to run on a branch other than
the requested one, and `expectedHeadOid` fails the call instead of
overwriting a newer push.
- `update-hashes-on-demand.yaml` and `regenerate-chain-states.yaml` mint
a `matterlabs-bots` App token (`contents: write` on this repository,
revoked at job end) once the PR's own build has run, then call the
action. The action is taken from the dispatched ref into `.ci-actions/`,
so PRs branched before it existed can still be regenerated.
- `RELEASE_TOKEN` is no longer used, checkouts run with
`persist-credentials: false`, and `contents: write` is dropped where
nothing pushes any more.

Commits are **Verified** and attributed to `matterlabs-bots[bot]`;
unlike pushes with `GITHUB_TOKEN`, they trigger the PR's workflows.

## Why ❔

The org ruleset requires signed commits on `main`, `release/**` and
`draft/**` and checks every commit on the PR head, so each regeneration
left an unsigned bot commit to re-sign and force-push before merging
(#2461, #2432, #2479, #2445).
[EVM-1688](https://linear.app/matterlabs/issue/EVM-1688/bot-signed-commits-for-ci-jobs).

Signing through the API keeps key material out of CI; the App and its
org secrets already exist and are installed here, and `helm-charts` uses
the same path. The other bases carrying these workflows
(`draft/v0.34.0`, `draft-v31`, `atomic-imt-reduced`,
`release/v0.33.0-atomic-interop`) need the change adapted to their
copies.

## Validation

- `update-hashes-on-demand.yaml` dispatched from this branch against a
`main`-based scratch PR with one corrupted hash and no action in its
tree
([run](https://github.com/matter-labs/era-contracts/actions/runs/34475450705),
#2495): the restoring commit is authored by `matterlabs-bots[bot]` with
`verification.reason == valid`, and the PR's workflows re-ran on it.
- `regenerate-chain-states.yaml` is not exercised, since `main` has no
anvil-interop tree. Its commit step is identical; the ~3.5 MB payload
gets covered on `draft/v0.34.0`.

## Checklist

- [x] PR title corresponds to the body of PR (we generate changelog
entries from PRs).
- [x] Validated through the CI run linked above; workflow-only change,
no test suite applies.
- [x] Documentation comments have been added / updated.

---------

Co-authored-by: Anton Baliasnikov <aba@matterlabs.dev>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
@0xValera 0xValera added the keep-open Exempt from the stale-PR bot label Oct 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

keep-open Exempt from the stale-PR bot

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants