refactor(ci): simplify workflows, update deps #270
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request updates GitHub Actions workflows and refactors the setup action to modernize the CI infrastructure. Changes include upgrading to Ubuntu 24.04, updating action versions, introducing corepack and Turbo-based caching, and integrating a dedicated compact compiler setup action. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/actions/setup/action.yml(1 hunks).github/workflows/apps.yml(2 hunks).github/workflows/checks.yml(2 hunks).github/workflows/contracts.yml(1 hunks).github/workflows/scorecard.yml(2 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
.github/workflows/contracts.yml
[warning] 11-32: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
.github/workflows/checks.yml
[warning] 15-47: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
.github/workflows/apps.yml
[warning] 15-79: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (10)
.github/workflows/checks.yml (1)
15-15: Runner and action version updates look good.Upgrading to ubuntu-24.04, harden-runner v2.13.3, and typos v1.40.0 maintains modern tooling. The version choices are consistent with other workflows in the PR.
Also applies to: 19-19, 39-39
.github/workflows/contracts.yml (1)
11-11: Runner and action updates are consistent.The ubuntu-24.04 upgrade and harden-runner v2.13.3 bump align with the modernization across the PR.
Verify that the removal of the "Check compiler version" validation step is properly handled by the new Setup Compact Compiler action (.github/actions/setup/action.yml, line 45-49). The setup action now manages compiler setup, but ensure the CI still validates compiler availability/correctness.
Also applies to: 16-16
.github/workflows/apps.yml (2)
25-25: Checkout action version inconsistent with other workflows.Line 25 uses
actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1, but other workflows in the PR (checks.yml, contracts.yml, scorecard.yml) use v5.0.0. This inconsistency may be unintended.Verify whether v6.0.1 is intentional for apps.yml or should be unified with v5.0.0 across all workflows. If v6.0.1 is required for this workflow, document the reason to help future maintainers understand the divergence.
Also applies to: 25-25
15-15: Runner, harden-runner, and fetch-depth updates are appropriate.Ubuntu-24.04 modernization and harden-runner v2.13.3 are consistent with other workflows. Setting fetch-depth to 2 aligns with Turbo team recommendations.
Also applies to: 20-20, 27-27
.github/workflows/scorecard.yml (2)
18-18: Excellent permissions configuration.This workflow correctly declares
permissions: read-allat the workflow level and granular permissions at the job level. This is a security best practice and serves as a reference example for the other workflows that are missing permissions blocks.Also applies to: 22-30
22-22: Runner and action updates are consistent with broader PR modernization.Ubuntu-24.04 upgrade and all action version bumps align with the tooling modernization across the repository.
Also applies to: 33-33, 38-38, 43-43, 52-52, 59-59
.github/actions/setup/action.yml (4)
13-15: Corepack enable ordering is correct, but verify Node.js includes corepack.Corepack is enabled before Node.js setup (line 13), then Node.js is installed (line 29). The ordering is sound since corepack became part of Node.js 16.13+.
Verify that the Node.js 22.x version on ubuntu-24.04 includes corepack. If corepack is not available by default, you may need to explicitly install it or adjust the step order. The current setup assumes corepack is available as part of the Node.js runtime.
Also applies to: 29-29
38-43: Turbo global installation pattern is reasonable, telemetry consolidation is good.Installing Turbo globally with
pnpm install turbo@${{ env.TURBO_MAJOR_VERSION }} -gis an appropriate pattern. The consolidation ofTURBO_TELEMETRY_DISABLEDfrom contracts.yml into this action is a good refactor that simplifies workflow configuration.
1-1: Node.js, pnpm, and corepack setup updates are appropriate for modernization.Quoting the action name/description, updating pnpm/action-setup to v4.2.0, actions/setup-node to v6.1.0, and setting node-version to "22.x" with pnpm caching are all consistent with modern best practices.
Also applies to: 2-2, 26-26, 29-29, 31-31, 32-32
47-47: The referenced commit hash is valid and appears to be actively maintained (dated October 17, 2025). However, themidnightntwrk/setup-compact-actionrepository does not publish version tags or releases—it relies on commit hashes as its standard versioning approach. If the upstream project maintains this pattern by design, using the commit hash is appropriate. Consider documenting why commit-based pinning is used here if it differs from other action references in the workflow.
| description: "Skip the fast compile step" | ||
| required: false | ||
| default: 'false' | ||
| default: "false" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/actions/setup/action.ymlRepository: OpenZeppelin/midnight-apps
Length of output: 1856
🏁 Script executed:
# Search for usage of skip-compile input within the action file itself
rg 'skip-compile|skip-compact' .github/actions/setup/action.yml -nRepository: OpenZeppelin/midnight-apps
Length of output: 137
🏁 Script executed:
# Find all workflow files that call this setup action
fd -t yaml . .github/workflows | head -20Repository: OpenZeppelin/midnight-apps
Length of output: 241
🏁 Script executed:
# Look for how skip-compile/skip-compact is referenced in callers
rg 'uses:.*setup' .github/workflows -A 5 -B 1 -t yamlRepository: OpenZeppelin/midnight-apps
Length of output: 1182
🏁 Script executed:
# Check if there are additional inputs defined or if skip-compact is defined
rg 'skip-compact' .github/actions/setup/ -rRepository: OpenZeppelin/midnight-apps
Length of output: 483
🏁 Script executed:
# Verify if there's any git history showing a previous version
git log --oneline -n 20 -- .github/actions/setup/action.yml 2>/dev/null | head -10Repository: OpenZeppelin/midnight-apps
Length of output: 143
🏁 Script executed:
# Check the full context around the input section
head -50 .github/actions/setup/action.yml | tail -30Repository: OpenZeppelin/midnight-apps
Length of output: 1060
Input name mismatch prevents skip-compile flag from working correctly.
The action defines an input named skip-compile (line 5), but the condition at line 46 references inputs.skip-compact which does not exist. This mismatch means the skip-compile input is ignored entirely—the condition will always evaluate to true (step always runs) because inputs.skip-compact is undefined.
To fix: Either rename the input to skip-compact or update the condition to reference inputs.skip-compile.
🤖 Prompt for AI Agents
In .github/actions/setup/action.yml around lines 8 and 46, the action defines an
input named "skip-compile" but the condition references "inputs.skip-compact",
causing the skip flag to be ignored; update the condition at line ~46 to
reference "inputs.skip-compile" (or alternatively rename the input to
"skip-compact") so the intended skip-compile input is actually used, and ensure
any other references in the file use the same exact input name.
There was a problem hiding this comment.
line 5, is skip-compact not skip-compile
There was a problem hiding this comment.
@0xisk You're absolutely right, and I apologize for the confusion. Looking at the full file content after this PR, line 5 indeed shows skip-compact, not skip-compile.
The diff was showing the change FROM skip-compile (old) TO skip-compact (new), and I misread it. Your PR is actually fixing the mismatch by renaming the input to skip-compact to match the condition at line 46 that references inputs.skip-compact. This is the correct change.
Thank you for the correction!
.github/actions/setup/action.yml
Outdated
| - name: Cache turbo build setup | ||
| uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 # v4.3.0 | ||
| with: | ||
| path: ~/.compact | ||
| key: compact-tools-${{ env.COMPILER_VERSION }} | ||
| path: .turbo | ||
| key: ${{ runner.os }}-turbo-${{ hashFiles('.turbo/*') }}-${{ github.sha }} | ||
| restore-keys: | | ||
| compact-tools- | ||
| - name: Install Compact developer tools | ||
| if: steps.cache-compact.outputs.cache-hit != 'true' | ||
| shell: bash | ||
| run: | | ||
| curl --proto '=https' --tlsv1.2 -LsSf https://github.com/midnightntwrk/compact/releases/download/compact-v0.2.0/compact-installer.sh | sh | ||
| - name: Add Compact to PATH | ||
| shell: bash | ||
| run: | | ||
| echo "$HOME/.compact/bin" >> "$GITHUB_PATH" | ||
| - name: Verify Compact installation | ||
| shell: bash | ||
| run: | | ||
| echo "🔍 Checking Compact installation..." | ||
| echo "HOME: $HOME" | ||
| ls -la "$HOME/.compact/bin/" || echo "⚠️ .compact/bin directory not found" | ||
| if [ -f "$HOME/.compact/bin/compact" ]; then | ||
| echo "✅ Compact binary found at $HOME/.compact/bin/compact" | ||
| else | ||
| echo "❌ Compact binary not found, installing..." | ||
| curl --proto '=https' --tlsv1.2 -LsSf https://github.com/midnightntwrk/compact/releases/download/compact-v0.2.0/compact-installer.sh | sh | ||
| fi | ||
| - name: Install and verify toolchain | ||
| shell: bash | ||
| run: | | ||
| export PATH="$HOME/.compact/bin:$PATH" | ||
| compact update ${{ env.COMPILER_VERSION }} | ||
| echo "🤖 Testing installation..." | ||
| compact compile --version | ||
| compact compile --language-version | ||
| ${{ runner.os }}-turbo-${{ hashFiles('.turbo/*') }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if .turbo directory exists in the repository
find . -type d -name '.turbo' 2>/dev/null | head -20
# Check if .turbo is in .gitignore
if [ -f .gitignore ]; then
echo "=== Checking .gitignore for .turbo ==="
grep -n "\.turbo" .gitignore || echo "No .turbo entry found in .gitignore"
fi
# Check git status of .turbo if it exists
if [ -d .turbo ]; then
echo "=== Git status of .turbo ==="
git status .turbo 2>/dev/null || echo "Cannot check git status"
fiRepository: OpenZeppelin/midnight-apps
Length of output: 119
🏁 Script executed:
# Read the full action.yml file to understand context
cat -n .github/actions/setup/action.ymlRepository: OpenZeppelin/midnight-apps
Length of output: 1856
🏁 Script executed:
# Verify GitHub Actions hashFiles behavior with non-existent paths
# by checking documentation or common patterns
web_search
GitHub Actions hashFiles empty directory non-existent path behaviorRepository: OpenZeppelin/midnight-apps
Length of output: 165
Turbo cache key generates empty hash because .turbo/ directory is git-ignored and doesn't exist.
The .turbo/ directory is listed in .gitignore (line 151) and is not committed to the repository. Since the cache step (lines 17-23) runs before any build commands that would create this directory, hashFiles('.turbo/*') will return an empty string, resulting in inconsistent cache keys. This causes the cache key to depend only on runner.os and github.sha, reducing cache effectiveness and potentially causing cache collisions across different workflow runs with the same commit.
Consider either: (1) committing a .turbo/.gitkeep placeholder file, (2) adding a step to create .turbo/ with content before the cache action, or (3) adjusting the cache key strategy to account for missing directories.
🤖 Prompt for AI Agents
In .github/actions/setup/action.yml around lines 17-23 the cache key uses
hashFiles('.turbo/*') which returns empty because .turbo/ is git-ignored and not
present at cache time; fix by either (A) add a committed placeholder file (e.g.
.turbo/.gitkeep) so the hash is stable, (B) insert a prior workflow step that
creates the .turbo directory and writes a deterministic file before the cache
action, or (C) change the cache key to use a stable file(s) (e.g. package.json,
turbo config) or include a fallback static value so the key isn’t empty — pick
and implement one of these options and update the cache step accordingly.
| jobs: | ||
| pipeline: | ||
| runs-on: ubuntu-22.04 | ||
| runs-on: ubuntu-24.04 |
There was a problem hiding this comment.
Add explicit permissions block to workflow.
The workflow lacks a permissions block, which is a security best practice. GitHub Actions workflows default to broad permissions if not explicitly declared.
Apply this diff to add minimal read-only permissions:
jobs:
pipeline:
+ permissions:
+ contents: read
runs-on: ubuntu-24.04
timeout-minutes: 20📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| runs-on: ubuntu-24.04 | |
| jobs: | |
| pipeline: | |
| permissions: | |
| contents: read | |
| runs-on: ubuntu-24.04 | |
| timeout-minutes: 20 |
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 15-79: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
🤖 Prompt for AI Agents
.github/workflows/apps.yml around line 15: the workflow is missing an explicit
permissions block; add a top-level permissions block granting minimal read-only
access (for example at least contents: read and any other specific read
permissions your jobs need) to restrict default broad permissions, placing it at
the root of the workflow file just below name/on.
andrew-fleming
left a comment
andrew-fleming
left a comment
There was a problem hiding this comment.
Looking good @emnul! I left some comments :)
packages/compact/src/Builder.ts
Outdated
| /** | ||
| * Executes a single build step. | ||
| * Runs the command, shows a spinner, and prints output with indentation. | ||
| * | ||
| * @param step - The build step containing command and message | ||
| * @param index - Current step index (0-based) for progress display | ||
| * @param total - Total number of steps for progress display | ||
| * @returns A promise that resolves when the step completes successfully | ||
| * @throws Error if the step fails | ||
| */ | ||
| private async executeStep( | ||
| step: { cmd: string; msg: string; shell?: string }, | ||
| index: number, | ||
| total: number, | ||
| ): Promise<void> { | ||
| const stepLabel: string = `[${index + 1}/${total}]`; | ||
| const spinner: Ora = ora(`[BUILD] ${stepLabel} ${step.msg}`).start(); | ||
| /** | ||
| * Executes a single build step. | ||
| * Runs the command, shows a spinner, and prints output with indentation. | ||
| * | ||
| * @param step - The build step containing command and message | ||
| * @param index - Current step index (0-based) for progress display | ||
| * @param total - Total number of steps for progress display | ||
| * @returns A promise that resolves when the step completes successfully | ||
| * @throws Error if the step fails |
There was a problem hiding this comment.
Indents are going from 2 spaces to 4. Personally, 4 spaces creates too much empty space for my taste, that's why I like 2 spaces. If I'm outvoted and everyone prefers 4, let's create a separate PR focused just on that and not let it creep into a CI/deps PR
packages/logger/biome.json
Outdated
| @@ -1,4 +1,4 @@ | |||
| { | |||
| "root": false, | |||
There was a problem hiding this comment.
When I ran biome's migration feature to bump it, I got the same thing with all the "root": false. Correct me if I'm wrong, but I don't think we need it. That's what's proposed in this PR anyway #274
packages/biome-config/biome.json
Outdated
| "useIgnoreFile": true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
| } | |
| } | |
| }, | ||
| "engineStrict": true | ||
| } | ||
| } |
There was a problem hiding this comment.
| } | |
| } | |
package.json
Outdated
| "packageManager": "pnpm@10.4.1", | ||
| "devDependencies": { | ||
| "@biomejs/biome": "1.9.4", | ||
| "@biomejs/biome": "2.3.8", |
There was a problem hiding this comment.
| "@biomejs/biome": "2.3.8", | |
| "@biomejs/biome": "2.3.9", |
Released a couple days ago
contracts/shielded-token/biome.json
Outdated
| { | ||
| "$schema": "https://biomejs.dev/schemas/1.9.4/schema.json", | ||
| "root": false, | ||
| "$schema": "https://biomejs.dev/schemas/2.3.5/schema.json", |
There was a problem hiding this comment.
| "$schema": "https://biomejs.dev/schemas/2.3.5/schema.json", | |
| "$schema": "https://biomejs.dev/schemas/2.3.9/schema.json", |
or 8 if you don't want to use the latest
contracts/compact-std/package.json
Outdated
| "@midnight-ntwrk/zswap": "^4.0.0" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
| } | |
| } | |
| - name: Enable corepack | ||
| shell: bash | ||
| env: | ||
| COMPILER_VERSION: "0.26.0" | ||
| run: | | ||
| echo "COMPILER_VERSION=$COMPILER_VERSION" >> "$GITHUB_ENV" | ||
| run: corepack enable |
There was a problem hiding this comment.
We have pnpm/action-setup so AFAICT corepack is redundant and can be removed
| exit 1 | ||
| fi | ||
| done | ||
| node-version: "22.x" |
There was a problem hiding this comment.
| node-version: "22.x" | |
| node-version-file: ".nvmrc" |
| env: | ||
| TURBO_MAJOR_VERSION: 2 | ||
| TURBO_TELEMETRY_DISABLED: 1 | ||
| run: pnpm install turbo@${{ env.TURBO_MAJOR_VERSION }} -g |
There was a problem hiding this comment.
turbo is already listed as a dep in the manifest so I don't think we need to install it again
Signed-off-by: 0xisk <0xisk@proton.me> Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
This PR updates workflow dependencies, simplifies setup/action.yml, and refactors its dependent workflows
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.