ci: add PR checks for develop (CI/CD Phase 1, PR 2 of 4) - #118
Conversation
Implements the workflow proposed in discussion InfiniteZeroFoundation#74. Runs on `pull_request` targeting `develop`, on open and on every push to the PR. Three jobs, plus a `CI OK` summary job intended to be the single required status check. One context keeps branch protection simple and avoids the trap where path-filtered jobs are marked required: a docs-only PR would wait forever on Solidity checks that never report. Blocking today: forge build/test (241 tests), hardhat compile/test (32), pytest -m "not integration" (298), and a relative-link check over Documentation/ (154 links). Advisory today: `forge lint` and `ruff`. Both are real but carry a backlog — 37 Solidity findings and 128 Python ones under the proposed gate set — and a gate that fails on pre-existing findings teaches contributors to fight CI. They run with continue-on-error and get promoted in a follow-up once the backlog is cleared. `forge lint` is gated through .github/scripts/forge_lint_gate.py rather than by scraping human-readable output. Two traps make that necessary: `--json` writes diagnostics to stderr, and it cannot be combined with `--color` (which exits 2 emitting nothing). The parser validates forge's diagnostic schema, not just its JSON syntax, so an unrecognised record type or a diagnostic with a missing severity fails rather than being silently skipped. Pinned deliberately: the Foundry release (v1.7.1 — the toolchain every measurement was taken against, and whose output the lint gate parses), the action SHAs, and the Python test versions via .github/constraints-ci.txt. `version: stable` would let an upstream release change compilation or lint output on an unrelated PR. `npm ci` runs in foundry/ before `forge test`, without which Upgrades.validateImplementation's parallel FFI calls to npx @openzeppelin/upgrades-core race each other. It also stops a test from fetching a package at test time, which matters because foundry.toml sets ffi = true. `defaults.run.shell: bash` is explicit rather than assumed: ci-ok's result loop relies on word splitting, which zsh and dash do not perform. .gitignore gains a general `__pycache__/` rule — .github/scripts/ is imported by tests, which was not a bytecode site before. No secrets are used or needed, so PRs from forks work and are safe by default. `main` is untouched. tests/test_ci_scripts.py covers both helper scripts (21 tests): the link checker's fence tracking and the lint gate's fail-closed schema validation. Verified locally against this branch rebased onto develop, by running every job's steps in order: hardhat green (32), python green (298 passed, 133 deselected), docs green (154 links, 0 broken). The advisory lint steps report their backlog without failing their jobs. The solidity job is RED until InfiniteZeroFoundation#117 merges, and not for a reason in this PR. develop currently fails 3 of 240 forge tests in SlashingInvariants.t.sol -- stale assertions against the pre-InfiniteZeroFoundation#65 slash() semantics. This PR touches no files under foundry/, so its merge commit inherits that failure verbatim. InfiniteZeroFoundation#117 fixes it (241 passed); this PR must not merge before it. That first red run is worth reading rather than apologising for: the gate's very first execution catches three tests that have been broken on develop since InfiniteZeroFoundation#66 and passed human review. That is the case for the gate, made by the gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wu3sbCps6PEhyvsQNGK3pr
|
Reviewed in an isolated worktree, branch head Claimed: the Solidity job is red on this PR's own branch (pre-#117), but that's not a defect in this PR — it's inherited from Verified — and independently checked the part the PR body couldn't (#117 wasn't merged yet when it was written): ran Claimed: all 4 pinned Action SHAs correspond to the versions named in their comments ( Verified exactly as claimed — queried each action's repo directly ( Claimed: Verified exactly as claimed — ran Claimed: Verified exactly as claimed, all four failure modes reproduced directly: piped synthetic records at the script — an unrecognised Claimed: the doc-link checker finds 154 relative links under Verified exactly as claimed — ran Claimed: Verified exactly as claimed — ran it directly: 21/21 pass, covering exactly the schema-fail-closed cases and fence-tracking edge cases described above. Claimed: Python job — 298 passed, 133 deselected; hardhat job — 32 passing; both green on the merge commit today. Verified exactly as claimed — Claimed: no secrets are used or needed, so fork PRs work safely by default; Verified — grepped the entire workflow for Not independently re-verified: the actual GitHub Actions run itself (this review was static/local execution of each job's steps, not a live Actions run) — which is exactly item 1 of the PR's own "three things I need" list, addressed separately below. Every checkable claim held up, including the one this PR's own description couldn't verify yet (that the Solidity job is actually green once #117 lands — now confirmed directly with a real trial merge onto current |
Files changed (6) — as of
|
| Field | Value |
|---|---|
| Change | New |
| Lines | +163 |
| Diff (what exactly is in this PR) | New pull_request: branches: [develop] workflow: solidity job (forge build/test, npm ci first, advisory forge lint via the gate script, then hardhat compile/test), python job (pytest -m "not integration", advisory ruff), docs job (relative-link check), and a ci-ok summary job that fails if any of the three didn't succeed — intended as the single required status check. |
| Functionality — how & why | How: all 4 third-party actions are pinned to full commit SHAs (verified each resolves to the exact tag named in its comment: checkout v7.0.1, setup-node v7.0.0, setup-python v7.0.0, foundry-toolchain v1.9.1), Foundry itself pinned to v1.7.1 via FOUNDRY_VERSION rather than stable. npm ci in foundry/ runs before forge test specifically because Upgrades.validateImplementation shells out over FFI (foundry.toml has ffi = true, confirmed) to npx @openzeppelin/upgrades-core, and parallel test execution racing an unpinned fetch is a real, reproducible failure mode. permissions: contents: read only, zero secrets.* references anywhere — fork PRs run safely with no maintainer-side secret exposure. Why: ci-ok as the single required context avoids the path-filter/required-check deadlock where a docs-only PR waits forever on a Solidity job that path-filtering never triggers. Pinning everything (actions, Foundry, PIP_CONSTRAINT) means an upstream release can't silently change compilation or lint output on an unrelated PR. |
Diff vs current develop HEAD |
None — untouched since the merge-base; net-new file. |
| Recommended merge proposal | Merged as-is. Independently verified all 4 action SHAs against their claimed tags via the GitHub API, confirmed ffi = true and zero secrets usage by reading the files directly, and — since this PR's own body couldn't verify it yet (written before #117 merged) — did a real trial merge onto current develop and ran forge test: 254/254 pass, confirming the Solidity job is actually green post-merge. |
| Actual merge proposal | Soon |
| Pending proposal | See the separate comment on the three asks from the PR body (workflow-run approval, branch protection). |
| Local merge conflict | No |
| GitHub merge conflict | No |
.github/scripts/forge_lint_gate.py
| Field | Value |
|---|---|
| Change | New |
| Lines | +119 |
| Diff (what exactly is in this PR) | Parses forge lint --json's newline-delimited JSON diagnostics on stdin and fails closed: an unparseable line, an unrecognised $message_type, a diagnostic missing level/code, or an unknown severity all raise SchemaError and exit 1 — not just a warning/error finding itself. |
| Functionality — how & why | How: independently reproduced all four fail-closed paths with synthetic input (unrecognised message type, missing level, unknown severity, unparseable JSON — each fails with a specific diagnostic message), confirmed empty input passes cleanly, and ran it against real forge lint --json output on the current tree — correctly reports 37 findings (31 unsafe-typecast, 6 block-timestamp), an exact match to the count independently confirmed in #117's review. Why: forge lint --json can't be combined with --color (confirmed: exits 2, "cannot be used with") and writes to stderr not stdout (confirmed: 0 bytes stdout, 60,059 bytes stderr) — a plain grep over human-readable output would be fragile to either trap; validating the schema itself means a future Foundry release changing the diagnostic shape fails the gate loudly instead of silently passing nothing. |
Diff vs current develop HEAD |
None — untouched since the merge-base; net-new file. |
| Recommended merge proposal | Merged as-is. All fail-closed paths and the real 37-finding count independently reproduced. |
| Actual merge proposal | Soon |
| Pending proposal | None |
| Local merge conflict | No |
| GitHub merge conflict | No |
.github/scripts/check_doc_links.py
| Field | Value |
|---|---|
| Change | New |
| Lines | +89 |
| Diff (what exactly is in this PR) | Checks inline relative Markdown links under Documentation/, skipping fenced code blocks and external/anchor-only links. Docstring explicitly states what it does not handle (reference-style links, multi-line destinations, balanced-paren destinations, percent-encoding) and confirms none of those forms exist in the current corpus. |
| Functionality — how & why | How: iter_prose's fence tracking records both the opening fence character and run length, so a closer must match both — read this directly and confirmed it correctly prevents a ~~~ block containing a literal triple-backtick from toggling fence state early, a real bug class a naive single-character check would miss. Ran it directly: 154 links checked, 0 broken, exact match to the PR's claim. Why: a docs-only PR needs a real, cheap, false-positive-free check to be part of the blocking docs job; the honestly-scoped docstring means the check's limits are documented rather than silently assumed away. |
Diff vs current develop HEAD |
None — untouched since the merge-base; net-new file. |
| Recommended merge proposal | Merged as-is. Fence-tracking logic and the 154/0 count both independently verified. |
| Actual merge proposal | Soon |
| Pending proposal | None |
| Local merge conflict | No |
| GitHub merge conflict | No |
tests/test_ci_scripts.py
| Field | Value |
|---|---|
| Change | New |
| Lines | +153 |
| Diff (what exactly is in this PR) | 21 tests covering both helper scripts: the lint gate's fail-closed schema validation (unknown message type, missing level, unknown severity, missing code, unparseable/non-object JSON) and the doc-link checker's fence-tracking edge cases (backtick vs. tilde fences, longer/shorter runs, indented fences, unterminated fences, closing fences with an info string). |
| Functionality — how & why | How/why: directly exercises the exact edge cases both scripts' docstrings claim to handle, at the unit level rather than only through the real forge lint/doc-corpus runs. Ran directly: 21/21 pass. |
Diff vs current develop HEAD |
None — untouched since the merge-base; net-new file. |
| Recommended merge proposal | Merged as-is. |
| Actual merge proposal | Soon |
| Pending proposal | None |
| Local merge conflict | No |
| GitHub merge conflict | No |
.github/constraints-ci.txt
| Field | Value |
|---|---|
| Change | New |
| Lines | +3 |
| Diff (what exactly is in this PR) | Pins torch==2.6.0, pytest==9.1.1, ruff==0.16.4, applied via the workflow's PIP_CONSTRAINT env var. |
| Functionality — how & why | How/why: keeps the Python job's dependency versions fixed the same way the Solidity job pins Foundry and action SHAs — an unrelated PR shouldn't see a different test/lint outcome because an upstream package released overnight. |
Diff vs current develop HEAD |
None — untouched since the merge-base; net-new file. |
| Recommended merge proposal | Merged as-is. |
| Actual merge proposal | Soon |
| Pending proposal | None |
| Local merge conflict | No |
| GitHub merge conflict | No |
.gitignore
| Field | Value |
|---|---|
| Change | Modified |
| Lines | +5 |
| Diff (what exactly is in this PR) | Adds a general __pycache__/ rule, since .github/scripts/ is now imported by tests and wasn't a bytecode site before. |
| Functionality — how & why | How/why: pure hygiene addition, comment explicitly notes the existing path-specific rules are now redundant but left in place to keep the diff small. |
Diff vs current develop HEAD |
None — untouched since the merge-base. |
| Recommended merge proposal | Merged as-is. |
| Actual merge proposal | Soon |
| Pending proposal | None |
| Local merge conflict | No |
| GitHub merge conflict | No |
Verification
Independently reproduced every real-execution claim in the PR body: forge lint gate against real output (37 findings, exact match) and against 4 synthetic schema-violation cases; doc-link checker (154/0, exact match); test_ci_scripts.py (21/21); pytest -m "not integration" (298 passed, 133 deselected, exact match); hardhat test (32 passing, exact match); all 4 pinned action SHAs verified against their tags via the GitHub API. Went one step further than the PR body could at time of writing: did a real trial merge onto current develop (which now has #117) and ran forge test there — 254/254 pass, confirming the Solidity job's red state was genuinely inherited and is now resolved. Full detail in the deep-verification comment above.
Local vs. GitHub agree: both report a clean, conflict-free merge.
|
Update (2026-09-10): PR #118 has since merged to @umermjd11 — checked all three asks directly against the repo's current state (my token has 1. Workflow-run approval — likely needed, but I can't fully confirm it eitherQueried the Actions API for this PR's exact head commit ( What to check: open the PR's Checks tab, or the repo's Actions tab, in the browser. If you see a banner or a run sitting in "action required," click Approve and run workflows. If you see genuinely nothing queued, that's new information — worth a comment on the PR either way so Santiago isn't stuck guessing. Update: moot for this PR specifically now that it's merged — there's nothing left to approve on #118 itself. Still relevant going forward: 2. Branch protection on
|
Actual outcome — PR #118 merged (pushed)One commit, on
Files unchanged from the PR (6 of 6)
Verification (against the actually-pushed state, not just the PR's reported numbers)
Local vs. GitHub agree: clean merge as predicted, no surprises. Not done as part of this mergeThe PR's two operational asks — approving the currently-held workflow run, and turning on branch protection with |
|
Status update — both pending items resolved. 1. Workflow-run approval — resolved, nothing further neededConfirmed via the Actions API: 2. Branch protection on
|
Important
Blocked on #117 — please merge that first. This PR touches no files under
foundry/, butdevelopcurrently fails 3 of 240forge testcases, so this PR's merge commit inherits them and the Solidity job goes red. #117 fixes exactly those three. Draft until it lands.PR 2 of 4 for CI/CD Phase 1, per discussion #74. PR 1 (#103) made the tree CI-ready; this turns the gate on.
What runs
One workflow on
pull_request→develop, three jobs plus aCI OKsummary job:forge build/test,hardhat compile/testpytest -m "not integration"Documentation/forge lintruffCI OKis meant to be the single required status check. One context keeps branch protection simple and avoids the trap where path-filtered jobs are marked required — a docs-only PR would otherwise wait forever on Solidity checks that never report.The two lint checks ship advisory on purpose. Making them blocking today would fail every PR on a pre-existing backlog and teach people to fight CI. They get promoted in PR 3, once the backlog is cleared.
About that red check
The Solidity job will be red until #117 merges, and it's worth reading rather than apologising for: the gate's very first run catches three tests that have been broken on
developsince #66 and passed human review. That's the case for having a gate, made by the gate.Python and Docs are green on the merge commit right now — I ran every job's steps locally against this branch rebased onto
develop. So the red is isolated and legible, not ambiguous.Heads-up for other contributors
Once this lands on
develop, open PRs targetingdevelopstart getting gated on their next push — currently #116, #110, #109, #77. Some may go red on the advisory-free blocking checks. That's the intended effect, but nobody should be surprised by it. PRs targeting feature branches (#72, #32, #31, #29, #27) are unaffected, since the trigger isbranches: [develop]only.A few choices worth flagging
Pinned, not floating. The Foundry release (
v1.7.1), the action SHAs, and the Python test versions (.github/constraints-ci.txt) are all pinned.version: stablewould let an upstream release change compilation or lint output on an unrelated PR.npm ciinfoundry/beforeforge test. Without it,Upgrades.validateImplementation's parallel FFI calls tonpx @openzeppelin/upgrades-corerace each other. Reproducible failure, and it also stops a test fetching a package at test time — which matters becausefoundry.tomlsetsffi = true.forge lintgoes through a parser, not a grep..github/scripts/forge_lint_gate.pyvalidates forge's diagnostic schema, so an unrecognised record type or a diagnostic missing its severity fails closed rather than being silently skipped. Two traps made this necessary:--jsonwrites diagnostics to stderr, and it can't be combined with--color(which exits 2 emitting nothing).No secrets are used or needed, so fork PRs work and are safe by default.
mainis untouched.tests/test_ci_scripts.pycovers both helper scripts (21 tests): the link checker's fence tracking and the lint gate's fail-closed schema validation.Three things I need from @umermjd11
triage, not write — GitHub holds workflow runs from fork PRs by non-write contributors until a maintainer clicks Approve and run workflows. I can't confirm it directly (runs inaction_requiredaren't visible without write access), so if you look and there's genuinely nothing queued, tell me and I'll dig further.develop? This PR is worth much less without it — the plan uses a singleCI OKcontext. I don't have admin.Is Actions enabled?Answered while writing this — yes, 248 runs. The four active workflows are all GitHub-managed (Dependabot, Dependency Graph, CodeQL default setup); there's no user-defined workflow file on any branch, so this is genuinely the first one.