Skip to content

Feat/security ci restructure#264

Merged
iap merged 1 commit into
devfrom
feat/security-ci-restructure
Jun 6, 2026
Merged

Feat/security ci restructure#264
iap merged 1 commit into
devfrom
feat/security-ci-restructure

Conversation

@iap
Copy link
Copy Markdown
Member

@iap iap commented Jun 5, 2026


Open in Devin Review

Greptile Summary

This PR restructures CI/CD into a two-tier model (ci-fast.yml for every PR, ci-full.yml for nightly/manual runs) backed by reusable workflow components, replaces Gitleaks-via-Docker with trufflehog, consolidates governance workflows, updates the ZK circuit with a domain-separated 5-input Poseidon for output commitments, and regenerates the Groth16 verifier.

  • CI restructure: ci-fast.yml now correctly declares on: workflow_call:, fixing the reusable-workflow contract that ci-full.yml depends on; actions are uniformly pinned to immutable SHAs.
  • Governance check-name mismatches (flagged in prior review): \"Gitleaks Scan\" remains in the governance required-check lists while the new trufflehog job is named \"Secret Scan (trufflehog)\", so running apply-governance.sh will require a check that no CI job ever satisfies.
  • Circuit & contract: Output commitment binding corrected to 5-input Poseidon with domain-separated dstChainId; verification key constants and contract name (Groth16Verifier) updated to match the new circuit.

Confidence Score: 2/5

Not safe to merge: running apply-governance.sh will require a check named 'Gitleaks Scan' that no workflow produces, permanently deadlocking all future PRs on dev and main.

The governance required-check lists still name 'Gitleaks Scan' while the new secrets job is 'Secret Scan (trufflehog)'. The release orchestration smoke tests were dropped with no replacement.

scripts/github/apply-governance.sh and scripts/github/verify-governance.sh need the 'Gitleaks Scan' entry corrected.

Important Files Changed

Filename Overview
.github/workflows/ci-fast.yml New top-level fast CI workflow; correctly declares workflow_call so ci-full.yml can invoke it; governance required-check list still names 'Gitleaks Scan' which no workflow produces.
.github/workflows/ci-full.yml New daily/manual full CI workflow; calls ci-fast.yml via workflow_call (now valid) then adds invariant tests and the release-gate container.
scripts/github/apply-governance.sh Required-check lists updated to match new CI job names but still include 'Gitleaks Scan' — trufflehog job is named 'Secret Scan (trufflehog)' — applying this config will permanently deadlock all PRs.
.github/workflows/_reusable-circuits-core.yml New reusable circuits workflow; circomspect runs with -l ERROR only, omitting --error unconstrained_signals and --error non_deterministic_circuits flags.
circuits/mark/MARKPool.circom Output commitment corrected to 5-input Poseidon with domain-separated dstChainId. Relayer constraint uses LessThan(161) instead of Num2Bits(160); equivalent for practical values.
contracts/src/pool/verifier/MARKPoolVerifier.sol Updated verification key constants; contract renamed to Groth16Verifier (snarkjs default); referenced by address/interface so rename does not break compilation.
contracts/test/integration/bridge/CrossChainDoubleSpend.t.sol testCrossChainDoubleSpendViaBridgeIn has no assertion in step 5 (previously flagged). Other four sub-tests are well-formed.
contracts/test/unit/pool/RYLACreditLedger.t.sol testSetAdapterAllowsReSetWhenZero never exercises its claimed re-set scenario; otherwise correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    PR([Pull Request / Push]) --> CF[ci-fast.yml]
    CF --> S[Secret Scan trufflehog]
    CF --> TL[Typecheck + Lint]
    CF --> CC[Contracts Core]
    CF --> CS[Contracts Security]
    CF --> CIR[Circuits Core]
    CF --> FE[Frontend Checks]
    RG[contracts-release-gate.yml] --> RG_JOB[check: Release Gate Container]
    GOV([Governance requires: Gitleaks Scan]) -.->|no workflow produces this|DEADLOCK([PR deadlock])
    SCHED([Schedule / Dispatch]) --> FULL[ci-full.yml]
    FULL --> CF
    FULL --> INV[Contracts Core With Invariants]
