fix: test project build and anvil process leaks#35
Merged
solofberlin merged 7 commits intomainfrom Feb 27, 2026
Merged
Conversation
Forge auto-generates incorrect remappings for safe-smart-account when treb-sol is symlinked into the test project, pointing to the repo root instead of the contracts/ subdirectory. Adding an explicit remappings.txt fixes forge build in test/testdata/project. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Always release test contexts back to pool, even with -treb.skipcleanup (preserves work directories but still kills anvils) - Always set inUse=false and signal pool on Release, even if Clean fails, preventing permanent context lockout - Fix pool init partial failure cleanup to iterate results slice instead of the empty pool.contexts - Wait for anvil process to actually exit after SIGTERM with 5s timeout before removing PID file, falling back to SIGKILL Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughBuild process refactoring introduces a dedicated setup phase in the Makefile with submodule initialization and dependency installation, while process termination in Anvil gains timeout-based synchronization. Test infrastructure simplifies cleanup logic and improves error handling, with dependency remappings and submodule pointer updates reflecting expanded integration scope. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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)
Makefile (1)
168-172:⚠️ Potential issue | 🟡 MinorMissing semicolon causes lint-install to fail.
Line 170 is missing a semicolon (
;) before the backslash, so theechoon line 171 becomes an argument to theshcommand rather than a separate statement.🐛 Proposed fix
lint-install: `@command` -v golangci-lint >/dev/null 2>&1 || { \ echo "Installing golangci-lint..."; \ - curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s -- -b $(go env GOPATH)/bin v2.1.6 \ + curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s -- -b $$(go env GOPATH)/bin v2.1.6; \ echo "✅ golangci-lint installed"; \ }Note: Also changed
$(go env GOPATH)to$$(go env GOPATH)to properly escape the shell command execution within Make.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 168 - 172, The Makefile's golangci-lint install block incorrectly omits a semicolon before the line-continuation so the subsequent echo is passed as an argument to sh, and the Makefile also fails to escape the subshell call; fix the block that references golangci-lint and the curl install script by adding a semicolon before the trailing backslash on the curl line and by replacing $(go env GOPATH) with $$(go env GOPATH) so the subshell is evaluated at shell-run time by Make; ensure the echo "✅ golangci-lint installed" remains a separate statement after the install command.
🧹 Nitpick comments (3)
mise.toml (1)
1-2: Consider pinning Go version for reproducibility.Using
go = "latest"may lead to inconsistent behavior across developer machines if different versions get resolved. Consider pinning to a specific version (e.g.,go = "1.23") to ensure consistent tooling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mise.toml` around lines 1 - 2, The tools section currently pins Go to "latest" which can cause inconsistent tool resolution; update the [tools] go entry to a specific semantic version (e.g., change go = "latest" to go = "1.23") in the mise.toml file so the go toolchain is reproducible across environments and CI, keeping the [tools] table and the go key name intact.Makefile (1)
13-20: Setup target runs unconditionally on every build.The
setuptarget has no prerequisites, so it runs fully on everymake buildinvocation. For faster iterative builds, consider either:
- Adding a sentinel file to skip if already set up
- Making setup a separate manual step not in the build dependency chain
Also,
abigen@latestmay resolve to different versions over time, similar to themise.tomlconcern.💡 Optional: Add sentinel file for idempotency
-setup: +.setup-done: `@echo` "🔨 Getting submodules" `@git` submodule init `@git` submodule update `@echo` "🔨 Installing forge deps" `@cd` treb-sol && forge install `@echo` "🔨 Installing abigen" - `@go` install github.com/ethereum/go-ethereum/cmd/abigen@latest + `@go` install github.com/ethereum/go-ethereum/cmd/abigen@v1.15.11 + `@touch` .setup-done + +setup: .setup-done # Build the CLI binary -build: setup bindings +build: .setup-done bindings🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 13 - 20, The setup Makefile target runs unconditionally—change it to be idempotent by creating and checking a sentinel (e.g., .setup_done) inside the setup rule (only run git submodule init/update, forge install, and abigen install if the sentinel is missing, and touch the sentinel after success), or remove setup from the build dependency chain so it is invoked manually; also avoid go install github.com/ethereum/go-ethereum/cmd/abigen@latest by pinning a specific version or adding a version variable to the Makefile so abigen installation is reproducible (refer to the setup target and the abigen install command to locate and update these lines).test/helpers/isolated.go (1)
18-18: Consider logging Release errors.The error returned by
pool.Release(testCtx)is silently ignored. While the test has already completed at this point, logging cleanup failures would aid debugging test infrastructure issues.💡 Suggested improvement
- defer pool.Release(testCtx) + defer func() { + if err := pool.Release(testCtx); err != nil { + t.Logf("Warning: failed to release test context: %v", err) + } + }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/helpers/isolated.go` at line 18, The deferred call to pool.Release(testCtx) currently ignores its returned error; wrap the defer in a closure that captures the error and logs it (e.g. using the test logger t.Logf or a package logger) so cleanup failures are visible: call pool.Release(testCtx), check its error return, and log a descriptive message including the error and context (reference: pool.Release and testCtx in the helper).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/anvil/anvil.go`:
- Around line 172-184: The goroutine calling process.Wait() is incorrect because
process was obtained via os.FindProcess(pid) (not a child) and Wait() returns an
unchecked error and won't block; replace that logic by polling the process
liveness using process.Signal(syscall.Signal(0)) (check and handle the returned
error) in a loop with a timeout instead of using process.Wait(), ensure you
check and propagate errors from Kill()/Signal calls, and only remove the PID
file after Signal(0) indicates the process has exited (or after the forced Kill
succeeded and the poll confirms it's gone); update the goroutine/timeout code
around the existing done channel and the Kill() path to perform this polling and
proper error checks.
---
Outside diff comments:
In `@Makefile`:
- Around line 168-172: The Makefile's golangci-lint install block incorrectly
omits a semicolon before the line-continuation so the subsequent echo is passed
as an argument to sh, and the Makefile also fails to escape the subshell call;
fix the block that references golangci-lint and the curl install script by
adding a semicolon before the trailing backslash on the curl line and by
replacing $(go env GOPATH) with $$(go env GOPATH) so the subshell is evaluated
at shell-run time by Make; ensure the echo "✅ golangci-lint installed" remains a
separate statement after the install command.
---
Nitpick comments:
In `@Makefile`:
- Around line 13-20: The setup Makefile target runs unconditionally—change it to
be idempotent by creating and checking a sentinel (e.g., .setup_done) inside the
setup rule (only run git submodule init/update, forge install, and abigen
install if the sentinel is missing, and touch the sentinel after success), or
remove setup from the build dependency chain so it is invoked manually; also
avoid go install github.com/ethereum/go-ethereum/cmd/abigen@latest by pinning a
specific version or adding a version variable to the Makefile so abigen
installation is reproducible (refer to the setup target and the abigen install
command to locate and update these lines).
In `@mise.toml`:
- Around line 1-2: The tools section currently pins Go to "latest" which can
cause inconsistent tool resolution; update the [tools] go entry to a specific
semantic version (e.g., change go = "latest" to go = "1.23") in the mise.toml
file so the go toolchain is reproducible across environments and CI, keeping the
[tools] table and the go key name intact.
In `@test/helpers/isolated.go`:
- Line 18: The deferred call to pool.Release(testCtx) currently ignores its
returned error; wrap the defer in a closure that captures the error and logs it
(e.g. using the test logger t.Logf or a package logger) so cleanup failures are
visible: call pool.Release(testCtx), check its error return, and log a
descriptive message including the error and context (reference: pool.Release and
testCtx in the helper).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Makefilemise.tomlpkg/anvil/anvil.gotest/helpers/isolated.gotest/helpers/test_context.gotest/testdata/project/remappings.txttreb-sol
- Fix unchecked process.Wait() return in anvil stop - Suppress gosec false positives for SSRF on configured API endpoints, path traversal on internal debug paths, and secret pattern on config struct field Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace process.Wait() with Signal(0) polling for non-child process termination detection in anvil stop - Log Release() errors in test isolation defer - Fix lint-install Makefile target: add missing semicolon and escape subshell in $(go env GOPATH) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
remappings.txttotest/testdata/project/soforge buildresolvessafe-smart-accountimports correctly (Forge auto-remapping misses thecontracts/subdirectory when treb-sol is symlinked)Clean()errors permanently locking contexts, partial pool init leaving orphans, andStopAnvilInstancenot waiting for process exitTest plan
cd test/testdata/project && forge buildsucceedsmake integration-testpasses with no orphaned anvil processes aftermake integration-testwith-treb.skipcleanuppreserves work dirs but still kills anvilsps aux | grep anvilshows no leftover processes after test runs🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores