Skip to content

ci: add spec-tests sync workflows#610

Open
vaclav-ssvlabs wants to merge 13 commits into
mainfrom
spec-test-ci
Open

ci: add spec-tests sync workflows#610
vaclav-ssvlabs wants to merge 13 commits into
mainfrom
spec-test-ci

Conversation

@vaclav-ssvlabs
Copy link
Copy Markdown

Summary

Adds GitHub Actions workflows to automatically sync generated spec test JSON files to the dedicated spec-tests repository.

  • generate-spec-tests composite action: sets up Go, runs generators for ssv/qbft/types spectests, copies output to ../spec-tests
  • sync-spec-tests-pr: on PR open/synchronize, pushes generated files to a matching branch in spec-tests repo and creates/updates a mirrored PR
  • sync-spec-tests-merge: on push to main, regenerates files and merges the corresponding spec-tests PR
  • test.yaml: bumped to ubuntu-24.04, node24-compatible action versions

Notes

  • This PR should be merged before the JSON file removal PR

GalRogozinski and others added 6 commits February 2, 2026 18:33
Adds GitHub Actions workflows to automatically sync generated spec test
JSON files to a dedicated spec-tests repository on PR open and merge.

- generate-spec-tests composite action: sets up Go, runs generators for
  ssv/qbft/types spectests, copies output to ../spec-tests
- sync-spec-tests-pr: on PR open/sync, pushes generated files to a
  matching branch in spec-tests repo and creates/updates a PR
- sync-spec-tests-merge: on push to main, regenerates files and merges
  the corresponding spec-tests PR
- test.yaml: bumped to ubuntu-24.04, node24-compatible action versions

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 23, 2026

Greptile Summary