Loading

Comments Outside Diff (1)

  1. scripts/github/verify-governance.sh, line 118-123 (link)

    P2 Push-restriction check inconsistent with apply-governance.sh

    check_branch hard-codes a check that restrictions.teams[].slug == "maintainers". But apply-governance.sh's new build_restrictions() function can be invoked with MAIN_PUSH_ALLOW_USERS=iap (documented in the header comments), which produces a user-based restriction with teams: []. When the governance is applied with user-based restrictions, every verify-governance.sh run will report FAIL: push restrictions do not include maintainers team on both branches. Either the verify script should also accept user-based restrictions, or the apply script should disallow that mode for protected branches.

    Fix in Codex

Fix All in Codex

Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
contracts/test/unit/pool/RYLACreditLedger.t.sol:110-130
**`testSetAdapterAllowsReSetWhenZero` never actually exercises a re-set**

The test name and its docstring promise to verify that `setAdapter` succeeds when `ADAPTER` is zero after a previous call, but the body deploys a fresh ledger and calls `setAdapter` only once (the same thing `testSetAdapterAllowsInitialSet` already tests). The second block ends with a comment explaining why the scenario cannot be tested, then does nothing. This test is not detecting any regression — a future developer could remove the guard `if (ADAPTER != address(0)) revert AdapterAlreadySet()` entirely and both tests would still pass. Either exercise the actual invariant (e.g., slot-write `ADAPTER` back to zero via `vm.store` and re-call) or rename the test to reflect what it actually covers.

### Issue 2 of 2
.github/workflows/_reusable-frontend-checks.yml:30-34
`fetch-depth: 0` clones the full repository history, which is unnecessary for a workflow that only runs `pnpm typecheck` and `pnpm lint`. The previous behaviour (shallow clone, depth 1) is sufficient and faster.

```suggestion
      - name: Checkout
        uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
        with:
          submodules: recursive
```

Reviews (25): Last reviewed commit: "ci: fix all CI failures for PR #264" | Re-trigger Greptile

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 5, 2026

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails
actions/github/codeql-action/upload-sarif afb54ba388a7dca6ecae48f608c4ff05ff4cc77a UnknownUnknown
actions/jdx/mise-action 691e684984939740113c1a3ae421e8a58f5b33cb UnknownUnknown
npm/drizzle-orm 0.45.2 UnknownUnknown

Scanned Files

  • .github/workflows/_reusable-secrets-scan.yml
  • .github/workflows/codeql.yml
  • .github/workflows/contracts-release-gate-container.yml
  • .github/workflows/governance-policy-guard.yml
  • .github/workflows/governance-verify.yml
  • .github/workflows/zk-proof-tests.yml
  • pnpm-lock.yaml

greptile-apps[bot]

This comment was marked as resolved.

@iap iap marked this pull request as ready for review June 5, 2026 08:09
@iap iap requested a review from a team as a code owner June 5, 2026 08:09
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fd00a59095

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread .github/workflows/ci-fast.yml Outdated
Comment thread .github/workflows/ci-full.yml
Comment thread scripts/github/verify-governance.sh Outdated
Comment thread circuits/mark/MARKPool.circom
Comment thread .github/workflows/ci-fast.yml Outdated
Comment thread contracts/docker/release-gate.Dockerfile
Comment thread contracts/test/integration/bridge/CrossChainDoubleSpend.t.sol
Comment thread .github/workflows/ci-fast.yml Outdated
Comment thread scripts/github/apply-governance.sh Outdated
Comment thread circuits/setup.mjs
greptile-apps[bot]

This comment was marked as resolved.

greptile-apps[bot]

This comment was marked as resolved.

greptile-apps[bot]

