ci-cd improvements#614
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
vbhattaccmu
changed the title
There was a problem hiding this comment.
Pull request overview
This PR restructures CI/CD workflows by moving security/quality scans into dedicated nightly workflows targeting develop, and by introducing a dedicated PR quality gate via Diffguard. It also updates the Go toolchain version used by builds (including Docker builds) to address govulncheck findings.
Changes:
- Updated the repo Go version and Docker builder base image to Go
1.26.4. - Replaced push/PR-time govulncheck and SonarCloud workflows with scheduled nightly workflows that run against
develop(Sonar kept informational). - Added/updated a dedicated
diffguardPR workflow with summary output and a JSON artifact report.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
go.mod |
Bumps the declared Go version (but currently uses a patch in the go directive, which can break tooling parsing). |
Dockerfile |
Updates the builder image to golang:1.26.4-alpine to align with the intended Go patch version. |
.github/workflows/security-build.yml |
Removes the previous push/PR SonarCloud workflow. |
.github/workflows/nightly-sonar.yml |
Adds a scheduled SonarCloud workflow that checks out develop and reports status as informational. |
.github/workflows/nightly-govulncheck.yml |
Adds a scheduled govulncheck workflow that checks out develop and runs make vulncheck. |
.github/workflows/govulncheck.yml |
Removes the previous push/PR govulncheck workflow. |
.github/workflows/diffguard.yml |
Adds/updates a PR-time Diffguard workflow with step-summary output and a JSON report upload. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Note: Change the go image version in Dockerfile if you change this. | ||
| go 1.26.3 | ||
| go 1.26.4 | ||
|
|
||
| require ( |
| - name: Install diffguard | ||
| run: go install github.com/0xPolygon/diffguard/cmd/diffguard@latest |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #614 +/- ##
=========================================
+ Coverage 48.2% 50.0% +1.8%
=========================================
Files 179 181 +2
Lines 19576 19864 +288
=========================================
+ Hits 9449 9946 +497
+ Misses 8881 8644 -237
- Partials 1246 1274 +28 see 10 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| restore-keys: ${{ runner.os }}-govulncheck- | ||
|
|
||
| - name: Run govulncheck | ||
| run: make vulncheck |
There was a problem hiding this comment.
Where would we be notified about outcome? bor nightly race tests for example pushes the results on an internal slack channel. We might want to do the same for nightly checks (vulncheck and sonar)
| @@ -1,5 +1,5 @@ | |||
| # ─── BUILDER STAGE ─────────────────────────────────────────────────────────────── | |||
| FROM golang:1.26-alpine AS builder | |||
| FROM golang:1.26.4-alpine AS builder | |||
There was a problem hiding this comment.
Have you checked that this works? Had some issues in the past with patch version in dockerfiles.
There was a problem hiding this comment.
Not checked it, Will run a test and report back here
| go 1.26.3 | ||
| go 1.26.2 | ||
|
|
||
| toolchain go1.26.4 |
There was a problem hiding this comment.
can we get rid of the toolchain and only use the desired explicit go version ?
|
@claude review |
| - name: Install diffguard | ||
| run: go install github.com/0xPolygon/diffguard/cmd/diffguard@latest |
| - uses: actions/setup-go@v6 | ||
| with: | ||
| go-version-file: go.mod | ||
| check-latest: true |
| on: | ||
| schedule: | ||
| - cron: '0 2 * * *' | ||
| workflow_dispatch: |
There was a problem hiding this comment.
🟡 This PR deletes .github/workflows/govulncheck.yml (which ran make vulncheck on every push/PR gated on Go/go.mod/go.sum/Makefile changes) and replaces it with a nightly-only scan on develop. A PR that adds or bumps a dependency with a known Go CVE will now merge without any govulncheck gate — the finding only surfaces up to ~24h later against develop, after the vulnerable code has already landed. This weakens the pre-merge check the repo's own .claude/rules/security.md calls out under Dependency Security; consider keeping a lightweight PR-triggered make vulncheck (e.g. when go.mod/go.sum change) alongside the nightly develop scan.
Extended reasoning...
What changed. The old .github/workflows/govulncheck.yml was triggered on push and pull_request and used technote-space/get-diff-action to gate on changes to **/*.go, go.mod, go.sum, or Makefile; when any of those changed, it ran make vulncheck. This PR deletes that workflow and adds .github/workflows/nightly-govulncheck.yml, which only runs on schedule: '0 2 * * *' (plus manual workflow_dispatch) and always checks out develop.\n\nWhy the gate is gone. I grepped the rest of .github/workflows/ for any remaining PR-time govulncheck coverage: only nightly-govulncheck.yml references govulncheck/vulncheck, ci.yml builds/lints/tests but does not call make vulncheck, and the Makefile vulncheck target still exists but has no invoker on PRs. codeql.yml does run on PRs, but CodeQL is a semantic-analysis tool for CWE-style patterns; it does not consult the Go vulnerability database (golang.org/x/vuln) and will not flag an imported module version that matches a listed CVE, so it does not fill this gap.\n\nStep-by-step: how a vulnerable dependency now lands on develop.\n1. Author opens a PR that either (a) adds import "github.com/example/foo" where foo@vX.Y.Z has a listed vulnerability, or (b) bumps an existing dependency in go.mod/go.sum to a CVE-affected version.\n2. ci.yml runs build/lint/test/codecov. None of them run govulncheck; CodeQL's PR run will not match against golang.org/x/vuln.\n3. diffguard.yml runs quality metrics (complexity, size, mutation) — not a vulnerability scan.\n4. All required checks pass, the PR is merged into develop.\n5. Up to ~24 hours later, at 02:00 UTC, nightly-govulncheck.yml checks out develop and runs make vulncheck — the vulnerable module is now already in the tree.\n6. Feature branches (release/*, main, hotfix/*) get zero govulncheck coverage until the fix is cherry-picked or merged back into develop.\n\nAlignment with the repo's own policy. .claude/rules/security.md under "Dependency Security" explicitly lists "Run govulncheck ./... to check for known vulnerabilities" as a pre-commit / pre-PR requirement, and "Dependency updates (especially forked cosmos-sdk, cometbft, bor)" appears in the "When to Trigger Security Review" section. Moving govulncheck to nightly-only weakens the specific dependency-CVE gate the security guide names.\n\nAcknowledging the trade-off. The PR description explicitly frames this shift as intentional — the Go bump from 1.26.3 → 1.26.4 was driven by stdlib govulncheck findings, and running on PRs means unrelated PRs get blocked whenever a new stdlib CVE lands until a Go patch bump happens. That is a real papercut and nightly-on-develop does provide within-24h catch-up for stdlib drift. So this isn't necessarily wrong — hence nit.\n\nSuggested fix. Keep the nightly develop scan for stdlib CVE drift (it's genuinely useful for that), and re-add a lightweight PR-triggered govulncheck that only runs when go.mod/go.sum change (i.e. the paths-filter case where new dependencies are actually being introduced). That preserves the pre-merge gate for the case the security policy actually cares about ("newly-introduced vulnerable deps") while avoiding the noise of unrelated PRs being blocked by stdlib findings.
| diffguard: | ||
| name: Quality metrics | ||
| runs-on: ubuntu-latest | ||
| steps: |
There was a problem hiding this comment.
🟡 The new diffguard.yml workflow does not declare a permissions: block, while the two sibling workflows added in this PR (nightly-govulncheck.yml and nightly-sonar.yml, both at lines 16-17) explicitly declare permissions: contents: read. Adding permissions: contents: read at the job level (between runs-on and steps) would match the pattern established by the other new workflows and follow least-privilege defaults.
Extended reasoning...
What the finding is
.github/workflows/diffguard.yml lines 13-17 declare the job as follows:
jobs:
diffguard:
name: Quality metrics
runs-on: ubuntu-latest
steps:There is no permissions: key at either the workflow or job level. The two sibling workflows introduced in this same PR both declare it:
nightly-govulncheck.ymllines 16-17:permissions: contents: readnightly-sonar.ymllines 16-17:permissions: contents: read
A repo-wide scan shows the pattern is established elsewhere too (codeql.yml, kurtosis-e2e.yml, release_ghcr.yml, stale.yml, mainnet_deb_profiles.yml, packager_deb.yml), so diffguard.yml is the outlier both within this PR and within the repo.
Why it matters
Without an explicit permissions: block, the workflow inherits whatever the repository/org default GITHUB_TOKEN permission set is. Depending on org policy, that default can be broader than contents: read (e.g. the legacy write-all default). Combined with the unpinned go install github.com/0xPolygon/diffguard/cmd/diffguard@latest on line 36 — which the author committed to pinning to SHA 527d7fd on 2026-06-24 but the current diff still shows @latest — a compromised diffguard release could exercise whatever the default token grants.
Realized impact
Small. This workflow triggers on pull_request (not pull_request_target), so for fork PRs GitHub already restricts GITHUB_TOKEN to read-only. The steps themselves only read source, write to $GITHUB_STEP_SUMMARY, and upload an artifact — none require elevated permissions. So today there is no active vulnerability. The concern is defense-in-depth and consistency with the pattern the PR itself is establishing.
Step-by-step example
- A malicious release of
0xPolygon/diffguardis published (or the@latestinstall pulls in a compromised transitive Go module). - Diffguard runs during a
synchronizeevent on an intra-repo PR (not a fork). GITHUB_TOKENis available in the environment with whatever the repo default is. If that default iswrite-all(still the legacy default in some orgs), the malicious binary can call the GitHub API to comment on issues, push to branches, create releases, etc.- Had
permissions: contents: readbeen declared at the job level, the token would be scoped down to read-only regardless of repo-wide default, containing the blast radius.
Suggested fix
Add two lines between runs-on: ubuntu-latest and steps: to match the sibling workflows:
diffguard:
name: Quality metrics
runs-on: ubuntu-latest
permissions:
contents: read
steps:
Summary
Govulnworkflow with a newNightly Govulncheckworkflow that runs daily againstdevelop.Nightly Sonar, scheduled daily againstdevelop, with the scan kept informational viacontinue-on-error.develop.Executed tests
origin/develop.go1.26.4.make vulnchecklocally becausegois not available in this shell.Rollout notes