Skip to content

Clear signing AI CI pipeline - #3057

Draft
marcocastignoli wants to merge 5 commits into
masterfrom
ci/ai-review
Draft

Clear signing AI CI pipeline#3057
marcocastignoli wants to merge 5 commits into
masterfrom
ci/ai-review

Conversation

@marcocastignoli

@marcocastignoli marcocastignoli commented Sep 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

A language model reviews the descriptors of a pull request and posts what it finds as a comment. It runs after the deterministic checks pass, only when a maintainer approves it, and it never blocks a merge. What it does and how is on the docs page of #3071; the model comparison and the prompt are in #3069; the design note is "clear-signing-ci-ai-pipeline" on notes.argot.org.

This is the integration pull request. It stays a draft while the steps below land on its branch ci/ai-review, each as its own pull request onto that branch, so reviewers see one step at a time. Nothing reaches master until this pull request merges.

Until then every step is tested on Marco's fork, marcocastignoli/clear-signing-erc7730-registry: its default branch is ci/ai-review, it has the Environment ai-review with Marco as reviewer, and a test pull request there runs the whole thing, approval included.

One rule holds in every job: nothing from the pull request is executed and nothing from it reaches a shell line. The jobs read pull request data through the GitHub API, and the only checkouts are the test-reports branch and the base branch.

Plan

Step 0: setup

  • Branch ci/ai-review and this pull request.
  • The fork, to test the automatic trigger before the merge.
  • A repository admin creates the Environment ai-review with required reviewers on this repository. This must exist before the merge: without it GitHub creates the Environment unprotected and the review runs without a click.

Step 1: the gate (in this pull request)