This PR introduces GitHub Actions workflows to automatically sync generated spec-test JSON files from ssv-spec to a dedicated spec-tests repository, plus a composite action to consolidate Go setup and generation steps.

  • generate-spec-tests composite action: Consolidates Go setup and go generate steps previously scattered in test.yaml — clean refactor.
  • sync-spec-tests-pr: On PR open/synchronize, generates spec tests and pushes them to a matching branch in the spec-tests repo, creating or updating a mirrored PR. Has two P2 issues: force-pushing to the mirrored PR branch (violates the project's no-force-push-to-PR-branches policy) and using a bare #PR_NUMBER reference in the PR body that will auto-link to the wrong repo.
  • sync-spec-tests-merge: On push to main, regenerates files and merges the corresponding spec-tests PR. Force-push is also present here. The approach of resolving the original PR via commits/{sha}/pulls is sound; the timing caveat is correctly acknowledged.
  • test.yaml: Pinned to ubuntu-24.04 and node24-compatible action versions — straightforward improvement.
  • Both sync workflows validate SPEC_TESTS_REPO only after the expensive generation step; adding an early check would avoid wasted CI minutes when the variable is misconfigured.

Confidence Score: 4/5

  • Safe to merge after addressing the cross-repo PR link bug and acknowledging the force-push trade-off for auto-generated content.
  • The core automation logic is correct and well-structured. The two remaining items are a broken cross-repo reference in the PR body (easy one-line fix) and the force-push pattern which, while flagged by the custom rule, may be intentional for auto-generated content. Neither blocks the primary sync flow.
  • sync-spec-tests-pr.yaml — contains both the cross-repo reference issue and the force-push pattern.

Important Files Changed

Filename Overview
.github/actions/generate-spec-tests/action.yaml New composite action that sets up Go 1.22, runs go generate for ssv/qbft/types spectests, and copies output to ../spec-tests. Logic is sound; the find+copy pattern safely handles multiple spectest directories.
.github/workflows/sync-spec-tests-pr.yaml On PR open/synchronize, generates spec tests and syncs to a matching branch in spec-tests repo. Two issues: (1) force-push to PR branch overwrites review history per custom rule; (2) cross-repo #PR_NUMBER reference in the PR body will link to the wrong repo.
.github/workflows/sync-spec-tests-merge.yaml On push to main, finds the merged PR by commit SHA, pushes final generated files to the spec-tests branch, then merges the mirrored PR. Force-push is present here too. The API timing caveat is acknowledged in the error message.
.github/workflows/test.yaml Refactored to use the new composite action, pinned to ubuntu-24.04 and node24-compatible action versions. Clean and straightforward improvement.

Sequence Diagram

sequenceDiagram
    participant Dev as Developer
    participant SSV as ssv-spec (GitHub)
    participant GHA as GitHub Actions
    participant ST as spec-tests repo

    Dev->>SSV: Open / push to PR branch
    SSV->>GHA: Trigger sync-spec-tests-pr
    GHA->>GHA: checkout + go generate (ssv/qbft/types)
    GHA->>GHA: Get GitHub App token
    GHA->>ST: git clone spec-tests
    GHA->>ST: git checkout -B {branch} origin/{base}
    GHA->>ST: cp generated files + git commit
    GHA->>ST: git push --force origin {branch}
    GHA->>ST: gh pr create / gh pr edit

    Dev->>SSV: Merge PR to main
    SSV->>GHA: Trigger sync-spec-tests-merge
    GHA->>GHA: checkout + go generate (ssv/qbft/types)
    GHA->>SSV: gh api commits/{SHA}/pulls → PR number + head_ref
    GHA->>GHA: Get GitHub App token
    GHA->>ST: git clone spec-tests
    GHA->>ST: git checkout -B {branch} origin/{branch}
    GHA->>ST: cp final generated files + git commit
    GHA->>ST: git push --force origin {branch}
    GHA->>ST: gh pr merge {spec-tests PR} --merge --delete-branch
Loading

Reviews (1): Last reviewed commit: "ci: add spec-tests sync workflows" | Re-trigger Greptile

Comment thread .github/workflows/sync-spec-tests-pr.yaml
Comment thread .github/workflows/sync-spec-tests-pr.yaml
Comment thread .github/workflows/sync-spec-tests-pr.yaml
@GalRogozinski
Copy link
Copy Markdown
Contributor

GalRogozinski commented Apr 5, 2026

Thanks @vaclav-ssvlabs
greptile says:

using a bare #PR_NUMBER reference in the PR body that will auto-link to the wrong repo.

Codex:

  1. sync-spec-tests-pr.yaml can succeed without creating anything, but sync-spec-tests-merge.yaml later assumes the branch/PR exists and will fail.

At sync-spec-tests-pr.yaml:108, the PR sync workflow exits early on git diff --cached --quiet with "No changes to sync" and never pushes the branch or opens the PR. But the merge workflow later hard-requires both to exist at sync-spec-tests-merge.yaml:109 and sync-spec-tests-merge.yaml:131. That means an ssv-spec PR that produces no fixture diff against its base branch can still cause the post-merge main workflow to fail. Either the PR workflow needs to create the mirror branch/PR even for the no-op case, or the merge workflow needs to tolerate them being absent.

Probably non issue but still pasting:
2. Fork PRs will fail because this workflow uses a private-key secret on pull_request.

At sync-spec-tests-pr.yaml:34, the workflow unconditionally calls actions/create-github-app-token using SPEC_TESTS_APP_PRIVATE_KEY, but GitHub does not expose repository secrets to fork-originated pull_request runs. If external contributors are expected to open PRs, this job will fail immediately unless it explicitly skips forks or uses a different event model.

@GalRogozinski
Copy link
Copy Markdown
Contributor

GalRogozinski commented Apr 5, 2026

Hey @vaclav-ssvlabs after the known issues, there may be more.

I recommend the following:

  1. On a ci-stage branch we should merge this and Move jsons to spec-tests repo #602. This way we have the complete feature and we don't need the 3 reviews.
  2. For tests, We need to tweak the environment so that the automation will work w.o main being merged. After we are confident we can collect the 3 reviews and merge to main.

wdyt?

I also opened #619 to fix the no changes edge case. Codex claims the greptile bug is a non-issue, but we should verify.
After you review my PR DM me and let's see if we can properly test this.

Resolve no test has changed edge case
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.

2 participants