This comment was marked as resolved.

Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 9 potential issues.

Open in Devin Review

Comment thread scripts/github/verify-governance.sh Outdated
Comment thread .github/workflows/ci-fast.yml Outdated
Comment thread contracts/src/pool/RYLACreditLedger.sol Outdated
Comment thread contracts/src/pool/verifier/MARKPoolVerifier.sol
Comment thread circuits/mark/MARKPool.circom
Comment thread .github/workflows/_reusable-frontend-checks.yml Outdated
Comment thread .mise.toml
Comment thread circuits/mark/MARKPool.circom
Comment thread .github/workflows/ci-full.yml
@iap iap removed the request for review from a team June 5, 2026 19:48
@devin-ai-integration devin-ai-integration Bot force-pushed the feat/security-ci-restructure branch 2 times, most recently from 0d29804 to ee808ae Compare June 5, 2026 20:03
@devin-ai-integration devin-ai-integration Bot reopened this Jun 5, 2026
@iap iap self-assigned this Jun 5, 2026
@iap iap added bug Something isn't working security Security vulnerability or hardening labels Jun 5, 2026 — with Devin.ai Integration
@iap iap removed their assignment Jun 5, 2026
devin-ai-integration Bot added a commit that referenced this pull request Jun 5, 2026
Address Devin Review findings on PR #264:

- apply/verify-governance.sh: reusable-workflow jobs in ci-fast.yml are
  reported by GitHub as compound "<caller> / <callee>" check names, not the
  caller name alone. Required-check lists used the plain caller names, so
  running apply-governance.sh would have deadlocked dev/main (strict checks
  that GitHub never reports). Use the compound names for all reusable-backed
  jobs and keep plain names only for direct jobs.
- Remove required checks 'Contracts Release Check (Dry-Run + Execute Smoke)'
  and 'Contracts Production Mode Smoke' — no workflow emits these contexts, so
  requiring them would also block merges indefinitely.
- verify-governance.sh: accept either team-based (maintainers) or user-based
  push restrictions, matching build_restrictions() in apply-governance.sh; and
  skip the dismiss_stale_reviews assertion when required approvals is 0 (reviews
  are disabled for the solo maintainer).
- contracts-release-gate.yml: fix stale push path filter referencing the old
  filename contracts-release-gate-container.yml.
- RYLACreditLedger.setAdapter: drop the redundant '&& adapter_ != address(0)'
  term (behavior-equivalent to the prior single-guard form) and correct the
  misleading comment; ADAPTER can never be reset to zero once set.

Co-Authored-By: Iko <iap@disroot.org>
devin-ai-integration Bot added a commit that referenced this pull request Jun 5, 2026
Address Devin Review findings on PR #264:

- apply/verify-governance.sh: reusable-workflow jobs in ci-fast.yml are
  reported by GitHub as compound "<caller> / <callee>" check names, not the
  caller name alone. Required-check lists used the plain caller names, so
  running apply-governance.sh would have deadlocked dev/main (strict checks
  that GitHub never reports). Use the compound names for all reusable-backed
  jobs and keep plain names only for direct jobs.
- Remove required checks 'Contracts Release Check (Dry-Run + Execute Smoke)'
  and 'Contracts Production Mode Smoke' — no workflow emits these contexts, so
  requiring them would also block merges indefinitely.
- verify-governance.sh: accept either team-based (maintainers) or user-based
  push restrictions, matching build_restrictions() in apply-governance.sh; and
  skip the dismiss_stale_reviews assertion when required approvals is 0 (reviews
  are disabled for the solo maintainer).
- contracts-release-gate.yml: fix stale push path filter referencing the old
  filename contracts-release-gate-container.yml.
- RYLACreditLedger.setAdapter: drop the redundant '&& adapter_ != address(0)'
  term (behavior-equivalent to the prior single-guard form) and correct the
  misleading comment; ADAPTER can never be reset to zero once set.