Step 2: collect the inputs (#3061)

  • For every affected descriptor: the descriptor before and after, its tests and results, and from Sourcify the verified sources, the ABI, the NatSpec, the proxy resolution and the constructor arguments.
  • One review unit per descriptor and distinct implementation, keyed by the hash of the ABI and the sources.
  • Sources focused on the reviewed functions, 400 KB cap. This cut the tokens by two thirds.
  • The pull request description and discussion in every unit (AI review: hello world end to end, two models, one comment each #3070, from the review).
  • Tested on the fork: run, artifact ai-review-inputs.
  • Reviewed and merged into ci/ai-review.
  • Later: the source of the contracts that embedded calldata is forwarded to (needs the raw transactions of the tests).

Step 3: the benchmark (#3069)

Step 4: hello world end to end (#3070)

  • The review job sends every unit to two models, one request each: Claude Sonnet 5.5 low and GPT-6 Luna xhigh. The prompt and the spec come from the base branch.
  • The model answers in Markdown, critical findings first; the prompt gains the check other.
  • A post job with pull-requests: write and no key posts one comment per model, headed as AI-generated and advisory, with its cost.
  • End to end on the fork with both keys in its Environment: run on fork PR #2, waiting check, approval, two comments, updated in place on the re-run. Claude Sonnet 5.5 low: 10 s, about $0.13; GPT-6 Luna xhigh: 4 min, about $0.014. The Anthropic key must be created inside a workspace, or ANTHROPIC_WORKSPACE_ID set.
  • Reviewed and merged into ci/ai-review.

Step 5: make the review better

  • Compare the two models on real pull requests for a while, then keep one.
  • Calibrate severity in the prompt: Luna marks too many findings critical.
  • A deterministic check of EIP-712 type hashes before the model runs.
  • Every prompt change comes with a benchmark run and its numbers.

Step 6: production

  • The Environment, its reviewers and the keys on this repository; the caps.
  • The docs page docs/ai-review/README.md (docs: describe the AI review pipeline #3071), kept current with every change.
  • A pointer to it in the README for contributors; cost tracking from the token usage in the artifacts.
  • Merge this pull request. Later: publish the review next to the test report for the viewer.

🤖 Generated with Claude Code

@marcocastignoli @claude
ci: gate the AI review on green checks and the test report bundle
e53451a 
First step of the AI review of descriptor pull requests. The new AI Review
workflow runs on workflow_run of Descriptor Tests, from the default branch,
with a read-only token and no secrets. Its gate job decides whether a review
can happen: Registry Checks and Descriptor Tests concluded with success for
the head commit (Registry Checks is waited for when still running, and any
other failed pull_request workflow stops the review too), the pull request
changes only files under registry/ and ercs/, and the test report bundle of
the commit exists on the test-reports branch. The bundle is uploaded as an
artifact for the next job. A gate that does not pass is a skip with the
reason in the job summary, never a failed check.

workflow_dispatch with a pull request number runs the same job on demand,
which tests a branch of the workflow before it reaches master.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions github-actions Bot added the ci Changes to continuous integration label Sep 28, 2026
@marcocastignoli marcocastignoli self-assigned this Sep 28, 2026
@marcocastignoli @claude
ci: start the AI review on pull_request_target so it shows among the …
5893370 
…checks

A run started by workflow_run is recorded against the default branch, so
the first version of the AI Review workflow never appeared among the checks
of the pull request it reviewed. pull_request_target, the event the queued
note and the labels use, starts with the pull request and shows there, and
it also runs the workflow file of the base branch with secrets, which the
review needs. The safety rule is the same as before: nothing from the pull
request is executed and nothing from it reaches a shell line.

The gate now starts alongside Registry Checks and Descriptor Tests, so it
waits for both, with a grace period for the runs to be listed, then for the
bundle as before. The pull request number comes from the event, the head
commit from the API. The workflow runs only when a descriptor or a shared
file changed, the same filter as Descriptor Tests.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
manuelwedler
manuelwedler reviewed Sep 28, 2026
View reviewed changes
Comment thread .github/workflows/ai-review.yml
Comment thread .github/workflows/ai-review.yml Outdated
Comment thread .github/workflows/ai-review.yml
Comment thread .github/scripts/ai-review-gate.js Outdated
@marcocastignoli @claude
ci: ask for the maintainer's approval first, then check GitHub once
4250435 
Manuel's proposal. The review job is bound to the Environment ai-review,
so every descriptor pull request shows the AI Review check as waiting until
a maintainer approves, and nothing runs before the click. After the click
the job reads GitHub once instead of polling: the changed files through
tj-actions/changed-files (any file outside registry/ and ercs/ stops it),
the latest pull_request runs of the head commit through the API (Registry
Checks and Descriptor Tests must be green, any other failed workflow stops
it too), and the test report bundle of the commit through a sparse checkout
of the test-reports branch. Each condition that does not hold fails the job
with its reason in the summary; a maintainer re-runs it later, which asks
for approval again. The bundle lands one to three minutes after the tests,
and the message for that case says to wait and re-run.

This removes the gate script, the wait loop and the workflow_dispatch
trigger. The security rule is unchanged: nothing from the pull request is
executed, nothing from it reaches a shell line, and the only checkout is
the test-reports branch.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@marcocastignoli

marcocastignoli commented Sep 28, 2026

Copy link
Copy Markdown
Member Author

@manuelwedler implemented what we agreed: pull_request_target, approval first through the Environment, then one read of GitHub (changed files, run conclusions, bundle), no script. The fork runs are linked in the description.

Posted with Claude Code

@manuelwedler

manuelwedler commented Sep 28, 2026

Copy link
Copy Markdown
Collaborator

The new approach in 4250435 looks good: approval first, then one read of GitHub, no polling, no gate script, and the check shows on the PR. The two approved runs on the fork go through all steps.

Two small points:

  1. tj-actions/changed-files is a third-party dependency for one API call. gh api /pulls/N/files --paginate --jq gives the same list, including previous_filename for renames, and the file names then stay in jq. Now $FILES is unquoted in the loop of the stop step, which is the one place where PR data touches a shell line.
  2. The Environment ai-review must exist with its required reviewers before this PR merges. If it does not exist, GitHub creates it on the first run without protection rules, and the job runs without a click. Harmless today, but the plan says "needed from step 4"; better before the merge.

@marcocastignoli @claude
ci: list the changed files with gh api, and keep their names inside jq
3a9377a 
Manuel's review of the approval-first version. tj-actions/changed-files was a
third-party action for one API call that gh already makes, and the stop step
expanded the file names unquoted in a shell loop, the one place where pull
request data touched a shell line. Now one gh api call lists the files with
previous_filename for renames, jq filters the ones outside registry/ and
ercs/, and jq writes the summary, with the names restricted to safe
characters. The error annotation carries only the count.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@marcocastignoli

marcocastignoli commented Sep 28, 2026

Copy link
Copy Markdown
Member Author

Both points taken. 3a9377a replaces tj-actions/changed-files with one gh api /pulls/N/files --paginate call; the names go from the API into jq, which filters and writes the summary, so no pull request data is expanded on a shell line anymore, and the error annotation carries only a count. On the Environment: agreed, it is the admin item of step 0 and a condition for the merge; the plan says so, and the fork has it with a required reviewer.

Posted with Claude Code

This was referenced Sep 28, 2026
Open
Open
Draft
Merged
@marcocastignoli @claude
docs: describe the AI review pipeline (#3071)
81437e3 
* docs: describe the AI review pipeline

One page for readers in a hurry: the gate, the information retrieval and the
review, each in a few lines with the details in expandable sections; the
fourteen checks in one table; how deployments are grouped into review units;
what the model must not report; how the comment is kept safe; what a unit
costs; how to change the prompt. The prompt itself sits next to it, linked
from the page, until it finds its final place.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* docs: shorten the AI review page after review

Drop the status line, the reasons behind the workflow shape, the note on
unverified deployments, the case for Markdown, the comment safety
paragraph, the prompt change procedure and the file list; every note is a
question for the reviewer. The answer's "What was reviewed" section becomes
"What could not be reviewed", present only when something limited the
review; the prompt copy follows.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@manuelwedler manuelwedler Awaiting requested review from manuelwedler

At least 1 approving review is required to merge this pull request.

Assignees

@marcocastignoli marcocastignoli

Labels

ci Changes to continuous integration

Projects

Clear Signing PM
Status: Triage

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@marcocastignoli @manuelwedler