chore: move from Makefile to mise #6368
Conversation
WalkthroughReplaces Makefile-based build/lint/test flows with a mise task runner: deletes Changes
Sequence Diagram(s)sequenceDiagram
actor Developer
participant GitHub as GitHub Actions
participant MiseAction as jdx/mise-action
participant Runner as Job Runner
participant Tools as Toolchain
Developer->>GitHub: push PR / commit
GitHub->>MiseAction: invoke mise-action step
MiseAction->>Runner: provision env & install mise
Runner->>Tools: run mise tasks (lint / test / install / codecov)
Tools-->>Runner: produce outputs/artifacts
Runner-->>GitHub: upload artifacts / report status
sequenceDiagram
participant Docker as Docker build
participant Script as scripts/install_mise.sh
participant GPG as Keyserver/GPG
participant Mise as mise installer
participant Tools as Toolchain
Docker->>Script: RUN ./scripts/install_mise.sh
Script->>GPG: import release key & verify signature
Script->>Mise: execute verified installer script
Mise-->>Docker: mise installed
Docker->>Mise: RUN mise trust && mise run install
Mise->>Tools: install project binaries
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
LesnyRumcajs
force-pushed
the
use-mise
branch
6 times, most recently
from
f11e87e to
983d358
Compare
LesnyRumcajs
changed the title
LesnyRumcajs
force-pushed
the
use-mise
branch
3 times, most recently
from
42d496b to
19b0dac
Compare
LesnyRumcajs
requested review from
akaladarshi and
hanabi1224
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/forest.yml (1)
58-60: Removesetup-gofrombuild-macos—it's unused.The
build-macosjob runs onlycargo checkand does not execute any Go code, makingactions/setup-go@v6unnecessary. Thecalibnet-checkjob legitimately requires Go for running./f3-sidecartests (as seen incalibnet_other_check.sh), but consider migrating both jobs tojdx/mise-action@v3for consistency across the CI pipeline, which already uses mise elsewhere for tool management on all platforms.
🤖 Fix all issues with AI agents
In @scripts/install_mise.sh:
- Around line 10-12: After decrypting into install-mise.sh, check that the GPG
command succeeded and the output is valid before running it: verify gpg exit
status, ensure install-mise.sh is non-empty and that its first bytes start with
a shebang ("#!") to confirm it's a shell script, and abort with an error message
(do not run sh ./install-mise.sh) if any check fails; apply these checks around
the existing curl | gpg --decrypt > install-mise.sh and the subsequent sh
./install-mise.sh invocation.
🧹 Nitpick comments (7)
mise-tasks/lint/lists.sh (1)
12-19: Fix indentation inconsistency.The loop logic is correct, but the indentation is inconsistent. Line 16 (
else) has extra leading spaces, and line 19 (done) should be aligned with theforstatement.🎨 Proposed fix for indentation
for FILE in "${LISTS[@]}"; do if [[ -f "$FILE" ]]; then sort --unique -o "$FILE" "$FILE" echo "Sorted $FILE" - else + else echo "File $FILE does not exist." fi - done +doneDockerfile (1)
26-26: Consider verifying mise installation before proceeding.While the installation approach is sound, adding a quick verification step would make build failures clearer if the installation script encounters issues.
♻️ Optional verification step
RUN ./scripts/install_mise.sh +RUN mise --versionThis ensures mise is correctly installed and on the PATH before proceeding to the main build steps.
mise.toml (5)
10-17: Uninitialized variable may cause issues withset -u.If the
--slimflag is not provided,SLIM_FLAGSis never set, which could cause an "unbound variable" error ifset -uis enabled elsewhere. Initialize the variable before the conditional.♻️ Suggested fix
run = ''' echo "Installing Forest binaries with profile: ${usage_profile?}" [[ -n "${usage_verbose}" ]] && set -x +SLIM_FLAGS="" if [ "${usage_slim}" = true ]; then SLIM_FLAGS="--no-default-features --features slim" fi cargo install --profile ${usage_profile} ${SLIM_FLAGS} --locked --path . --force '''
106-112: Redundantyarninvocation.
yarnwithout arguments is equivalent toyarn install, soyarn && yarn installis redundant.♻️ Suggested simplification
[tasks."lint:yaml"] description = "Lint YAML files." run = ''' -corepack enable && yarn && yarn install +corepack enable && yarn install yarn yaml-fmt yarn yaml-check '''
114-122: Same redundantyarninvocation as in lint:yaml.♻️ Suggested simplification
[tasks.fmt] description = "Format all supported code." run = ''' cargo fmt --all taplo fmt -corepack enable && yarn && yarn install +corepack enable && yarn install yarn md-fmt yarn yaml-fmt '''
20-23: Consider addingtaplo-cliversion pin for reproducibility.The lint tools are installed with
cargo binstallbut without version pins. For CI reproducibility, consider pinning versions (at least for critical tools).
156-166: Refactor to usedependsfor consistency with other composite tasks.The
testtask can be refactored to declare dependencies viadependsinstead of nestedmise task runcalls, matching the approach used in tasks likelint. Mise'sdependsfeature supports passing arguments (e.g.,depends = ["test:rust ${usage_profile?}", "test:docs ${usage_profile?}"]), so this refactoring is cleanly achievable. For better maintainability, consider declaring the profile argument using task argument templating (e.g.,{{arg(name="profile")}}).
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.dockerignore.github/workflows/butterflynet.yml.github/workflows/cargo-advisories.yml.github/workflows/coverage.yml.github/workflows/docker.yml.github/workflows/forest.yml.github/workflows/go-lint.yml.github/workflows/rust-lint.yml.github/workflows/scripts-lint.yml.github/workflows/unit-tests.ymlCHANGELOG.mdCONTRIBUTING.mdDockerfileMakefileREADME.mdbuild/vendored-docs-redirect.index.htmldocs/dictionary.txtdocs/docs/users/getting_started/install.mdmise-tasks/add_license.shmise-tasks/lint/lists.shmise.tomlscripts/install_mise.shscripts/linters/find_unused_deps.rb
💤 Files with no reviewable changes (1)
- Makefile
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: LesnyRumcajs
Repo: ChainSafe/forest PR: 5907
File: src/rpc/methods/state.rs:523-570
Timestamp: 2025-08-06T15:44:33.467Z
Learning: LesnyRumcajs prefers to rely on BufWriter's Drop implementation for automatic flushing rather than explicit flush() calls in Forest codebase.
📚 Learning: 2025-12-18T08:12:03.919Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6362
File: f3-sidecar/manifest.go:14-17
Timestamp: 2025-12-18T08:12:03.919Z
Learning: In the Forest codebase, F3.GetRawNetworkName queries the network name from genesis and returns "testnetnet" on mainnet, not "mainnet". This is different from StateNetworkName which returns "mainnet". The raw network name "testnetnet" is the correct key for manifest lookups in f3-sidecar when using RawNetwork2PredefinedManifestMappings.
Applied to files:
docs/dictionary.txt
📚 Learning: 2025-08-07T13:12:23.359Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5886
File: Makefile:1-1
Timestamp: 2025-08-07T13:12:23.359Z
Learning: golangci-lint version v2.3.1 exists and is a valid release that can be used in Makefiles and CI configurations. It was released after v2.3.0.
Applied to files:
.github/workflows/go-lint.yml
📚 Learning: 2025-08-07T13:12:23.359Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5886
File: Makefile:1-1
Timestamp: 2025-08-07T13:12:23.359Z
Learning: golangci-lint version v2.3.1 exists and was released on August 2, 2025. It is currently the latest stable version and can be used in Makefiles and CI configurations. The version follows the v2.x.x numbering scheme, not v1.x.x as I previously incorrectly stated.
Applied to files:
.github/workflows/go-lint.yml
📚 Learning: 2025-10-17T09:36:15.757Z
Learnt from: elmattic
Repo: ChainSafe/forest PR: 6128
File: src/ipld/util.rs:23-30
Timestamp: 2025-10-17T09:36:15.757Z
Learning: Always run `cargo check` or `cargo build` to verify actual compilation errors in the Forest codebase before flagging them as issues. Do not rely solely on documentation or assumptions about trait implementations.
Applied to files:
CONTRIBUTING.md.github/workflows/unit-tests.ymlDockerfileREADME.md.github/workflows/forest.yml
📚 Learning: 2025-08-08T12:10:45.218Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5867
File: src/ipld/util.rs:553-558
Timestamp: 2025-08-08T12:10:45.218Z
Learning: Forest project targets Rust stable >=1.89; features stabilized in 1.88 like let-chains are acceptable in this codebase.
Applied to files:
.github/workflows/unit-tests.yml
📚 Learning: 2025-08-07T13:39:15.107Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5886
File: interop-tests/src/tests/go_app/gen.go:29-29
Timestamp: 2025-08-07T13:39:15.107Z
Learning: Auto-generated files like those created by rust2go (indicated by "Generated by rust2go. Please DO NOT edit this C part manually." comment) in the Forest project should be skipped during code review as they are not intended for manual editing.
Applied to files:
.github/workflows/forest.yml
📚 Learning: 2025-08-07T13:39:29.732Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5886
File: interop-tests/src/tests/go_app/gen.go:58-58
Timestamp: 2025-08-07T13:39:29.732Z
Learning: In the Forest project, auto-generated files (like those generated by rust2go tool) should be skipped during code review as they are not meant to be manually edited.
Applied to files:
.github/workflows/forest.yml
🧬 Code graph analysis (1)
scripts/linters/find_unused_deps.rb (1)
src/cli_shared/cli/mod.rs (1)
file(296-296)
🪛 Gitleaks (8.30.0)
scripts/install_mise.sh
[high] 5-5: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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). (8)
- GitHub Check: tests-release
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: Coverage
- GitHub Check: Build Ubuntu
- GitHub Check: Build MacOS
- GitHub Check: cargo-publish-dry-run
- GitHub Check: All lint checks
- GitHub Check: rubocop
🔇 Additional comments (37)
docs/dictionary.txt (1)
79-79: LGTM!The dictionary entry is correctly alphabetized and aligns with the PR's migration to mise tooling.
mise-tasks/add_license.sh (1)
2-3: LGTM!The MISE metadata annotation format is correct and the description accurately describes the script's purpose.
scripts/linters/find_unused_deps.rb (1)
19-19: Good refinement!Narrowing the glob pattern to specifically target
Cargo.tomlfiles is more precise and prevents false positives from other TOML files (such as the newly introducedmise.toml).mise-tasks/lint/lists.sh (2)
1-3: LGTM!The shebang and MISE metadata annotation are correctly formatted.
4-10: LGTM!The array definition is correct and exporting
LC_ALL=Censures consistent, locale-independent sorting behavior.build/vendored-docs-redirect.index.html (1)
1-9: LGTM!The formatting changes improve readability and use the standard lowercase HTML5 doctype declaration. The redirect functionality remains unchanged.
scripts/install_mise.sh (1)
5-5: Clarification: The static analysis alert is a false positive.The Gitleaks scanner flagged this line as a potential API key, but
GPG_KEYcontains a public GPG key fingerprint (for the mise release key), not a secret. This is safe to include in the codebase and is necessary for signature verification.CHANGELOG.md (1)
42-43: LGTM! Clear and informative changelog entry.The changelog entry accurately describes the migration and provides actionable guidance for contributors. The format follows the project's conventions.
Dockerfile (1)
32-33: LGTM! Correct migration to mise-based installation.The
mise trustcommand is necessary to trust the mise.toml configuration, and the cache mounts are appropriately preserved. The migration maintains the same logical flow as the previous make-based installation..github/workflows/coverage.yml (2)
70-70: LGTM! Command migration aligns with the overall change.The transition from
make codecovtomise codecovis consistent with the broader migration strategy.
62-62: Migration to mise-action@v3 is correct, but review comment contained inaccurate assumptions.The mise-action@v3 step correctly installs the toolchains defined in
mise.toml[tools]: go, yarn, and cargo-binstall. However:
- Rust is not configured in mise.toml [tools] (it must be pre-installed in the GitHub Actions environment)
- cargo-llvm-cov and nextest are intentionally installed separately via
taiki-e/install-action(not via mise.toml), which is appropriate- The
mise codecovtask on line 70 correctly invokes the codecov task defined in mise.tomlThe workflow setup is valid; no changes needed.
Likely an incorrect or invalid review comment.
CONTRIBUTING.md (1)
190-190: LGTM! Documentation correctly updated for the tool migration.All three command updates (install-lint-tools, test, and fmt) accurately reflect the migration from Makefile-based commands to mise equivalents. These changes will help contributors use the new tooling correctly.
Also applies to: 201-201, 244-244
.github/workflows/go-lint.yml (2)
18-18: Good addition of mise.toml to path triggers.Including
mise.tomlin the workflow path triggers ensures the linting workflow runs when the Mise configuration changes. This is the correct approach.Also applies to: 26-26
36-37: Themise lint:golangtask is properly configured.The
lint:golangtask in mise.toml (lines 83-88) correctly installs golangci-lint and runs linting on the specified Go directories (./f3-sidecarand./interop-tests/src/tests/go_app). The.golangci.ymlconfiguration file is present and properly defines linting rules. Mise will handle tool installation and task execution as expected..github/workflows/docker.yml (2)
105-109: Verify thatmise run install releasematches the previousmake installbehavior.The command changed from
make installtomise run install releasein the ARM64 build job. The addition of thereleaseargument may change the build profile or optimization level compared to the previous Makefile target. This should behave identically to the AMD64 job changes.
62-67: No action needed. Themise run install releasecommand is correctly specified.The mise.toml configuration confirms that "release" is the default profile for the
installtask (default="release"). Explicitly specifyingreleasein the docker.yml workflow matches the intended build behavior and is consistent with the optimization flags set viaRUSTFLAGS. This explicit specification improves clarity in the CI workflow..github/workflows/butterflynet.yml (2)
32-32: Command is properly defined in mise.toml.The
mise run install --slimcommand is explicitly configured in mise.toml with clear documentation. The--slimflag sets--no-default-features --features slimduring cargo installation. The old Makefile target no longer exists, so this is the correct approach. No action needed.
27-27: No issues identified. Thejdx/mise-action@v3version is stable and documented in the official repository..github/workflows/cargo-advisories.yml (1)
11-11: Remove the suggestion to reconsider mise-action—it is necessary for this workflow.The
jdx/mise-action@v3step is required: it installscargo-binstall(configured as a tool in mise.toml), which is immediately used on the next line to installcargo-deny. Without this step, the workflow would fail becausecargo binstallwould not be available..github/workflows/unit-tests.yml (2)
82-82: Themise test releasecommand correctly executes both test suites.The command runs both Rust tests and documentation tests via the
testtask in mise.toml, which callstest:rustandtest:docssubtasks with the release profile. Both test suites are properly executed.
80-80: jdx/mise-action@v3 is valid and stable, with v3.5.1 being the latest release in the v3 series. No changes needed..github/workflows/rust-lint.yml (2)
66-67: LGTM! Clean migration to mise-based linting.The lint workflow now uses mise tasks (
install-lint-toolsandlint:all-rust), which provides better tooling consistency across the project.
53-53: The mise-action integration is correctly configured.The
jdx/mise-action@v3setup at line 53 is properly integrated. Themise.tomlfile exists and contains all required tasks:install-lint-tools(which installs cargo-binstall, taplo-cli, cargo-spellcheck, and cargo-deny) andlint:all-rust(which runs deny, spellcheck, rust-fmt, clippy, and unused-deps checks). The workflow correctly calls both tasks at lines 66-67..dockerignore (1)
8-8: LGTM! Correctly includes the mise install script.The negated ignore rule ensures
scripts/install_mise.shis available in the Docker build context, which is necessary for the mise-based installation flow referenced in the Dockerfile..github/workflows/forest.yml (3)
109-109: LGTM! Consistent mise-action usage.
156-158: Verify the Go setup approach in calibnet-check.The
calibnet-checkjob still usesactions/setup-go@v6while other jobs have migrated tojdx/mise-action@v3. Verify if this is intentional or if it should also migrate for consistency.
85-93: Clean migration to mise-based installation.The replacement of
make install-slim-quickwithmise run install --slimis properly supported. Theinstalltask in mise.toml explicitly defines the--slimflag, which correctly configures the slim build by passing--no-default-features --features slimto cargo install. The removal of the apt dependencies step suggests these are now handled by mise or are no longer required.docs/docs/users/getting_started/install.md (1)
102-106: LGTM! Clear documentation update.The documentation correctly reflects the migration to mise-en-place, providing users with a clear reference link and the updated installation command. The change is straightforward and well-documented.
README.md (3)
70-72: LGTM! Clear dependency documentation.The addition of mise-en-place to the dependencies section is well-placed and provides users with the necessary context and reference link.
93-93: LGTM! Installation command updated correctly.The migration from
make installtomise installis straightforward and maintains the same installation behavior.
184-185: LGTM! Simplified linting workflow.The migration to mise-based linting significantly simplifies the developer experience by consolidating tool installation and execution into two clear commands. This is a substantial improvement over managing individual linters.
.github/workflows/scripts-lint.yml (4)
18-24: LGTM - shellcheck job migrated to mise.The job correctly uses
jdx/mise-action@v3to set up mise and then invokes the lint task. The runner upgrade toubuntu-24.04-armis consistent across jobs.
26-31: LGTM - rubocop job simplified with mise.The job now relies on mise to handle Ruby tooling installation and execution, reducing boilerplate.
68-73: LGTM - yaml-lint job migrated to mise.Clean migration using the mise task runner.
74-82: LGTM - lists-lint job migrated correctly.The
git diff --exit-codecheck after the lint task ensures sorted lists are committed.mise.toml (2)
50-59: LGTM - Comprehensive clippy coverage.The clippy task covers multiple feature configurations including slim, no-default-features, benchmarks, and docs.rs builds. This ensures thorough lint coverage across all build variants.
174-177: Verifylatestversion pins for production CI stability.Using
latestfor tools likegoandyarnmay cause unexpected breakages if a new version introduces incompatibilities. Consider pinning to specific major versions for stability.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @scripts/install_mise.sh:
- Line 10: Update the curl invocation that downloads the signature in the
scripts/install_mise.sh script to fail on HTTP errors and surface errors
cleanly: modify the curl call (the line starting with "curl
https://mise.jdx.dev/install.sh.sig | gpg --decrypt > install-mise.sh") to
include --fail and add --silent --show-error (or at least --fail --show-error)
so HTTP error responses aren't piped into gpg and error output is clearer.
- Around line 5-9: The gpg command hardcodes the fingerprint instead of using
the GPG_KEY variable; replace the literal fingerprint in the gpg --recv-keys
invocation with the variable (e.g., use "$GPG_KEY" or "${GPG_KEY}") so the
script uses the exported GPG_KEY value, and ensure it is quoted to avoid
word-splitting.
🧹 Nitpick comments (1)
mise.toml (1)
174-177: Consider pinning tool versions for reproducibility.The tools are configured with
"latest"which can lead to non-reproducible builds and potential breakage from version changes. Consider pinning to specific versions (e.g.,go = "1.25",yarn = "4.x") to ensure consistent behavior across environments.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.dockerignoreCHANGELOG.mdDockerfilebuild/vendored-docs-redirect.index.htmlmise.tomlscripts/install_mise.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- build/vendored-docs-redirect.index.html
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: LesnyRumcajs
Repo: ChainSafe/forest PR: 5907
File: src/rpc/methods/state.rs:523-570
Timestamp: 2025-08-06T15:44:33.467Z
Learning: LesnyRumcajs prefers to rely on BufWriter's Drop implementation for automatic flushing rather than explicit flush() calls in Forest codebase.
📚 Learning: 2025-08-07T13:39:15.107Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5886
File: interop-tests/src/tests/go_app/gen.go:29-29
Timestamp: 2025-08-07T13:39:15.107Z
Learning: Auto-generated files like those created by rust2go (indicated by "Generated by rust2go. Please DO NOT edit this C part manually." comment) in the Forest project should be skipped during code review as they are not intended for manual editing.
Applied to files:
mise.toml
📚 Learning: 2025-08-07T13:39:29.732Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5886
File: interop-tests/src/tests/go_app/gen.go:58-58
Timestamp: 2025-08-07T13:39:29.732Z
Learning: In the Forest project, auto-generated files (like those generated by rust2go tool) should be skipped during code review as they are not meant to be manually edited.
Applied to files:
mise.toml
📚 Learning: 2025-10-17T09:36:15.757Z
Learnt from: elmattic
Repo: ChainSafe/forest PR: 6128
File: src/ipld/util.rs:23-30
Timestamp: 2025-10-17T09:36:15.757Z
Learning: Always run `cargo check` or `cargo build` to verify actual compilation errors in the Forest codebase before flagging them as issues. Do not rely solely on documentation or assumptions about trait implementations.
Applied to files:
Dockerfile
⏰ 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). (10)
- GitHub Check: Coverage
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: Check
- GitHub Check: Deploy to Cloudflare Pages
- GitHub Check: rubocop
- GitHub Check: All lint checks
- GitHub Check: tests-release
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: Build Ubuntu
🔇 Additional comments (1)
scripts/install_mise.sh (1)
5-12: The original review comment incorrectly questions whetherMISE_VERSIONandMISE_INSTALL_PATHare used by the installer. Both environment variables are officially supported by the mise installer script and are used during installation:
MISE_VERSIONpins which mise release to installMISE_INSTALL_PATHsets where the mise binary will be writtenThese variables are correctly exported in the script and should not be removed.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @mise.toml:
- Around line 10-17: Initialize SLIM_FLAGS to an empty string before the
conditional and quote variable expansions in the run script: set SLIM_FLAGS=""
before testing usage_slim, keep the conditional that sets SLIM_FLAGS when
usage_slim is true, and change unquoted ${usage_profile} and ${SLIM_FLAGS} to
quoted expansions so the cargo install invocation uses "${usage_profile}" and
"${SLIM_FLAGS}" to avoid unbound-variable and word-splitting issues.
- Around line 174-177: The tools table currently pins all tool versions to
"latest", which makes builds non-reproducible; update the [tools] section by
replacing the "latest" values for the keys cargo-binstall, go, and yarn with
fixed version strings (e.g., cargo-binstall -> "1.16.6", go -> "1.25.5", yarn ->
"4.12.0") or alternatively generate and commit a mise.lock to lock resolved
versions so CI and developer environments use identical tool versions.
🧹 Nitpick comments (4)
mise.toml (4)
61-67: Consider optimizing gem installation.The task installs the
toml-rbgem on every invocation, which is inefficient. Consider checking if the gem is already installed or documenting that users should install it separately.♻️ Proposed optimization
run = ''' -gem install --no-document toml-rb +gem list -i toml-rb >/dev/null 2>&1 || gem install --no-document toml-rb ruby scripts/linters/find_unused_deps.rb '''
102-104: Consider includinglint:tomlin the Rust linting tasks.Since TOML files (like
Cargo.toml) are integral to Rust projects, you may want to includelint:tomlin thelint:all-rusttask for completeness.♻️ Proposed enhancement
[tasks."lint:all-rust"] description = "Run all Rust linting tasks." -depends = ["lint:deny", "lint:spellcheck", "lint:rust-fmt", "lint:clippy", "lint:unused-deps"] +depends = ["lint:deny", "lint:spellcheck", "lint:toml", "lint:rust-fmt", "lint:clippy", "lint:unused-deps"]
106-122: Simplify yarn initialization.Both tasks run
yarn && yarn install, which is redundant sinceyarn(with no arguments) already performsyarn install. This can be simplified to justyarn install.♻️ Proposed simplification
For
lint:yaml:run = ''' -corepack enable && yarn && yarn install +corepack enable && yarn install yarn yaml-fmt yarn yaml-check '''For
fmt:run = ''' cargo fmt --all taplo fmt -corepack enable && yarn && yarn install +corepack enable && yarn install yarn md-fmt yarn yaml-fmt '''
156-166: Consider using mise's task orchestration syntax instead of shell invocations.While mise supports calling
mise task runfrom within tasks, the idiomatic approach is to use inline task objects. The code can be refactored to:run = [ { task = "test:rust", args = ["${usage_profile?}"] }, { task = "test:docs", args = ["${usage_profile?}"] } ]This is more aligned with mise's task orchestration patterns and provides better clarity around task dependencies and parameter passing.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mise.toml
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: LesnyRumcajs
Repo: ChainSafe/forest PR: 5907
File: src/rpc/methods/state.rs:523-570
Timestamp: 2025-08-06T15:44:33.467Z
Learning: LesnyRumcajs prefers to rely on BufWriter's Drop implementation for automatic flushing rather than explicit flush() calls in Forest codebase.
📚 Learning: 2025-08-07T13:39:15.107Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5886
File: interop-tests/src/tests/go_app/gen.go:29-29
Timestamp: 2025-08-07T13:39:15.107Z
Learning: Auto-generated files like those created by rust2go (indicated by "Generated by rust2go. Please DO NOT edit this C part manually." comment) in the Forest project should be skipped during code review as they are not intended for manual editing.
Applied to files:
mise.toml
📚 Learning: 2025-08-07T13:39:29.732Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5886
File: interop-tests/src/tests/go_app/gen.go:58-58
Timestamp: 2025-08-07T13:39:29.732Z
Learning: In the Forest project, auto-generated files (like those generated by rust2go tool) should be skipped during code review as they are not meant to be manually edited.
Applied to files:
mise.toml
⏰ 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). (8)
- GitHub Check: Build Ubuntu
- GitHub Check: Build MacOS
- GitHub Check: cargo-publish-dry-run
- GitHub Check: tests-release
- GitHub Check: All lint checks
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: rubocop
- GitHub Check: Coverage
🔇 Additional comments (6)
mise.toml (6)
20-23: LGTM!The task correctly installs the required linting tools using cargo-binstall.
50-59: LGTM!The task comprehensively checks Clippy across different feature combinations and the docs.rs build configuration.
69-96: LGTM!All specialized linting tasks are properly configured with their respective tools and targets.
124-129: LGTM!The cleanup task safely removes build artifacts and dependencies.
168-172: LGTM!The code coverage task is correctly configured.
37-42: Verify error propagation in mise task run blocks.The task runs two
taplocommands sequentially without explicit error handling. Confirm whether mise's run block execution automatically fails on the first command's error (e.g., iftaplo fmt --checkfails) or if both commands execute regardless. If explicit error handling is needed, use the&&operator.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
mise.toml (4)
10-17: InitializeSLIM_FLAGSto prevent potential shell errors.The
SLIM_FLAGSvariable is only set whenusage_slimis true, but it's always referenced on line 16. While bash tolerates unset variables in unquoted expansions, this is fragile and could fail withset -uor in stricter shell environments.🛡️ Defensive coding improvement
run = ''' echo "Installing Forest binaries with profile: ${usage_profile?}" [[ -n "${usage_verbose}" ]] && set -x +SLIM_FLAGS="" if [ "${usage_slim}" = true ]; then SLIM_FLAGS="--no-default-features --features slim" fi cargo install --profile ${usage_profile} ${SLIM_FLAGS} --locked --path . --force '''
106-112: Remove redundantyarncommand.Line 109 runs
yarn && yarn install, but in Yarn v2+ (Berry),yarnwithout arguments is equivalent toyarn install. The first invocation is redundant.🔧 Simplify command
run = ''' -corepack enable && yarn && yarn install +corepack enable && yarn install yarn yaml-fmt yarn yaml-check '''
114-122: Remove redundantyarncommand.Line 119 has the same redundancy as
tasks."lint:yaml"- runningyarn && yarn installwhenyarn installalone is sufficient in Yarn v2+.🔧 Simplify command
run = ''' cargo fmt --all taplo fmt -corepack enable && yarn && yarn install +corepack enable && yarn install yarn md-fmt yarn yaml-fmt '''
174-177: Pin tool versions for reproducible builds.All tools are set to
"latest", which can cause CI builds to break unexpectedly when tools update and may introduce inconsistencies across different build environments. For production CI/CD pipelines, pinning to specific versions ensures deterministic, reproducible builds.📌 Pin tool versions
Consider pinning to specific versions:
[tools] -cargo-binstall = "latest" -go = "latest" -yarn = "latest" +cargo-binstall = "1.10.15" # or your preferred version +go = "1.23.4" # or your preferred version +yarn = "4.5.3" # or your preferred versionAfter determining appropriate versions, you can periodically update them in a controlled manner through dependency update PRs.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mise.toml
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: LesnyRumcajs
Repo: ChainSafe/forest PR: 5907
File: src/rpc/methods/state.rs:523-570
Timestamp: 2025-08-06T15:44:33.467Z
Learning: LesnyRumcajs prefers to rely on BufWriter's Drop implementation for automatic flushing rather than explicit flush() calls in Forest codebase.
📚 Learning: 2025-08-07T13:39:15.107Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5886
File: interop-tests/src/tests/go_app/gen.go:29-29
Timestamp: 2025-08-07T13:39:15.107Z
Learning: Auto-generated files like those created by rust2go (indicated by "Generated by rust2go. Please DO NOT edit this C part manually." comment) in the Forest project should be skipped during code review as they are not intended for manual editing.
Applied to files:
mise.toml
📚 Learning: 2025-08-07T13:39:29.732Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5886
File: interop-tests/src/tests/go_app/gen.go:58-58
Timestamp: 2025-08-07T13:39:29.732Z
Learning: In the Forest project, auto-generated files (like those generated by rust2go tool) should be skipped during code review as they are not meant to be manually edited.
Applied to files:
mise.toml
📚 Learning: 2025-08-08T12:10:45.218Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5867
File: src/ipld/util.rs:553-558
Timestamp: 2025-08-08T12:10:45.218Z
Learning: Forest pins Rust toolchain to 1.89.0 via rust-toolchain.toml; features stabilized in 1.88 (e.g., let-chains) are acceptable in this codebase.
Applied to files:
mise.toml
⏰ 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). (11)
- GitHub Check: Build Ubuntu
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests-release
- GitHub Check: Coverage
- GitHub Check: All lint checks
- GitHub Check: Go lint checks
- GitHub Check: rubocop
- GitHub Check: Check
- GitHub Check: Deploy to Cloudflare Pages
🔇 Additional comments (1)
mise.toml (1)
168-172: Verify codecov scope is intentional.The codecov task only generates coverage for the
forest-filecoinpackage. The workspace also containsforest-interop-tests(theinterop-testsmember), which is not included in the coverage report. Confirm whether coverage forforest-interop-testsshould be added or if excluding it is intentional.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @mise.toml:
- Around line 76-81: The shellcheck task's run script uses the **/*.sh glob
which requires Bash's globstar option; update the tasks."lint:shellcheck" run
command to enable globstar before invoking shellcheck (e.g., set shopt -s
globstar or run bash with -O globstar) so nested scripts are matched, or
alternatively replace the glob with a find/xargs pipeline that locates all *.sh
under SCRIPTDIR and feeds them to shellcheck; ensure you reference the existing
run string and SCRIPTDIR symbol when making the change.
🧹 Nitpick comments (3)
mise.toml (3)
106-112: Remove redundant yarn command.Line 109 runs
yarn && yarn install, butyarnwithout arguments defaults toyarn install, making the second call redundant.♻️ Proposed refactor
[tasks."lint:yaml"] description = "Lint YAML files." run = ''' -corepack enable && yarn && yarn install +corepack enable && yarn install yarn yaml-fmt yarn yaml-check '''
114-122: Remove redundant yarn command.Same redundancy as in
lint:yaml:yarn && yarn installon line 119 can be simplified to justyarn install.♻️ Proposed refactor
[tasks.fmt] description = "Format all supported code." run = ''' cargo fmt --all taplo fmt -corepack enable && yarn && yarn install +corepack enable && yarn install yarn md-fmt yarn yaml-fmt '''
61-67: Consider caching gem installation for efficiency.The task installs
toml-rbgem on every run, which is inefficient but functional. Consider documenting that users should pre-install the gem, or check if it's already installed before attempting installation.♻️ Proposed optimization
[tasks."lint:unused-deps"] description = "Check for unused dependencies in Rust code." tools.ruby = "latest" run = ''' -gem install --no-document toml-rb +gem list toml-rb -i || gem install --no-document toml-rb ruby scripts/linters/find_unused_deps.rb '''
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mise.toml
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: LesnyRumcajs
Repo: ChainSafe/forest PR: 5907
File: src/rpc/methods/state.rs:523-570
Timestamp: 2025-08-06T15:44:33.467Z
Learning: LesnyRumcajs prefers to rely on BufWriter's Drop implementation for automatic flushing rather than explicit flush() calls in Forest codebase.
📚 Learning: 2025-08-07T13:39:15.107Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5886
File: interop-tests/src/tests/go_app/gen.go:29-29
Timestamp: 2025-08-07T13:39:15.107Z
Learning: Auto-generated files like those created by rust2go (indicated by "Generated by rust2go. Please DO NOT edit this C part manually." comment) in the Forest project should be skipped during code review as they are not intended for manual editing.
Applied to files:
mise.toml
📚 Learning: 2025-08-07T13:39:29.732Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5886
File: interop-tests/src/tests/go_app/gen.go:58-58
Timestamp: 2025-08-07T13:39:29.732Z
Learning: In the Forest project, auto-generated files (like those generated by rust2go tool) should be skipped during code review as they are not meant to be manually edited.
Applied to files:
mise.toml
📚 Learning: 2025-08-08T12:10:45.218Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5867
File: src/ipld/util.rs:553-558
Timestamp: 2025-08-08T12:10:45.218Z
Learning: Forest pins Rust toolchain to 1.89.0 via rust-toolchain.toml; features stabilized in 1.88 (e.g., let-chains) are acceptable in this codebase.
Applied to files:
mise.toml
⏰ 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). (10)
- GitHub Check: Build MacOS
- GitHub Check: Build Ubuntu
- GitHub Check: cargo-publish-dry-run
- GitHub Check: tests-release
- GitHub Check: Coverage
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: All lint checks
- GitHub Check: rubocop
- GitHub Check: Check
- GitHub Check: Deploy to Cloudflare Pages
🔇 Additional comments (4)
mise.toml (4)
131-166: LGTM!The test task structure is well-organized with proper profile argument handling and task composition. The separation of doctests and regular tests via nextest is appropriate given nextest's limitation with doctests.
13-13: No issue found. The comparison[ "${usage_slim}" = true ]is correct—mise passes boolean flags as the literal strings"true"or"false"to task environment variables, so the check works as intended.
168-172: No action required — the codecov command is already proven to work.This task duplicates the existing Makefile target
codecov, which uses the identical command and is already running successfully in the CI pipeline (.github/workflows/coverage.yml). Bothcargo-llvm-covavailability and the-p forest-filecoinpackage scope are already validated by the active CI workflow. If adding this task tomise.tomlfor convenience, ensure it remains synchronized with the Makefile version.Likely an incorrect or invalid review comment.
174-177: All tool versions are valid. The Go version 1.25.5, Yarn 4.12, and cargo-binstall 1.16.6 are all available releases as of January 2026. No version corrections needed.
| [tasks."lint:shellcheck"] | ||
| description = "Lint shell scripts using shellcheck." | ||
| tools.shellcheck = "latest" | ||
| run = ''' | ||
| shellcheck --external-sources --source-path=SCRIPTDIR **/*.sh | ||
| ''' |
There was a problem hiding this comment.
Enable globstar for recursive shell script matching.
The **/*.sh pattern requires bash's globstar option to expand recursively. Without it, shellcheck will only match files in the current directory, potentially missing nested scripts.
🐚 Proposed fix to enable globstar
[tasks."lint:shellcheck"]
description = "Lint shell scripts using shellcheck."
tools.shellcheck = "latest"
run = '''
+shopt -s globstar
shellcheck --external-sources --source-path=SCRIPTDIR **/*.sh
'''📝 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.
| [tasks."lint:shellcheck"] | |
| description = "Lint shell scripts using shellcheck." | |
| tools.shellcheck = "latest" | |
| run = ''' | |
| shellcheck --external-sources --source-path=SCRIPTDIR **/*.sh | |
| ''' | |
| [tasks."lint:shellcheck"] | |
| description = "Lint shell scripts using shellcheck." | |
| tools.shellcheck = "latest" | |
| run = ''' | |
| shopt -s globstar | |
| shellcheck --external-sources --source-path=SCRIPTDIR **/*.sh | |
| ''' |
🤖 Prompt for AI Agents
In @mise.toml around lines 76 - 81, The shellcheck task's run script uses the
**/*.sh glob which requires Bash's globstar option; update the
tasks."lint:shellcheck" run command to enable globstar before invoking
shellcheck (e.g., set shopt -s globstar or run bash with -O globstar) so nested
scripts are matched, or alternatively replace the glob with a find/xargs
pipeline that locates all *.sh under SCRIPTDIR and feeds them to shellcheck;
ensure you reference the existing run string and SCRIPTDIR symbol when making
the change.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted filessee 13 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Summary by CodeRabbit
Chores
New Features
Documentation
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.