Co-Authored-By: Iko <iap@disroot.org>
@devin-ai-integration devin-ai-integration Bot force-pushed the feat/security-ci-restructure branch from cd1f1ff to ba6ac91 Compare June 5, 2026 22:04
@devin-ai-integration
Copy link
Copy Markdown

@greptileai

Re-review requested after addressing the prior findings in ba6ac91:

  • Governance scripts now use the compound <caller> / <callee> check names that GitHub actually reports for reusable-workflow jobs, and drop two required checks that no workflow emits (would have deadlocked dev/main).
  • verify-governance.sh now accepts user-based push restrictions and skips the stale-review assertion when required approvals = 0, matching apply-governance.sh.
  • contracts-release-gate.yml push path filter points at the current filename.
  • RYLACreditLedger.setAdapter redundant guard removed and comment corrected.

@iap iap added the ci CI/CD workflow changes label Jun 5, 2026
@devin-ai-integration devin-ai-integration Bot force-pushed the feat/security-ci-restructure branch from 67f4192 to 8b7d552 Compare June 5, 2026 22:22
@devin-ai-integration
Copy link
Copy Markdown

@greptileai

Addressed the docs drift in 8b7d552: docs/BRANCHING.md and .github/PRODUCTION_GOVERNANCE_CHECKLIST.md now use the compound <caller> / <callee> check names matching apply-governance.sh, and the two removed checks (Contracts Release Check (Dry-Run + Execute Smoke), Contracts Production Mode Smoke) are gone. scripts/ci/validate-governance-policy.sh passes locally.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 5, 2026

Both files look correct in 8b7d552. All five compound names are in place across every required-checks section in both docs:

  • Contracts Core (Unit + Invariant) / Contracts Core
  • Contracts Security (Semgrep + Slither) / Semgrep Scan
  • Contracts Security (Semgrep + Slither) / Slither Core Contracts
  • Circuits Core (Build + Tests + Circomspect) / Circuits Core
  • Frontend Checks (Node 24) / Frontend Checks (Node 24.3.0)

And both dropped checks (Contracts Release Check (Dry-Run + Execute Smoke), Contracts Production Mode Smoke) are gone from all four locations. The docs-drift issues are resolved.

Comment thread .github/workflows/_reusable-circuits-core.yml Outdated
@iap iap marked this pull request as draft June 5, 2026 23:12
@iap iap marked this pull request as ready for review June 5, 2026 23:12
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8b7d552a50

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread .github/workflows/_reusable-frontend-checks.yml Outdated
Comment thread .github/workflows/_reusable-circuits-core.yml Outdated
Comment thread .github/workflows/contracts-mainnet-readiness.yml
Comment thread .github/PRODUCTION_GOVERNANCE_CHECKLIST.md
Comment thread .github/workflows/_reusable-circuits-core.yml
Comment thread scripts/github/apply-governance.sh
@github-advanced-security
Copy link
Copy Markdown

You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool.

What Enabling Code Scanning Means:

  • The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
  • Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
  • You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

- Replace Gitleaks with trufflehog (v3.95.5, pinned SHA)
- Fix circom build: add mkdir -p build before compile
- Update gas snapshots (forge snapshot)
- Fix semgrep/slither: use uvx/uv run, fix config format
- Fix circomspect: cargo caching, pnpm install, correct lib path, SARIF upload
- Format fixes: forge fmt, shfmt
- Replace Gitleaks license with trufflehog
- Fix Slither config: string format, add @interop-lib remap, increase timeout
- Fix reusable workflows: semgrep/slither installation steps

All 27 original commits squashed with author Iko <iap@disroot.org>
@iap iap force-pushed the feat/security-ci-restructure branch from a1a504c to 3326bb1 Compare June 6, 2026 11:18
@iap iap merged commit f13f179 into dev Jun 6, 2026
25 of 27 checks passed
@iap iap deleted the feat/security-ci-restructure branch June 6, 2026 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ci CI/CD workflow changes security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants