Review fixes for #610: spec-tests sync hardening, fork policy, docs

.github/actions/generate-spec-tests/action.yaml

@@ -5,7 +5,7 @@ runs:
   using: composite
   steps:
     - name: Set up Go
-      uses: actions/setup-go@v6.3.0
+      uses: actions/setup-go@4b73464bb391d4059bd26b0524d20df3927bd417 # v6.3.0
       with:
         go-version: 1.22.x
 

.github/dependabot.yml

@@ -0,0 +1,10 @@
+version: 2
+updates:
+  # "/" covers .github/workflows only. The composite action lives outside that
+  # directory, so it needs its own entry or its pins never get bumped.
+  - package-ecosystem: github-actions
+    directories:
+      - "/"
+      - "/.github/actions/generate-spec-tests"
+    schedule:
+      interval: weekly

.github/workflows/README.md

@@ -2,7 +2,7 @@
 
 ## Overview
 
-Every PR to this repo automatically keeps a matching branch and PR open in the [spec-tests](https://github.com/ssvlabs/spec-tests) repository, populated with freshly generated JSON test fixtures. When the ssv-spec PR is merged, the spec-tests PR is finalized and merged automatically.
+Every PR from a branch in this repo whose fixtures differ from `spec-tests` automatically keeps a matching branch and PR open in the [spec-tests](https://github.com/ssvlabs/spec-tests) repository, populated with freshly generated JSON test fixtures. When the ssv-spec PR is merged, the spec-tests PR is finalized and merged automatically. Fork PRs are excluded — see [Fork PRs](#fork-prs).
 
 ```
 ssv-spec PR opened/updated
@@ -32,9 +32,24 @@ ssv-spec PR merged
 | File | Trigger | Purpose |
 |---|---|---|
 | `test.yaml` | PR, push to `main` | Build, generate, run tests |
-| `sync-spec-tests-pr.yaml` | PR opened / updated / reopened | Sync generated files to spec-tests, create/update PR |
+| `sync-spec-tests-pr.yaml` | PR opened / updated / reopened, same-repo only | Sync generated files to spec-tests, create/update PR |
 | `sync-spec-tests-merge.yaml` | Push to `main` | Push final files to spec-tests branch, merge spec-tests PR |
-| `../.github/actions/generate-spec-tests/action.yaml` | (composite, called by all above) | Set up Go, generate JSON fixtures |
+| `../actions/generate-spec-tests/action.yaml` | (composite, called by all above) | Set up Go, generate JSON fixtures |
+
+---
+
+## Running Locally
+
+The JSON fixtures are no longer committed to this repo — they are generated into a **sibling** `spec-tests` directory, so generation must run before the tests:
+
+```console
+foo@bar:~/ssv-spec$ go generate ./...   # or: make generate-jsons
+foo@bar:~/ssv-spec$ make test
+```
+
+Fixtures resolve to `<parent-of-ssv-spec>/spec-tests/<module>` (`qbft`, `ssv`, `types`), created automatically by `go generate`. A fresh clone that runs `make test` first will fail — `make test` does not depend on `generate-jsons`.
+
+> **Multiple checkouts:** the path is derived from the repo root's parent and is not namespaced per checkout, so two clones or worktrees sharing a parent directory also share one `spec-tests` and overwrite each other's output. Give each checkout its own parent directory.
 
 ---
 
@@ -76,6 +91,16 @@ Runs on every push to `main`.
 
 ---
 
+### Fork PRs
+
+Fork PRs get no mirror branch or mirror PR while open, because `pull_request` from a fork receives no secrets. Their fixtures are still generated and tested by `test.yaml`, which needs no secrets, so correctness is verified either way.
+
+After merge, their fixtures reach spec-tests with the next in-repo merge **that has a mirror branch** — each sync replaces the whole generated tree, so it carries the fork's changes along. The gap: an in-repo PR whose own fixtures match spec-tests never pushes a mirror branch, and if such a PR merges next, its merge run finds no mirror branch, sees the fork's fixture diff, and fails loudly rather than pushing. The sync then completes on the following in-repo merge that does have one.
+
+Contributing from a branch in `ssvlabs/ssv-spec` is the recommended path: it gets the mirror PR preview while the PR is open.
+
+---
+
 ## Authentication
 
 | Operation | Token used |
@@ -109,4 +134,4 @@ Set these in **ssv-spec → Settings → Secrets and variables → Actions**.
 |---|---|
 | `SPEC_TESTS_APP_PRIVATE_KEY` | Contents of the `.pem` private key file for the GitHub App |
 
-See the [GitHub App setup guide](https://github.com/ssvlabs/ssv-spec/blob/main/.github/workflows/GITHUB_APP_SETUP.md) for step-by-step instructions on creating the app.
+To create the App: [Registering a GitHub App](https://docs.github.com/en/apps/creating-github-apps/registering-a-github-app/registering-a-github-app) with the two permissions above, installed on `spec-tests` only. Its App ID goes in `SPEC_TESTS_APP_ID` and its generated `.pem` private key in `SPEC_TESTS_APP_PRIVATE_KEY`.

.github/workflows/lint.yaml

@@ -11,18 +11,18 @@ jobs:
   build:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v4
+      - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4
 
       - name: Set up Go
-        uses: actions/setup-go@v5
+        uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff # v5
         with:
           go-version: 1.22.x
 
       - name: Get Dependencies
         run: go get -v -t -d ./...
 
       - name: Lint
-        uses: golangci/golangci-lint-action@v9
+        uses: golangci/golangci-lint-action@ba0d7d2ec06a0ea1cb5fa41b2e4a3ab91d21278a # v9
         with:
           version: v2.12.2
           args: -v ./...

.github/workflows/stale.yml

@@ -15,7 +15,7 @@ jobs:
   stale:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/stale@v9.1.0
+      - uses: actions/stale@5bef64f19d7facfb25b37b414482c7164d639639 # v9.1.0
         with:
           days-before-stale: 60
           days-before-close: -1

.github/workflows/sync-spec-tests-merge.yaml

@@ -14,7 +14,7 @@ jobs:
     env:
       SPEC_TESTS_REPO: ${{ vars.SPEC_TESTS_REPO }}
     steps:
-      - uses: actions/checkout@v6.0.2
+      - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
 
       - name: Validate required variables
         run: |
@@ -33,7 +33,7 @@ jobs:
         run: echo "name=${SPEC_TESTS_REPO##*/}" >> "$GITHUB_OUTPUT"
 
       - name: Get GitHub App token
-        uses: actions/create-github-app-token@v3.0.0
+        uses: actions/create-github-app-token@f8d387b68d61c58ab83c6c016672934102569859 # v3.0.0
         id: app-token
         with:
           app-id: ${{ vars.SPEC_TESTS_APP_ID }}
@@ -66,23 +66,63 @@ jobs:
           # Resolve the ssv-spec PR that introduced this merge commit (head branch name)
           # Uses GITHUB_TOKEN which is scoped to ssv-spec
           PR_INFO=$(gh api "repos/${{ github.repository }}/commits/${GITHUB_SHA}/pulls" \
-            --jq '.[0] | {number: (.number // empty), head_ref: (.head.ref // empty), base_ref: (.base.ref // empty)}' \
+            --jq '.[0] | {number: (.number // empty), head_ref: (.head.ref // empty), base_ref: (.base.ref // empty), head_repo: (.head.repo.full_name // "")}' \
             2>/dev/null || echo "{}")
           PR_NUMBER=$(echo "${PR_INFO}" | jq -r '.number // empty')
           PR_HEAD_REF=$(echo "${PR_INFO}" | jq -r '.head_ref // empty')
           PR_BASE_REF=$(echo "${PR_INFO}" | jq -r '.base_ref // empty')
+          PR_HEAD_REPO=$(echo "${PR_INFO}" | jq -r '.head_repo // empty')
 
           if [ -z "${PR_NUMBER}" ] || [ -z "${PR_HEAD_REF}" ]; then
             echo "No PR found for commit ${GITHUB_SHA} in ${{ github.repository }}." >&2
             echo "Every push to main must come from a merged PR. If this is a GitHub API timing issue, re-run the workflow." >&2
             exit 1
           fi
 
+          # Same-repo PRs always report head.repo; it is null only when the head
+          # repository is gone (deleted fork). Unknown therefore means "not this
+          # repo", and skipping the sync is the safe reading — there is no mirror
+          # branch to finalize and nothing to push.
+          IS_FORK=true
+          if [ "${PR_HEAD_REPO}" = "${{ github.repository }}" ]; then
+            IS_FORK=false
+          fi
+
+          # PR_HEAD_REF flows into `git checkout -B`, `git push --force` and
+          # `gh pr list --head` below. It is a bare branch name with no fork
+          # owner, so a fork PR opened from the fork's default branch yields
+          # `main` — and `origin/main` always exists, so the "mirror branch
+          # missing" check cannot catch it. Enforce the same shape and
+          # protected-name rules as sync-spec-tests-pr.yaml before it is used.
+          # Same-repo only: for fork PRs, PR_HEAD_REF never reaches
+          # `git checkout -B`, `git push` or `gh pr list --head`, because the
+          # push step below is skipped for them entirely.
+          if [ "${IS_FORK}" != "true" ]; then
+            if ! [[ "${PR_HEAD_REF}" =~ ^[A-Za-z0-9._/-]+$ ]] \
+              || [[ "${PR_HEAD_REF}" == -* ]] \
+              || [[ "${PR_HEAD_REF}" == *..* ]] \
+              || [[ "${PR_HEAD_REF}" == "HEAD" ]]; then
+              echo "Refusing to sync: unsafe branch name '${PR_HEAD_REF}'" >&2
+              exit 1
+            fi
+
+            if [ "${PR_HEAD_REF}" = "main" ] || [ "${PR_HEAD_REF}" = "master" ]; then
+              echo "Refusing to sync: branch name '${PR_HEAD_REF}' would overwrite a protected branch in ${SPEC_TESTS_REPO}" >&2
+              exit 1
+            fi
+          fi
+
           echo "number=${PR_NUMBER}" >> "${GITHUB_OUTPUT}"
           echo "head-ref=${PR_HEAD_REF}" >> "${GITHUB_OUTPUT}"
           echo "base-ref=${PR_BASE_REF}" >> "${GITHUB_OUTPUT}"
+          echo "is-fork=${IS_FORK}" >> "${GITHUB_OUTPUT}"
 
+      # Fork PRs never got a mirror branch (the PR sync workflow skips them),
+      # so there is nothing here to finalize. Their fixtures land with a later
+      # in-repo merge that does have a mirror branch — each sync below replaces
+      # the whole generated tree. See "Fork PRs" in README.md for the gap.
       - name: Push final generated files to spec-tests branch
+        if: steps.find-ssv-pr.outputs.is-fork == 'false'
         env:
           GH_TOKEN: ${{ steps.app-token.outputs.token }}
           PR_HEAD_REF: ${{ steps.find-ssv-pr.outputs.head-ref }}

.github/workflows/sync-spec-tests-pr.yaml

@@ -4,16 +4,26 @@ on:
   pull_request:
     types: [opened, synchronize, reopened]
 
+# Both runs force-push the same mirror branch with no ordering, so an older commit's
+# fixtures can land last and leave it stale. Keyed on PR number, not head_ref (collides across forks).
+concurrency:
+  group: sync-spec-tests-${{ github.event.pull_request.number }}
+  cancel-in-progress: true
+
 jobs:
 
   # Runs PR-controlled code (`go generate`). Deliberately holds no spec-tests
   # credentials: its only output is an artifact consumed as data by `sync`.
+  #
+  # Fork PRs get no secrets on `pull_request` anyway, so sync cannot work; they
+  # are synced after merge by sync-spec-tests-merge.yaml instead.
   generate:
+    if: github.event.pull_request.head.repo.full_name == github.repository
     runs-on: ubuntu-24.04
     permissions:
       contents: read
     steps:
-      - uses: actions/checkout@v6.0.2
+      - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
 
       - name: Validate required variables
         run: |
@@ -31,7 +41,7 @@ jobs:
         run: tar -czf spec-tests.tar.gz -C "${GITHUB_WORKSPACE}/.." spec-tests
 
       - name: Upload generated spec tests
-        uses: actions/upload-artifact@v7.0.1
+        uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
         with:
           name: spec-tests
           path: spec-tests.tar.gz
@@ -42,14 +52,15 @@ jobs:
   # checked out here, the generated tree is only ever treated as data.
   sync:
     needs: generate
+    if: github.event.pull_request.head.repo.full_name == github.repository
     runs-on: ubuntu-24.04
     permissions:
       contents: read
     env:
       SPEC_TESTS_REPO: ${{ vars.SPEC_TESTS_REPO }}
     steps:
       - name: Download generated spec tests
-        uses: actions/download-artifact@v8.0.1
+        uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1
         with:
           name: spec-tests
 
@@ -68,7 +79,7 @@ jobs:
         run: echo "name=${SPEC_TESTS_REPO##*/}" >> "$GITHUB_OUTPUT"
 
       - name: Get GitHub App token
-        uses: actions/create-github-app-token@v3.0.0
+        uses: actions/create-github-app-token@f8d387b68d61c58ab83c6c016672934102569859 # v3.0.0
         id: app-token
         with:
           app-id: ${{ vars.SPEC_TESTS_APP_ID }}

.github/workflows/test.yaml

@@ -13,7 +13,7 @@ jobs:
     permissions:
       contents: read
     steps:
-      - uses: actions/checkout@v6.0.2
+      - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
 
       - name: Generate spec tests
         uses: ./.github/actions/generate-spec-tests

Makefile

@@ -1,9 +1,14 @@
 GOPATH?=$(shell go env GOPATH)
 TEST_PKG?=./...
+# Keep in sync with the version pinned in .github/workflows/lint.yaml
+GOLANGCI_LINT_VERSION?=v2.12.2
 
 .PHONY: lint-prepare
 lint-prepare:
-	curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh| sh -s latest
+	@tmp=$$(mktemp) \
+		&& curl -sfL https://golangci-lint.run/install.sh -o "$$tmp" \
+		&& sh "$$tmp" $(GOLANGCI_LINT_VERSION) \
+		&& rm -f "$$tmp"
 
 .PHONY: lint
 lint:

README.md

@@ -184,3 +184,9 @@ To generate all json spec tests, run:
 foo@bar:~$ go generate ./...
 ```
 Then run all tests
+
+```console
+foo@bar:~$ make test
+```
+
+The generated JSONs are not committed here — they are written to a sibling `spec-tests` directory (`<parent-of-ssv-spec>/spec-tests/<module>`), so `go generate ./...` must run before `make test`. See [.github/workflows/README.md](.github/workflows/README.md#running-locally) for the full local setup, including the caveat about multiple checkouts sharing one `spec-tests` directory.

qbft/spectest/run_test.go

@@ -31,7 +31,7 @@ func TestJson(t *testing.T) {
 	basedir, _ := os.Getwd()
 	specTestsDir, err := typescomparable.SpecTestsDirFrom(basedir)
 	if err != nil {
-		panic(err.Error())
+		t.Fatalf("Failed to resolve spec-tests dir: %v", err)
 	}
 	path := filepath.Join(specTestsDir, "tests.json")
 	untypedTests := map[string]interface{}{}

types/spectest/run_test.go

@@ -42,7 +42,7 @@ func TestJson(t *testing.T) {
 	basedir, _ := os.Getwd()
 	specTestsDir, err := comparable.SpecTestsDirFrom(basedir)
 	if err != nil {
-		panic(err.Error())
+		t.Fatalf("Failed to resolve spec-tests dir: %v", err)
 	}
 	path := filepath.Join(specTestsDir, "tests.json")
 	untypedTests := map[string]interface{}{}