Review fixes for #610: spec-tests sync hardening, fork policy, docs - #635
Merged
Conversation
Add a Running Locally section to the CI workflow docs covering the <parent-of-ssv-spec>/spec-tests/<module> layout, the generate-before-test order, and the shared-directory caveat for multiple checkouts. Point to it from the root README.
make lint-prepare installed 'latest' while lint.yaml pins v2.12.2, so local and CI could run different linter versions. Pin the Makefile to the same version via GOLANGCI_LINT_VERSION.
sync-spec-tests-merge.yaml used the merged PR's head ref unvalidated in git checkout -B, git push --force and gh pr list --head. head.ref is a bare branch name, so a fork PR opened from the fork's default branch yields 'main'; origin/main always exists, so the missing-mirror-branch check at line 130 cannot catch it and the run force-pushes generated fixtures onto spec-tests main outside the mirror PR. Apply the same shape check and main/master refusal that sync-spec-tests-pr.yaml already performs, at the point the ref is resolved.
The install.sh served from the golangci-lint master branch fails checksum verification for v2 releases, so 'make lint-prepare' pinned to v2.12.2 installed nothing: hash_sha256_verify checksum for golangci-lint-2.12.2-darwin-arm64.tar.gz did not verify https://golangci-lint.run/install.sh installs the same pinned version successfully. Same change as #629, kept together with the version pin so local and CI stay on one version.
Two pushes in quick succession force-push the same mirror branch with no ordering, so an older commit's fixtures can land last and leave the mirror stale until the next push.
Fork PRs get no secrets on pull_request, so the pre-merge sync workflow never worked for them and the merge workflow failed loudly instead. Post-merge, fork code is already trusted: a human reviewed and merged it, and the merge workflow already runs go generate from main in the same job that holds the App token. Pre-merge mirroring for forks would let any GitHub user trigger App-token writes to spec-tests without review, which is the risk this change avoids.
The main/master and unsafe-shape checks on PR_HEAD_REF ran before is-fork was known, so a fork PR opened from its own default branch (head ref "main") tripped the protected-name refusal and never reached the fork sync path. Move the is-fork computation earlier and skip that validation for fork PRs, since their head ref is never used as a push/checkout target on the fork path.
…path Each sync in this step replaces the entire generated tree from main, so the next same-repo merge already carries a fork's fixtures into spec-tests. A dedicated fork push path only bought a shorter staleness window at the cost of a second, unaudited write path into spec-tests — skip the step for fork-originated merges instead.
Codex review of the fork policy found two defects and one wrong claim: - jq's 'empty' suppresses the whole object, so head_repo: (.head.repo.full_name // empty) collapsed PR_INFO when the head repository is gone (deleted fork), making the run exit 'No PR found' and never reaching the fork check. Use "" instead. - Unknown head repo now means 'not this repo': skip the sync. There is no mirror branch to finalize in that case, and treating it as same-repo risked pushing to an unrelated mirror branch. - The push step gate is 'is-fork == false' rather than '!= true', so a missing output fails closed. README: the 'next in-repo merge' guarantee was too strong. An in-repo PR with no fixture diff never pushes a mirror branch, so if it merges next its merge run fails loudly instead of syncing; the sync lands on the following in-repo merge that has a mirror branch.
…leanly - README Files table: ../actions/... not ../.github/actions/..., which resolved to .github/.github/actions. - Drop the link to a GITHUB_APP_SETUP.md that does not exist in this repo; point at GitHub's own App registration docs and name the two secrets. - qbft and types spectest TestJson: t.Fatalf instead of panic on a spec-tests dir resolution failure, matching ssv/spectest/run_test.go.
Contributor
Greptile SummaryThe PR hardens spec-test synchronization by serializing PR runs, validating mirror refs, and explicitly excluding fork PRs from credential-dependent synchronization. It also aligns local lint tooling with CI, improves local-generation documentation, and reports fixture-path failures through the Go test framework.
Confidence Score: 5/5The PR appears safe to merge, with the delayed synchronization of fork-originated fixtures explicitly documented as an intentional policy tradeoff. The changed workflow guards consistently prevent fork refs from reaching mirror-branch operations, same-repository refs receive matching validation in both synchronization workflows, and the tooling and test changes preserve their existing contracts.
|
| Filename | Overview |
|---|---|
| .github/workflows/sync-spec-tests-merge.yaml | Adds fork classification, same-repository ref validation, and conditional finalization of mirror branches without an unacknowledged actionable defect. |
| .github/workflows/sync-spec-tests-pr.yaml | Serializes updates per PR and skips both generation and synchronization for fork PRs as documented. |
| Makefile | Pins the local linter to CI’s v2.12.2 version and switches to the supported installer endpoint. |
| .github/workflows/README.md | Documents fork synchronization policy, local fixture generation, and the shared-directory limitation across checkouts. |
| README.md | Clarifies that fixture generation must precede tests and points to the detailed workflow guide. |
| qbft/spectest/run_test.go | Converts fixture-directory resolution failure from a panic into a test-scoped fatal error. |
| types/spectest/run_test.go | Converts fixture-directory resolution failure from a panic into a test-scoped fatal error. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Pull request opened or updated] --> B{Same repository?}
B -->|Yes| C[Generate fixture artifact]
C --> D[Sync mirror branch and PR]
B -->|Fork| E[Skip credential-dependent PR sync]
F[PR merged to main] --> G[Generate final fixtures]
G --> H{Fork-originated merge?}
H -->|No| I[Validate mirror branch ref]
I --> J[Push final fixtures]
J --> K[Merge spec-tests PR]
H -->|Yes| L[Skip final push]
L --> M[Fixtures carried by later qualifying in-repo merge]
Reviews (1): Last reviewed commit: "docs: fix composite action path and dead..." | Re-trigger Greptile
Codex review of the branch found three issues in what we changed: - 'curl ... | sh' reports the exit status of sh, and sh with empty input exits 0, so a failed download looked like a successful install. Download to a temp file and run it as an &&-chain instead (verified: a 404 URL now exits 56 rather than 0). - The fork comment in the merge sync claimed fixtures reach spec-tests with 'the next same-repo merge', contradicting the gap documented in README.md. - The workflows README overview claimed every in-repo PR keeps a mirror PR open; PRs whose fixtures match spec-tests never create one.
Tags are mutable refs: whoever controls the action repo can retag to different code, which then runs inside jobs holding the spec-tests App private key. Pin checkout, upload/download-artifact, create-github-app-token, setup-go, and golangci-lint-action to their resolved commit SHAs (with the version as a trailing comment). Add .github/dependabot.yml for the github-actions ecosystem so these pins keep getting bumped instead of going stale.
github-actions with directory '/' scans .github/workflows and a root action.yml only, so the setup-go pin in .github/actions/generate-spec-tests would never be bumped. Add that path explicitly.
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
1 participant
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 review comments on #610. Targets
spec-test-ciso the fixes land in that PR.3002d0052spec-tests/<module>layout, generate-before-make test, multiple-checkout collision99f5ced30fa23e5883masterinstall.shfails sha256 verification for v2 (same change as #629, plus the pin)5b53256f2PR_HEAD_REFin the merge sync before it reachesgit checkout -B/git push --force1608fb86ce5f973ae5,f69153349,50c07ff0c,2db08b6f3,59a8536e67f4aae97et.Fatalfinstead ofpanicin qbft/types spectestNot taken, with reasoning in the review threads: extracting the duplicated sync core (the control flow around it is deliberately asymmetric), renaming mirror branches to
ssv-spec-pr-<number>, and theworkflow_runrestructure for fork PRs.Two things for whoever merges:
Test / build—mainis governed by an org-level ruleset I can't read.latest.Every workflow change was reviewed by codex; the review caught a jq
// emptydefect that collapsed the PR lookup for deleted forks, fixed in59a8536e6.🤖 Generated with Claude Code