Timelock build pipeline #1305
Timelock build pipeline #1305
Conversation
WalkthroughA new GitHub Actions workflow for Docker build and deployment is introduced. The pre-commit hook disables lint-staged checks. The Dockerfile for the timelock deployment script is replaced with a new Ubuntu-based Dockerfile, while the old Alpine-based Dockerfile is removed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if: github.ref != 'refs/heads/main' | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: checkout code | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: set up docker buildx | ||
| uses: docker/setup-buildx-action@v3 | ||
|
|
||
| - name: get short sha | ||
| id: vars | ||
| run: echo "sha_short=$(git rev-parse --short HEAD)" >> "$GITHUB_OUTPUT" | ||
|
|
||
| - name: build docker image (test only) | ||
| run: | | ||
| docker buildx build \ | ||
| --no-cache \ | ||
| --platform linux/arm64 \ | ||
| -f script/deploy/safe/ExecutePendingTimelock.Dockerfile \ | ||
| -t image-test:${{ steps.vars.outputs.sha_short }} . |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
To fix the issue, add a permissions block to the build-branch-dockerfile job. Since this job does not require elevated permissions, the permissions block should specify contents: read as the minimal privilege required. This ensures that the GITHUB_TOKEN is limited to read-only access to repository contents.
The changes will be made in the .github/workflows/build-and-push.yaml file, specifically within the build-branch-dockerfile job definition.
| @@ -84,2 +84,4 @@ | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| steps: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
script/deploy/safe/ExecutePendingTimelock.Dockerfile (3)
5-7: Copying the whole repo bloats the image and risks leaking build artefacts
COPY . .brings in.git, CI cache directories and other unneeded files. Add a.dockerignore(or refine an existing one) to exclude at least.git,node_modules,target,out, and any secret files.
8-22: Use--no-install-recommendsand consolidate RUN to slim the layer
APT pulls in many optional packages by default. You can cut ~100 MB with:-RUN apt-get update && apt-get install -y \ +RUN apt-get update && apt-get install -y --no-install-recommends \Also consider chaining the cleanup in the same RUN to keep the final layer small:
RUN apt-get update && \ apt-get install -y --no-install-recommends build-essential python3 nodejs npm \ libudev-dev libusb-1.0-0-dev curl jq sudo unzip git && \ rm -rf /var/lib/apt/lists/*
32-35: Combine the four RUN statements to leverage layer caching
Runningbun install,typechain, andforge installeach in its own layer increases image size and rebuild time. Merging them keeps intermediate files in a single layer and re-uses cache when only TS changes:RUN bun install --frozen-lockfile --production && \ bun run typechain && \ forge install.github/workflows/build-and-push.yaml (2)
1-3: Add a descriptive YAML header comment per repo conventions
conventions.mdrequires a short description at the top of every workflow. Consider:# Builds the timelock Docker image and pushes to ECR (main) or does a test build (branches) name: build & deploy
6-8:'*'already matchesmain; duplication may be unintentional
Listing bothmainand'*'underbranchesmakes the explicitmainentry redundant. If you intended to exclude tags, keep'*'but dropmain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build-and-push.yaml(1 hunks).husky/pre-commit(1 hunks)script/deploy/safe/ExecutePendingTimelock.Dockerfile(1 hunks)script/deploy/safe/ExecutePendingTimelockDockerfile(0 hunks)
💤 Files with no reviewable changes (1)
- script/deploy/safe/ExecutePendingTimelockDockerfile
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: 0xDEnYO
PR: lifinance/contracts#1256
File: deployments/zksync.diamond.json:81-87
Timestamp: 2025-07-04T08:59:08.108Z
Learning: When analyzing deployment PRs in the lifinance/contracts repository, carefully verify that target state configuration files (like script/deploy/_targetState.json) have been updated before flagging missing entries. The AI summary section should be consulted to understand all file changes, as manual searches might miss entries due to formatting differences or search limitations.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1168
File: script/deploy/_targetState.json:1564-1589
Timestamp: 2025-05-27T12:36:26.987Z
Learning: When reviewing deployment PRs in the lifinance/contracts repository, target state configuration files (like script/deploy/_targetState.json) may be updated for multiple networks even when the PR is focused on deploying to a specific network. The scope should be determined by the PR title and description, not just by all configuration changes present in the files.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:129-129
Timestamp: 2025-07-17T04:21:50.790Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers not to add private key format validation in deployment scripts like execute-pending-timelock-tx.ts, prioritizing code simplicity over strict validation in their controlled environment.
Learnt from: mirooon
PR: lifinance/contracts#1187
File: .github/workflows/versionControlAndAuditCheck.yml:596-612
Timestamp: 2025-06-05T11:25:43.443Z
Learning: The user mirooon has an internal system for tracking tickets and technical debt. They acknowledged the security concern about disabled commit hash verification in the audit workflow and plan to implement conditional logic to skip verification only for revert PRs in a future update.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:334-334
Timestamp: 2025-07-17T04:21:55.549Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep private key processing simple in scripts like execute-pending-timelock-tx.ts without adding format validation, prioritizing code simplicity over strict validation in controlled environments.
Learnt from: mirooon
PR: lifinance/contracts#1048
File: script/deploy/safe/safe-decode-utils.ts:0-0
Timestamp: 2025-03-21T13:48:54.006Z
Learning: When reviewing PRs with a specific focused purpose (like migrating between package managers), restrict suggestions to issues directly related to that purpose rather than general code improvements.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1237
File: conventions.md:56-60
Timestamp: 2025-07-03T01:44:43.968Z
Learning: Always consult the conventions.md file for the latest rules and conventions when reviewing PRs or code changes in the lifinance/contracts repository. Make suggestions when code changes do not match the documented conventions in this file.
Learnt from: 0xDEnYO
PR: lifinance/contracts#853
File: .github/workflows/protectSecurityRelevantCode.yml:67-100
Timestamp: 2024-11-08T03:12:44.309Z
Learning: In the `.github/workflows/protectSecurityRelevantCode.yml` file, suggested security improvements for configuring organization and team names as environment variables, handling API rate limits, addressing potential file permission issues, and mitigating command injection risks are not required.
Learnt from: 0xDEnYO
PR: lifinance/contracts#853
File: .github/workflows/protectSecurityRelevantCode.yml:78-78
Timestamp: 2024-11-08T03:42:16.803Z
Learning: In the `.github/workflows/protectSecurityRelevantCode.yml` file, ensure that any suggested code changes are secure, thoroughly vetted for potential issues, and do not prompt further modifications to previously recommended code.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to .github/workflows/*.yml : Begin GitHub workflow files with a clear description (YAML comments) and include descriptive comments throughout.
Learnt from: 0xDEnYO
PR: lifinance/contracts#902
File: .github/workflows/forceMergePRBypassAudit.yml:68-82
Timestamp: 2024-12-12T02:13:02.343Z
Learning: When implementing GitHub Actions workflows that allow bypassing audit requirements, such as the `Force-Merge PR (Bypass Audit Requirement)` workflow, include a step to send a message to the Discord server to inform the team when this action is executed.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to .github/workflows/*.yml : Set explicit permissions and include notifications in GitHub workflow files.
Learnt from: 0xDEnYO
PR: lifinance/contracts#879
File: .github/workflows/types.yaml:58-62
Timestamp: 2024-11-27T04:28:26.318Z
Learning: In the file `.github/workflows/types.yaml`, additional error handling for file operations is not required in the 'Copy Type Bindings and ABI' step.
Learnt from: 0xDEnYO
PR: lifinance/contracts#921
File: .github/workflows/ensureSCCoreDevApproval.yml:20-20
Timestamp: 2025-01-16T03:34:05.229Z
Learning: In GitHub Actions workflows for lifinance/contracts repository, it is acceptable to declare the CONTINUE environment variable at the workflow level (within individual steps) rather than at the job level.
.husky/pre-commit (4)
Learnt from: 0xDEnYO
PR: #807
File: deployments/bsc.staging.json:28-28
Timestamp: 2024-09-23T02:08:02.297Z
Learning: For staging files in deployments, avoid making comments regarding incomplete contract deployments or missing bytecode, as these comments are not necessary.
Learnt from: 0xDEnYO
PR: #807
File: deployments/bsc.staging.json:28-28
Timestamp: 2024-10-09T03:47:21.269Z
Learning: For staging files in deployments, avoid making comments regarding incomplete contract deployments or missing bytecode, as these comments are not necessary.
Learnt from: 0xDEnYO
PR: #1139
File: package.json:81-82
Timestamp: 2025-05-14T03:33:25.089Z
Learning: Bun allows direct invocation of npm scripts defined in package.json without requiring the bun run prefix (e.g., bun scriptName works, not just bun run scriptName), unlike some other package managers like npm.
Learnt from: 0xDEnYO
PR: #1232
File: script/deploy/_targetState.json:1680-1703
Timestamp: 2025-06-24T07:35:58.970Z
Learning: The user (0xDEnYO) has instructed to never review the target state file (script/deploy/_targetState.json). This file should be excluded from code review analysis.
script/deploy/safe/ExecutePendingTimelock.Dockerfile (10)
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to script/deploy/**/*.sh : Wrap Foundry's deployment functionality in Bash scripts for reliability and automation.
Learnt from: 0xDEnYO
PR: #1207
File: script/deploy/facets/DeployPioneerFacet.s.sol:17-27
Timestamp: 2025-06-13T08:30:28.665Z
Learning: Deployment scripts rely on external automation (pre-script checks) to validate constructor arguments such as non-zero addresses, so adding explicit require guards inside the Foundry deployment scripts is generally unnecessary.
Learnt from: 0xDEnYO
PR: #1256
File: deployments/zksync.diamond.json:81-87
Timestamp: 2025-07-04T08:59:08.108Z
Learning: When analyzing deployment PRs in the lifinance/contracts repository, carefully verify that target state configuration files (like script/deploy/_targetState.json) have been updated before flagging missing entries. The AI summary section should be consulted to understand all file changes, as manual searches might miss entries due to formatting differences or search limitations.
Learnt from: 0xDEnYO
PR: #1184
File: script/deploy/safe/deploy-safe.ts:0-0
Timestamp: 2025-07-03T07:34:47.349Z
Learning: In script/deploy/safe/deploy-safe.ts, 0xDEnYO intentionally changed the allowOverride flag default from false to true to allow overwriting existing Safe addresses by default in deployment workflows.
Learnt from: ezynda3
PR: #861
File: script/deploy/safe/add-owners-to-safe.ts:48-48
Timestamp: 2024-11-21T08:23:50.099Z
Learning: In the script script/deploy/safe/add-owners-to-safe.ts, additional defensive checks for network configuration may be unnecessary because the script will fail anyway when the network configuration is missing.
Learnt from: ezynda3
PR: #875
File: script/utils/fetch-missing-deployments.ts:46-46
Timestamp: 2024-12-02T08:19:07.783Z
Learning: In our codebase, scripts like script/utils/fetch-missing-deployments.ts are intended to run in Node.js version 18 or newer, so global fetch is available without importing.
Learnt from: ezynda3
PR: #924
File: script/deploy/zksync/DeployReceiverStargateV2.s.sol:19-21
Timestamp: 2025-01-28T14:27:50.689Z
Learning: In LiFi's deployment scripts, the deploy method in DeployScriptBase handles the concatenation of constructor arguments with the contract's creation code, so child contracts don't need to concatenate the arguments explicitly.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to {script/deploy/facets/.sol,script/deploy/zksync/.sol} : Deployment scripts must reside in script/deploy/facets/ for base deployments and script/deploy/zksync/ for ZKSync-specific scripts.
Learnt from: 0xDEnYO
PR: #807
File: script/deploy/facets/DeployGasZipPeriphery.s.sol:13-20
Timestamp: 2024-09-23T01:42:55.475Z
Learning: In deployment scripts, do not suggest adding error handling to catch and report deployment failures, as errors will be displayed by Foundry in the output.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to {script/deploy/facets/.sol,script/deploy/zksync/.sol} : Deployment scripts must inherit DeployScriptBase and use JSON config with stdJson.
.github/workflows/build-and-push.yaml (5)
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to .github/workflows/*.yml : Begin GitHub workflow files with a clear description (YAML comments) and include descriptive comments throughout.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to .github/workflows/*.yml : Set explicit permissions and include notifications in GitHub workflow files.
Learnt from: 0xDEnYO
PR: #853
File: .github/workflows/protectSecurityRelevantCode.yml:78-78
Timestamp: 2024-11-08T03:42:16.803Z
Learning: In the .github/workflows/protectSecurityRelevantCode.yml file, ensure that any suggested code changes are secure, thoroughly vetted for potential issues, and do not prompt further modifications to previously recommended code.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to .github/workflows/*.yml : Use GitHub Secrets for sensitive data in GitHub workflow files.
Learnt from: 0xDEnYO
PR: #853
File: .github/workflows/protectSecurityRelevantCode.yml:67-100
Timestamp: 2024-11-08T03:12:44.309Z
Learning: In the .github/workflows/protectSecurityRelevantCode.yml file, suggested security improvements for configuring organization and team names as environment variables, handling API rate limits, addressing potential file permission issues, and mitigating command injection risks are not required.
🪛 GitHub Check: CodeQL
.github/workflows/build-and-push.yaml
[warning] 83-102: 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). (4)
- GitHub Check: build-branch-dockerfile
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
- GitHub Check: build-branch-dockerfile
🔇 Additional comments (1)
.github/workflows/build-and-push.yaml (1)
50-56:UNIQ_IDgeneration: potential race across parallel jobs
If multiple pushes to main happen within the same minute,date +'%y%m%d'+ short SHA is fine, but if you rebased and force-pushed the same commit later the tag would collide. Consider including the full SHA or a timestamp with seconds to guarantee uniqueness.
| permissions: | ||
| id-token: write | ||
| contents: read | ||
| steps: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Second job misses explicit permissions block — tighten token scope
Static analysis flagged this. Add a minimal permissions block to build-branch-dockerfile for parity and least-privilege:
runs-on: ubuntu-latest
permissions:
contents: read🤖 Prompt for AI Agents
In .github/workflows/build-and-push.yaml around lines 30 to 33, the second job
named build-branch-dockerfile lacks an explicit permissions block, which reduces
security by granting broader token scopes than necessary. Add a minimal
permissions block under the build-branch-dockerfile job specifying "contents:
read" to restrict token permissions to the least privilege required, matching
the pattern used in the first job.
| echo "Running 'bun lint-staged' now:" | ||
| bun lint-staged | ||
| # bun lint-staged | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Disabling lint-staged removes an important safety net — reconsider or gate behind an env flag
Commenting out bun lint-staged turns off all staged-file linting/formatting checks, so style or even minor type issues will now reach CI instead of being caught pre-commit. If the motivation is performance or local tool availability, consider:
-# bun lint-staged
+if [ -z "$SKIP_LINT" ]; then
+ bun lint-staged
+fiDevelopers can then opt-out with SKIP_LINT=1 git commit …, keeping the default safe.
🏁 Script executed:
#!/bin/bash
# Search for any use of lint-staged in code, config, and scripts
rg -n "lint-?staged"
# Check for lint-staged configuration files
fd -HI ".lintstagedrc*"
fd -HI "lint-staged.config.js"
# Inspect the beginning of the pre-commit hook to confirm context
sed -n '1,30p' .husky/pre-commitLength of output: 2142
Re-enable lint-staged in pre-commit hook or gate behind an env flag
Commenting out bun lint-staged disables all staged-file lint/format checks. Since you already have a lint-staged config in package.json, it’s better to run it by default and allow opt-out when needed:
• File: .husky/pre-commit (around line 17)
• Update the snippet to:
- echo "Running 'bun lint-staged' now:"
- # bun lint-staged
+ if [ -z "$SKIP_LINT" ]; then
+ echo "Running 'bun lint-staged' now:"
+ bun lint-staged
+ fiDevelopers can skip the hook with:
SKIP_LINT=1 git commit …🤖 Prompt for AI Agents
In .husky/pre-commit around lines 15 to 17, the command `bun lint-staged` is
commented out, disabling lint and format checks on staged files. Re-enable this
command by uncommenting it and add a conditional check to skip running it if an
environment variable like SKIP_LINT is set. This allows lint-staged to run by
default while giving developers the option to bypass it by setting SKIP_LINT=1
before committing.
| # Install Bun | ||
| RUN curl -fsSL https://bun.sh/install | bash | ||
| ENV PATH="/root/.bun/bin:$PATH" | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Remote install script executed without checksum ⇒ supply version pin or signature check
Piping curl into bash from https://bun.sh/install gives the build root-level code execution with no integrity guarantee. At minimum pin to a specific release tag and verify checksum; ideally fetch the release tarball directly:
ARG BUN_VERSION=1.0.17
RUN curl -fsSL https://bun.sh/install | bash -s -- --version $BUN_VERSION --yes \
&& echo "$BUN_SHA256 /root/.bun/bin/bun" | sha256sum -c -Pin Bun installer and verify checksum in Dockerfile
Piping the installer script directly from bun.sh without version pinning or integrity checks exposes the build to supply-chain attacks. Please update the Dockerfile as follows:
• Define build arguments for the Bun version and its expected SHA-256 checksum
• Invoke the installer with the --version flag
• Verify the binary’s checksum immediately after installation
Example snippet:
# Install Bun (pinned version + checksum verification)
ARG BUN_VERSION=1.0.17
ARG BUN_SHA256=<insert-official-sha256-here>
RUN curl -fsSL https://bun.sh/install \
| bash -s -- --version "$BUN_VERSION" --yes \
&& echo "$BUN_SHA256 /root/.bun/bin/bun" | sha256sum -c -
ENV PATH="/root/.bun/bin:$PATH"– Replace <insert-official-sha256-here> with the checksum published by the Bun maintainers.
– This ensures you’re installing a known, unaltered release.
🤖 Prompt for AI Agents
In script/deploy/safe/ExecutePendingTimelock.Dockerfile around lines 23 to 26,
the Bun installer is run by piping the script directly from the URL without
version pinning or checksum verification, which risks supply-chain attacks. Fix
this by adding build arguments for the Bun version and its SHA-256 checksum,
modifying the install command to specify the version explicitly, and verifying
the installed binary's checksum immediately after installation. Replace the
placeholder checksum with the official one from Bun maintainers to ensure the
installed binary is authentic and unaltered.
| # Install Foundry | ||
| RUN curl -L https://foundry.paradigm.xyz | bash | ||
| ENV PATH="/root/.foundry/bin:$PATH" | ||
|
|
||
| RUN foundryup |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same integrity concern for Foundry install
For consistency and supply-chain safety, pin the Foundry commit and verify the binary checksum instead of pulling foundryup HEAD each build.
🤖 Prompt for AI Agents
In script/deploy/safe/ExecutePendingTimelock.Dockerfile around lines 27 to 31,
the Foundry installation pulls the latest version without pinning a specific
commit or verifying the binary checksum, which poses a supply-chain security
risk. Modify the Dockerfile to download a specific, fixed commit or release
version of Foundry and include a step to verify the checksum of the downloaded
binary before installation. This ensures consistent builds and improves security
by preventing unverified code execution.
Which Jira task belongs to this PR?
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)