feat: fork mode for testing deployments against local network forks#38
Merged
feat: fork mode for testing deployments against local network forks#38
Conversation
Add ForkState domain model with ForkEntry and SnapshotEntry structs, ForkStateStore interface in ports, and file-based adapter that persists to .treb/priv/fork-state.json. Includes comprehensive unit tests for serialization round-trip, IsForkActive/GetActiveFork helpers, and adapter CRUD operations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implement `treb fork enter <network>` CLI command that starts a forked anvil instance, backs up registry files, takes an EVM snapshot, saves fork state, and adds `.treb/priv/` to `.gitignore`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add fork.setup configuration key to local config model, allowing users to configure a setup script that runs automatically during fork enter. - Add ForkSetup field to LocalConfig and fork.setup config key - Support treb config set/remove fork.setup <path> - Execute configured setup script after anvil health check in fork enter - Take initial EVM snapshot AFTER setup script completes - Abort fork enter if setup script fails (stop anvil, clean up state) - Skip silently if not configured or script file doesn't exist - Integration tests for setup script execution, failure, and skip Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add fork awareness to `treb list` command: - [fork] indicator on deployments added during fork mode - --fork flag to filter only fork-added deployments - --no-fork flag to filter only pre-fork deployments - --json flag with fork: true field on fork-added entries - Integration tests for all fork list scenarios Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Forked anvil instances need to connect to the remote RPC and fetch initial state before they can serve requests, which takes much longer than 200ms. Replace the fixed sleep with a polling loop: 5s timeout for fresh anvils, 30s for forked ones. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add log file path to fork status, fork enter, and fork restart output - Show "FORK" mode (in purple) instead of "LIVE" in the deployment banner when running scripts in fork mode Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds comprehensive fork management: persistent fork state, filesystem backup/restore, Anvil readiness + RPC snapshot/revert, fork-aware CLI and use-cases (enter/exit/revert/restart/status/history/diff), Forge env overrides for forks, and extensive tests and wiring. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Fork CLI
participant UC as EnterFork UseCase
participant AnvilMgr as AnvilManager
participant ForkState as ForkStateStore
participant ForkFiles as ForkFileManager
participant Forge as ForgeRunner
CLI->>UC: Execute(params)
UC->>ForkState: Load()
ForkState-->>UC: current fork state
UC->>AnvilMgr: Start(instance)
AnvilMgr->>AnvilMgr: waitForReady()
AnvilMgr-->>UC: ready
UC->>UC: executeSetupFork()
UC->>Forge: Run(setup script)
Forge-->>UC: setup result
UC->>ForkFiles: BackupFiles()
ForkFiles-->>UC: backup complete
UC->>AnvilMgr: TakeSnapshot()
AnvilMgr-->>UC: snapshot ID
UC->>ForkState: Save(ForkEntry)
ForkState-->>UC: saved
UC-->>CLI: EnterForkResult
sequenceDiagram
participant CLI as Fork CLI
participant UC as RevertFork UseCase
participant AnvilMgr as AnvilManager
participant ForkState as ForkStateStore
participant ForkFiles as ForkFileManager
CLI->>UC: Execute(network, all?)
UC->>ForkState: Load()
ForkState-->>UC: fork state
UC->>UC: select snapshot(s)
UC->>AnvilMgr: RevertSnapshot(ID)
AnvilMgr-->>UC: reverted
UC->>ForkFiles: RestoreFiles(snapshot dir)
ForkFiles-->>UC: files restored
UC->>ForkState: Save(updated state)
ForkState-->>UC: saved
UC-->>CLI: RevertForkResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cli/show.go (1)
55-57:⚠️ Potential issue | 🟠 MajorPotential nil pointer dereference when setting network name.
If the
--networkflag is provided butapp.Config.Networkis nil (e.g., when no network was configured), this will panic. Consider initializing the Network struct if nil before accessing its Name field.🐛 Proposed fix
if network != "" { + if app.Config.Network == nil { + app.Config.Network = &config.Network{} + } app.Config.Network.Name = network }Note: You may need to import the config package if not already imported.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/show.go` around lines 55 - 57, The code sets app.Config.Network.Name when network != "" but can panic if app.Config.Network is nil; before assigning, check if app.Config.Network == nil and initialize it (e.g., app.Config.Network = &config.Network{} or appropriate struct) so you can safely set Name; update the branch that handles the --network flag in internal/cli/show.go to ensure app.Config.Network is non-nil prior to setting app.Config.Network.Name and import the config package if needed.
🧹 Nitpick comments (20)
test/testdata/project/.gitignore (1)
15-15: Redundant ignore rule under already-ignored parentSince Line 14 already ignores
.treb, Line 15 (.treb/priv/) has no additional effect. Consider keeping only one rule based on intent (ignore all of.trebvs only private subtree) to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/testdata/project/.gitignore` at line 15, The .gitignore contains overlapping rules: `.treb` and `.treb/priv/`; decide which behavior you want and remove the redundant entry — either keep `.treb` to ignore the entire directory or keep only `.treb/priv/` to ignore only the private subtree; update the file by deleting the unneeded rule (`.treb` or `.treb/priv/`) so the ignore rules are unambiguous.scripts/ralph/progress.txt (1)
276-276: Tighten wording for readability.At Line 276, replace “exact same pattern” with “same pattern” for cleaner phrasing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ralph/progress.txt` at line 276, Update the wording in the sentence describing the isForkDeployment method: change the phrase "exact same pattern" to "same pattern" so the sentence reads that isForkDeployment follows the same pattern as computeForkDeploymentIDs in ListDeployments but checks a single deployment ID instead of computing the full set; locate the sentence referencing the isForkDeployment method and computeForkDeploymentIDs and make this simple text edit.internal/cli/root.go (1)
157-168: Silent error swallowing may mask issues during debugging.The function silently returns
falsewhenForkStateStore.Loadfails (line 164-165). While this is a reasonable defensive choice to avoid blocking commands when state can't be loaded, it could mask legitimate errors like permission issues or corrupted state files.Consider whether logging the error at debug level would aid troubleshooting without impacting normal flow.
💡 Optional: Log errors at debug level
func isForkActiveForCurrentNetwork(ctx context.Context, a *app.App) (bool, string) { if a.Config.Network == nil { return false, "" } state, err := a.ForkStateStore.Load(ctx) if err != nil { + // Debug log could help troubleshoot fork state issues return false, "" } return state.IsForkActive(a.Config.Network.Name), a.Config.Network.Name }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/root.go` around lines 157 - 168, In isForkActiveForCurrentNetwork, don't silently swallow the error from a.ForkStateStore.Load; instead log the error at debug level while preserving the current return behavior—capture err from a.ForkStateStore.Load and call the app logger's debug method (e.g., a.Logger.Debugf or the project's equivalent) with a message including the network name (a.Config.Network.Name) and the error, then return false, "" as before.internal/adapters/fs/fork_state_store.go (1)
49-66: Consider atomic write pattern for improved crash safety.The current implementation writes directly to the state file. If the process crashes during write, the file could be left in a corrupted state. For a development tool this is acceptable, but for extra robustness consider writing to a temporary file and then renaming.
♻️ Optional: Atomic write pattern
func (s *ForkStateStoreAdapter) Save(_ context.Context, state *domain.ForkState) error { dir := filepath.Dir(s.statePath) if err := os.MkdirAll(dir, 0755); err != nil { return fmt.Errorf("failed to create fork state directory: %w", err) } data, err := json.MarshalIndent(state, "", " ") if err != nil { return fmt.Errorf("failed to marshal fork state: %w", err) } - if err := os.WriteFile(s.statePath, data, 0644); err != nil { + tmpPath := s.statePath + ".tmp" + if err := os.WriteFile(tmpPath, data, 0644); err != nil { return fmt.Errorf("failed to write fork state file: %w", err) } + if err := os.Rename(tmpPath, s.statePath); err != nil { + return fmt.Errorf("failed to rename fork state file: %w", err) + } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapters/fs/fork_state_store.go` around lines 49 - 66, The Save method on ForkStateStoreAdapter currently writes directly to s.statePath which can leave a corrupted file if interrupted; change ForkStateStoreAdapter.Save to write the marshalled data to a temporary file in the same directory (e.g., statePath + ".tmp" or using ioutil.TempFile in that dir), fsync the file if possible, close it, then atomically rename it to s.statePath (os.Rename) and handle/propagate errors from each step; ensure the directory creation and JSON marshaling remain, but replace the direct os.WriteFile call with the temp-write+rename sequence to achieve atomic writes.internal/cli/show.go (1)
90-90: Color detection is a no-op placeholder.The expression
cmd.OutOrStdout() == cmd.OutOrStdout()always evaluates totrue. If color detection isn't critical now, consider adding a TODO or using a proper terminal detection library.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/show.go` at line 90, The current color detection in internal/cli/show.go is a no-op because color := cmd.OutOrStdout() == cmd.OutOrStdout() always yields true; replace this with a real terminal check (for example, use an isatty check such as isatty.IsTerminal(os.Stdout.Fd()) or an equivalent library) and set color based on that, or if you prefer to defer, replace the line with a clear TODO comment and a safe default (e.g., color := false) so it's not a silent always-true placeholder; update the code referencing the color variable accordingly (look for the color variable assignment and any downstream formatting logic using it).test/integration/fork_test.go (1)
340-353: Consider adding timeout to HTTP client for RPC calls.The
ethGetCodeandethGetBalancehelpers usehttp.Postwithout a timeout. While tests typically have overall timeouts, consider using anhttp.Clientwith a reasonable timeout (e.g., 5 seconds) to prevent tests from hanging indefinitely if the Anvil instance becomes unresponsive.♻️ Optional: Add timeout to RPC helpers
var rpcClient = &http.Client{Timeout: 5 * time.Second} // Then use rpcClient.Post instead of http.Post resp, err := rpcClient.Post(rpcURL, "application/json", bytes.NewBuffer(reqBody))Also applies to: 566-579
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/fork_test.go` around lines 340 - 353, The RPC helpers ethGetCode and ethGetBalance use the package-level http.Post which lacks a timeout; add a reusable http.Client with a timeout (e.g., rpcClient := &http.Client{Timeout: 5 * time.Second}) and replace calls to http.Post with rpcClient.Post in the ethGetCode and ethGetBalance helpers (and the other helper at the cited lines) so RPC calls fail fast if the Anvil instance is unresponsive.internal/usecase/run_script.go (1)
302-308: Consider extracting AnvilInstance construction helper.The
AnvilInstanceconstruction fromForkEntryis duplicated incheckForkHealth(lines 302-308) andtakePreRunSnapshot(lines 337-343). Consider extracting a helper method likebuildAnvilInstanceFromFork(fork *domain.ForkEntry) *domain.AnvilInstanceto reduce duplication and ensure consistency.Also applies to: 337-343
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/usecase/run_script.go` around lines 302 - 308, Extract the duplicated AnvilInstance construction into a helper function (e.g., buildAnvilInstanceFromFork) that takes a *domain.ForkEntry and returns *domain.AnvilInstance; replace the inline constructions in checkForkHealth and takePreRunSnapshot with calls to buildAnvilInstanceFromFork(fork) so Port, Name, ChainID, PidFile and LogFile are created consistently (use portFromURL and fmt.Sprintf as in the originals inside the helper).internal/adapters/fs/fork_file_manager.go (1)
64-83: Non-atomic restore could leave partial state on failure.If
RestoreFilesfails midway through the file loop (e.g., disk full on third file), the registry will be in a partially restored state. For fork mode this is likely acceptable since the user can retry or exit, but consider documenting this behavior or implementing atomic restore (copy to temp, then rename) for critical scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapters/fs/fork_file_manager.go` around lines 64 - 83, RestoreFiles performs per-file copy which can leave a partially restored state if it fails mid-loop; update RestoreFiles to perform atomic restores by copying each registry file (from snapshotDir join name via copyFile or a new helper) to a temporary file in m.dataDir (e.g., name+".tmp") and then rename (os.Rename) the temp to the final dst, handling cleanup of temps on error; reference functions/vars: RestoreFiles, copyFile, snapshotDir, registryFiles, and m.dataDir (or dataDir) when locating files and temps.internal/usecase/exit_fork.go (2)
169-178: ExtractportFromURLto a shared utility.This function is duplicated across
exit_fork.go,revert_fork.go,restart_fork.go, andfork_status.go. Consider moving it to a shared location.♻️ Suggested refactor
Create a shared file like
internal/usecase/fork_utils.go:package usecase import "net/url" // portFromURL extracts the port from a URL like "http://127.0.0.1:12345" func portFromURL(rawURL string) string { u, err := url.Parse(rawURL) if err != nil { return "" } return u.Port() }Using
url.Parseis more robust than manual string scanning and handles edge cases like IPv6 addresses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/usecase/exit_fork.go` around lines 169 - 178, The portFromURL implementation in exit_fork.go is duplicated; extract it into a single shared helper (e.g., create internal/usecase/fork_utils.go) as func portFromURL(rawURL string) string and implement it using net/url (parse the rawURL and return u.Port(), returning "" on parse error) so it correctly handles IPv6 and edge cases; then remove the duplicate definitions and have exit_fork.go, revert_fork.go, restart_fork.go, and fork_status.go use the shared portFromURL function from the same package.
136-140: Avoid directfmt.Printfin use case layer.Printing directly to stdout in the use case layer breaks the separation of concerns. The warning should be communicated through the result struct or a logger interface.
♻️ Suggested approach
Add a
Warnings []stringfield toExitForkResultand collect warnings there:// ExitForkResult contains the result of exiting fork mode type ExitForkResult struct { ExitedNetworks []string Message string + Warnings []string }Then in
cleanupFork, return warnings instead of printing:-func (uc *ExitFork) cleanupFork(ctx context.Context, entry *domain.ForkEntry) error { +func (uc *ExitFork) cleanupFork(ctx context.Context, entry *domain.ForkEntry) (warning string, err error) { // ... if err := uc.anvilManager.Stop(ctx, instance); err != nil { - fmt.Printf("Warning: failed to stop anvil for '%s': %v\n", entry.Network, err) + warning = fmt.Sprintf("failed to stop anvil for '%s': %v", entry.Network, err) } // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/usecase/exit_fork.go` around lines 136 - 140, Replace the direct fmt.Printf call in the use case with collecting the warning into the use case result: add a Warnings []string field to ExitForkResult, change cleanupFork/ExitFork to append the formatted warning (e.g., fmt.Sprintf("failed to stop anvil for '%s': %v", entry.Network, err)) to that slice instead of printing, and ensure callers (the presenter/handler) read ExitForkResult.Warnings and log or display them; specifically update the block that calls uc.anvilManager.Stop(ctx, instance) to push the message into ExitForkResult.Warnings when err != nil and remove the fmt.Printf usage.internal/cli/render/fork.go (1)
83-92:formatDurationmay produce unexpected output for edge cases.The function doesn't handle:
- Negative durations (could happen if clocks are skewed)
- Durations >= 24 hours
For a CLI tool, these edge cases are likely rare but worth considering.
♻️ Suggested improvement
func formatDuration(d time.Duration) string { + if d < 0 { + return "0s" + } if d < time.Minute { return fmt.Sprintf("%ds", int(d.Seconds())) } if d < time.Hour { return fmt.Sprintf("%dm%ds", int(d.Minutes()), int(d.Seconds())%60) } + if d < 24*time.Hour { + return fmt.Sprintf("%dh%dm", int(d.Hours()), int(d.Minutes())%60) + } + days := int(d.Hours()) / 24 + hours := int(d.Hours()) % 24 + return fmt.Sprintf("%dd%dh", days, hours) - return fmt.Sprintf("%dh%dm", int(d.Hours()), int(d.Minutes())%60) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/render/fork.go` around lines 83 - 92, formatDuration currently returns unexpected strings for negative durations and durations >=24h; update formatDuration to handle these edge cases by (1) normalizing or explicitly handling negative durations (e.g., return "-{...}" or clamp to "0s" depending on CLI convention) and (2) adding day-resolution for durations >=24h (compute days via int(d.Hours())/24 and then remaining hours/minutes). Modify the function body in formatDuration to detect d < 0 and handle it, and to compute days/hours/minutes/seconds for large durations so outputs like "1d3h" or a clear negative representation are produced.internal/usecase/show_deployment.go (1)
114-115: VerifyloadDeploymentIDsis accessible from this file.The function
loadDeploymentIDsis called here but appears to be defined infork_status.goas an unexported function. Since both files are in the same package, this should work, but the path construction pattern (Line 114) is duplicated across multiple files in this PR.Consider extracting a helper like
getSnapshotBackupPath(dataDir, network, snapshotIndex string) stringto centralize this path construction and reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/usecase/show_deployment.go` around lines 114 - 115, The call to loadDeploymentIDs is valid only because both files share the same package; confirm loadDeploymentIDs remains unexported and accessible, then eliminate the duplicated filepath.Join pattern by adding a helper function getSnapshotBackupPath(dataDir, networkName, snapshotIndex string) string and replace the inline construction in show_deployment.go (where backupPath is built from uc.config.DataDir, "priv", "fork", networkName, "snapshots", "0", "deployments.json") and other files to call getSnapshotBackupPath(uc.config.DataDir, networkName, "0") before calling loadDeploymentIDs; keep the helper name and parameters consistent across callers so future changes centralize the path format.internal/usecase/fork_status.go (2)
72-76: Non-deterministic ordering of fork status entries.Iterating over
state.Forksmap produces entries in random order. For consistent CLI output, consider sorting entries by network name.♻️ Suggested fix
+import "sort" func (uc *ForkStatus) Execute(ctx context.Context) (*ForkStatusResult, error) { // ... + // Collect and sort network names for consistent ordering + networks := make([]string, 0, len(state.Forks)) + for network := range state.Forks { + networks = append(networks, network) + } + sort.Strings(networks) + var entries []ForkStatusEntry - for _, entry := range state.Forks { + for _, network := range networks { + entry := state.Forks[network] statusEntry := uc.buildStatusEntry(ctx, entry, currentNetwork) entries = append(entries, statusEntry) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/usecase/fork_status.go` around lines 72 - 76, state.Forks is a map and iterating over it produces non-deterministic ordering; update the loop in the function that builds the slice (where uc.buildStatusEntry is called) to first collect the map keys (network names), sort them (lexicographically or by desired network order), then iterate the sorted keys to call uc.buildStatusEntry(ctx, entry, currentNetwork) and append to entries so the resulting []ForkStatusEntry has deterministic, sorted order for CLI output.
142-160:loadDeploymentIDsis a shared utility across multiple use cases.This function is used by
show_deployment.goandfork_status.go. Consider moving it to a dedicated utilities file (e.g.,fork_utils.go) to make its shared nature explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/usecase/fork_status.go` around lines 142 - 160, The helper function loadDeploymentIDs is shared by show_deployment.go and fork_status.go and should be moved into a common utility file: create a new file (e.g., fork_utils.go) in the same package and cut/paste the loadDeploymentIDs function there, preserving its signature and behavior; update imports if needed (json, os) and ensure both show_deployment.go and fork_status.go continue to call loadDeploymentIDs (no API change), keeping the function unexported unless you intend to use it outside the package.internal/usecase/diff_fork.go (2)
128-134: JSON string comparison may produce false positives for modifications.Comparing
string(currentRaw) != string(backupRaw)can flag entries as modified when only the JSON key ordering differs, even if the semantic content is identical. This depends on how the JSON is serialized.If the deployment files are always written by the same code path with consistent ordering, this is likely fine. Otherwise, consider unmarshaling both and comparing fields.
♻️ Alternative approach if false positives occur
// For more robust comparison, unmarshal both and compare struct fields var currentDep, backupDep deploymentJSON if err := json.Unmarshal(currentRaw, ¤tDep); err != nil { continue } if err := json.Unmarshal(backupRaw, &backupDep); err != nil { // Backup parse failed, treat as modified entry.ChangeType = "modified" modifiedDeps = append(modifiedDeps, entry) continue } if currentDep != backupDep { entry.ChangeType = "modified" modifiedDeps = append(modifiedDeps, entry) }Note: This only compares the fields in
deploymentJSON. If other fields matter, expand the struct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/usecase/diff_fork.go` around lines 128 - 134, The current comparison uses string(currentRaw) != string(backupRaw) which can yield false positives due to JSON key ordering; instead unmarshal currentRaw and backupRaw into a struct (e.g., deploymentJSON) and compare the resulting values (or fields) for equality, treating unmarshalling errors on backupRaw as "modified" and appending to modifiedDeps while setting entry.ChangeType = "modified"; update the block that references currentRaw, backupRaw, entry.ChangeType, and modifiedDeps to perform json.Unmarshal into deploymentJSON variables and compare those instead of raw string equality.
140-155: Function naming:loadDeploymentIDsused for transactions.The function
loadDeploymentIDs(fromfork_status.go) is being used to load transaction IDs. While it works (both files are JSON maps), the name is misleading. Consider renaming toloadJSONMapKeysor creating a transaction-specific variant for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/usecase/diff_fork.go` around lines 140 - 155, The helper name loadDeploymentIDs is misleading when used by diffTransactions to read transaction IDs; either rename it to a generic loadJSONMapKeys (update its definition in fork_status.go and all callers including diffTransactions) or add a thin wrapper function loadTransactionIDs in fork_status.go that delegates to the existing implementation and use that from DiffFork.diffTransactions; update DiffFork.diffTransactions to call the new/renamed symbol (diffTransactions, loadDeploymentIDs / loadJSONMapKeys / loadTransactionIDs) so the intent is clear without changing behavior.internal/usecase/revert_fork.go (2)
99-101: Consider logging or returning warnings for snapshot directory removal failures.Silently ignoring
os.RemoveAllerrors could hide filesystem permission issues or other problems. While this isn't critical (the revert succeeded), it may leave orphaned directories.♻️ Suggested approach
At minimum, consider debug logging:
- _ = os.RemoveAll(snapshotDir) + if err := os.RemoveAll(snapshotDir); err != nil { + // Log but don't fail - revert succeeded, cleanup is best-effort + // Consider adding to a warnings slice in the result + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/usecase/revert_fork.go` around lines 99 - 101, The code currently ignores errors from os.RemoveAll when deleting snapshotDir; update the removal to capture the returned error from os.RemoveAll(snapshotDir) and emit a non-fatal log (e.g., uc.logger.Warnf or uc.log.Warn depending on available logger) including snapshotDir, uc.cfg.ProjectRoot, entry.Network and topSnapshot.Index to aid debugging; do not change the revert outcome—only log the failure (or return a warning value) so orphaned directories or permission issues are visible.
100-101: UseForkFileManagerfor directory cleanup instead of directos.RemoveAll.The
ForkFileManagerinterface already hasCleanupForkDir. For consistency with the hexagonal architecture and to keep filesystem operations abstracted, consider adding a method likeRemoveSnapshotDir(ctx, network, index)to the interface, or use the existing cleanup patterns.This also applies to Lines 148-149 in
revertAll.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/usecase/revert_fork.go` around lines 100 - 101, Replace direct filesystem calls to os.RemoveAll in revert_fork.go with the ForkFileManager abstraction: add a new method to the ForkFileManager interface such as RemoveSnapshotDir(ctx context.Context, network string, index int64) (or reuse an existing CleanupForkDir variant) and call uc.fileManager.RemoveSnapshotDir(ctx, entry.Network, topSnapshot.Index) instead of computing snapshotDir and calling os.RemoveAll; do the same replacement in the revertAll code path (lines referenced around 148-149) so all snapshot directory deletions go through the ForkFileManager implementation.internal/cli/fork.go (1)
88-92: Interactive prompt may behave unexpectedly with piped input.Using
fmt.Scanlnfor interactive prompts can behave unexpectedly when stdin is piped or redirected. The current error handling treats any scan error as a "no" response, which is reasonable, but consider using a dedicated interactive input library for robustness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/fork.go` around lines 88 - 92, The interactive prompt using fmt.Scanln(&answer) can misbehave when stdin is piped; update the confirmation flow around fmt.Scanln and answer to first detect if stdin is a terminal (use os.Stdin and a terminal check such as golang.org/x/term.IsTerminal) and, if not a terminal, treat it as non-interactive and fail with a clear error instead of attempting to Scanln; if it is a terminal, read the line more robustly (e.g., with bufio.Reader.ReadString('\n')) and normalize the response before comparing to "y"/"Y" so the fork prompt behaves consistently.internal/usecase/restart_fork.go (1)
181-184: Silent error handling when loading local config may hide issues.When
uc.localConfig.Load(ctx)fails, the function returns(false, nil)without logging. If the config file exists but is malformed, users won't know why the setup script didn't run.Consider distinguishing between "file not found" (acceptable) vs. other errors (worth logging/warning).
♻️ Suggested approach
func (uc *RestartFork) executeSetupFork(ctx context.Context, entry *domain.ForkEntry, forkEnvOverrides map[string]string) (bool, error) { localCfg, err := uc.localConfig.Load(ctx) if err != nil { + // Only ignore "not found" errors; other errors may indicate real problems + if !os.IsNotExist(err) { + // Log warning about config load failure + } return false, nil }
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/adapters/anvil/manager_test.go`:
- Around line 119-132: In newMockRPCServer's HTTP handler, avoid calling
t.Fatalf from the handler goroutine; instead create an error channel (e.g.,
errCh := make(chan error, 1) in newMockRPCServer or accept a testing-safe
reporter) and on JSON decode/encode errors send the error into that channel and
return early (or call t.Error/t.Log on the main test goroutine by reading the
channel), then let the test read from errCh after exercising the server and fail
the test there; update the handler's json.Decode and json.Encode error paths to
send errors to the channel (or call t.Error) rather than calling t.Fatalf
directly so failures are reported from the test goroutine.
In `@internal/adapters/forge/forge.go`:
- Around line 318-321: The loop that merges config.ForkEnvOverrides into env
(using config.ForkEnvOverrides and the env map in forge.go) risks leaking RPC
secrets because the code later logs the entire env with values; change the
logging to never include raw secret values—either log only the keys or build a
redacted copy of env where values for any key present in config.ForkEnvOverrides
(and any likely secret keys) are replaced with "<redacted>" before calling the
logger. Keep the assignment env[k]=v as-is but ensure the logger call that
currently logs env uses the redacted map or a keys-only list instead of the real
env values.
In `@internal/cli/fork.go`:
- Around line 340-347: runForkHistory currently sets network to empty when no
arg is provided, causing inconsistent behavior with
runForkRevert/runForkRestart/runForkExit; change the logic so it defaults to the
configured network (same as the other fork commands). Initialize network from
the command config (e.g., cfg.Network) and only override it when len(args) > 0,
then pass that network into usecase.ForkHistoryParams.Network so runForkHistory
behaves consistently with the other fork handlers.
In `@internal/cli/list.go`:
- Around line 103-105: The flags forkOnly and noFork are mutually exclusive but
not validated; in the command's RunE handler (the function that executes after
cmd.Flags() parsing), add a check that if forkOnly && noFork then return a
user-facing error (e.g., fmt.Errorf("flags --fork and --no-fork are mutually
exclusive")) so the command fails fast; locate the flags defined with
cmd.Flags().BoolVar(&forkOnly, "fork", ...) and cmd.Flags().BoolVar(&noFork,
"no-fork", ...) and add this validation at the start of the RunE function (or
use Cobra's MarkFlagsMutuallyExclusive on those two flag names) to prevent both
being specified together.
- Around line 141-146: The JSON renderer renderListJSON currently prints via
fmt.Println; change it to write to the Cobra command's output writer by using
cmd.OutOrStdout() as the io.Writer (e.g., get w := cmd.OutOrStdout() and write
the json.MarshalIndent result with fmt.Fprintln or w.Write), keeping the same
error handling and return values so output redirection/testing behaves like the
color renderer that already uses cmd.OutOrStdout().
In `@internal/cli/render/fork.go`:
- Around line 98-102: The output prints duplicate "Reverted:" labels; update the
printing block that checks result.RevertedCommand and the subsequent prints so
labels are distinct (e.g., change the first fmt.Printf for
result.RevertedCommand to "Reverted Command:" or change the second to "Reverted
Snapshots:") so it's unambiguous; adjust the statements that reference
result.RevertedCommand, result.RevertedCount and result.RemainingSnapshots in
the fork.go print block accordingly.
In `@internal/config/rpc_env.go`:
- Around line 92-101: The current code builds oldEntry/newEntry and does a
fragile string Replace on content (using variables networkName, oldValue,
envVarName, oldEntry, newEntry) which breaks for different
quoting/whitespace/comments; instead parse the TOML into a data structure with a
TOML library (e.g., BurntSushi/toml or pellet), locate the networkName key in
the parsed structure, replace its value with the env var expression (using
envVarName), then re-encode/write the TOML back to content so updates are robust
and not based on plain string matching; if round-trip formatting preservation is
required, use a library that supports editing/preserving formatting or
explicitly document the string-replacement limitation.
- Around line 120-125: The current duplicate check uses
strings.Contains(existing, envVarName+"=") which can yield false positives
(matches MY_VAR_EXTRA). In the function handling envVarName/entry/existing,
replace that check with a line-based match: split existing by lines (or use a
regexp anchored at line start) and verify any line begins with envVarName+"="
(or matches `^envVarName=`) before returning; only treat as present when a full
variable name match is found. Update the same function that builds entry to use
this new check to avoid partial-name collisions.
In `@internal/usecase/enter_fork.go`:
- Around line 184-188: The current handler swallows all errors from
uc.localConfig.Load(ctx); change it to distinguish "file not found" vs other
errors by checking the error with errors.Is(err, fs.ErrNotExist) (or
os.IsNotExist via the underlying error) and only treat not-found as a silent
skip (return false, nil), while returning other errors up (return false, err);
update the code around uc.localConfig.Load(ctx) and related localCfg usage
accordingly, using errors.Is to handle wrapped errors from
LocalConfigRepository.Load.
- Around line 74-78: getAvailablePort() currently binds to an ephemeral port,
closes the listener, and returns the port number, creating a race where another
process can take the port before uc.anvilManager.Start(...) binds it; fix by
eliminating the bind-release gap: modify uc.enterFork flow to either pass the
open net.Listener (from getAvailablePort or a new getAvailableListener) into
AnvilManager.Start (preferential) or, if Start cannot accept a listener, wrap
uc.anvilManager.Start in a retry loop that attempts to Start with a fresh port
on EADDRINUSE until success (use a small backoff and limit retries); update
getAvailablePort/getAvailableListener and AnvilManager.Start signatures
accordingly and ensure the code paths at the other occurrence (lines around
242-250) use the same approach.
In `@internal/usecase/list_deployments.go`:
- Around line 95-131: In computeForkDeploymentIDs, when uc.forkState.Load(ctx)
returns an error you currently return nil; change that to return an empty map
(make(map[string]bool)) to match loadDeploymentIDs' behavior and avoid potential
nil derefs. Update the early return in the error branch after
uc.forkState.Load(ctx) to return make(map[string]bool); keep other logic
(networkName checks and active-fork check) unchanged.
In `@treb-sol`:
- Line 1: The submodule gitlink for treb-sol points to an unreachable commit
(204a0a6328f1d0019e680e6aa8e83394938bb77a) which blocks checkouts; fix by either
pushing that missing commit to the treb-sol remote, or repointing the
superproject's treb-sol gitlink to a reachable commit (e.g.,
3941f8244c51a36b814d1e3ec278cfa30c5fb635 on refs/heads/main). To repoint, fetch
the remote for treb-sol, checkout the target reachable commit in the treb-sol
submodule, stage the updated gitlink in the superproject (so the submodule
reference is updated), and commit the change so subsequent clones and git
submodule update --init --recursive succeed.
---
Outside diff comments:
In `@internal/cli/show.go`:
- Around line 55-57: The code sets app.Config.Network.Name when network != ""
but can panic if app.Config.Network is nil; before assigning, check if
app.Config.Network == nil and initialize it (e.g., app.Config.Network =
&config.Network{} or appropriate struct) so you can safely set Name; update the
branch that handles the --network flag in internal/cli/show.go to ensure
app.Config.Network is non-nil prior to setting app.Config.Network.Name and
import the config package if needed.
---
Nitpick comments:
In `@internal/adapters/fs/fork_file_manager.go`:
- Around line 64-83: RestoreFiles performs per-file copy which can leave a
partially restored state if it fails mid-loop; update RestoreFiles to perform
atomic restores by copying each registry file (from snapshotDir join name via
copyFile or a new helper) to a temporary file in m.dataDir (e.g., name+".tmp")
and then rename (os.Rename) the temp to the final dst, handling cleanup of temps
on error; reference functions/vars: RestoreFiles, copyFile, snapshotDir,
registryFiles, and m.dataDir (or dataDir) when locating files and temps.
In `@internal/adapters/fs/fork_state_store.go`:
- Around line 49-66: The Save method on ForkStateStoreAdapter currently writes
directly to s.statePath which can leave a corrupted file if interrupted; change
ForkStateStoreAdapter.Save to write the marshalled data to a temporary file in
the same directory (e.g., statePath + ".tmp" or using ioutil.TempFile in that
dir), fsync the file if possible, close it, then atomically rename it to
s.statePath (os.Rename) and handle/propagate errors from each step; ensure the
directory creation and JSON marshaling remain, but replace the direct
os.WriteFile call with the temp-write+rename sequence to achieve atomic writes.
In `@internal/cli/fork.go`:
- Around line 88-92: The interactive prompt using fmt.Scanln(&answer) can
misbehave when stdin is piped; update the confirmation flow around fmt.Scanln
and answer to first detect if stdin is a terminal (use os.Stdin and a terminal
check such as golang.org/x/term.IsTerminal) and, if not a terminal, treat it as
non-interactive and fail with a clear error instead of attempting to Scanln; if
it is a terminal, read the line more robustly (e.g., with
bufio.Reader.ReadString('\n')) and normalize the response before comparing to
"y"/"Y" so the fork prompt behaves consistently.
In `@internal/cli/render/fork.go`:
- Around line 83-92: formatDuration currently returns unexpected strings for
negative durations and durations >=24h; update formatDuration to handle these
edge cases by (1) normalizing or explicitly handling negative durations (e.g.,
return "-{...}" or clamp to "0s" depending on CLI convention) and (2) adding
day-resolution for durations >=24h (compute days via int(d.Hours())/24 and then
remaining hours/minutes). Modify the function body in formatDuration to detect d
< 0 and handle it, and to compute days/hours/minutes/seconds for large durations
so outputs like "1d3h" or a clear negative representation are produced.
In `@internal/cli/root.go`:
- Around line 157-168: In isForkActiveForCurrentNetwork, don't silently swallow
the error from a.ForkStateStore.Load; instead log the error at debug level while
preserving the current return behavior—capture err from a.ForkStateStore.Load
and call the app logger's debug method (e.g., a.Logger.Debugf or the project's
equivalent) with a message including the network name (a.Config.Network.Name)
and the error, then return false, "" as before.
In `@internal/cli/show.go`:
- Line 90: The current color detection in internal/cli/show.go is a no-op
because color := cmd.OutOrStdout() == cmd.OutOrStdout() always yields true;
replace this with a real terminal check (for example, use an isatty check such
as isatty.IsTerminal(os.Stdout.Fd()) or an equivalent library) and set color
based on that, or if you prefer to defer, replace the line with a clear TODO
comment and a safe default (e.g., color := false) so it's not a silent
always-true placeholder; update the code referencing the color variable
accordingly (look for the color variable assignment and any downstream
formatting logic using it).
In `@internal/usecase/diff_fork.go`:
- Around line 128-134: The current comparison uses string(currentRaw) !=
string(backupRaw) which can yield false positives due to JSON key ordering;
instead unmarshal currentRaw and backupRaw into a struct (e.g., deploymentJSON)
and compare the resulting values (or fields) for equality, treating
unmarshalling errors on backupRaw as "modified" and appending to modifiedDeps
while setting entry.ChangeType = "modified"; update the block that references
currentRaw, backupRaw, entry.ChangeType, and modifiedDeps to perform
json.Unmarshal into deploymentJSON variables and compare those instead of raw
string equality.
- Around line 140-155: The helper name loadDeploymentIDs is misleading when used
by diffTransactions to read transaction IDs; either rename it to a generic
loadJSONMapKeys (update its definition in fork_status.go and all callers
including diffTransactions) or add a thin wrapper function loadTransactionIDs in
fork_status.go that delegates to the existing implementation and use that from
DiffFork.diffTransactions; update DiffFork.diffTransactions to call the
new/renamed symbol (diffTransactions, loadDeploymentIDs / loadJSONMapKeys /
loadTransactionIDs) so the intent is clear without changing behavior.
In `@internal/usecase/exit_fork.go`:
- Around line 169-178: The portFromURL implementation in exit_fork.go is
duplicated; extract it into a single shared helper (e.g., create
internal/usecase/fork_utils.go) as func portFromURL(rawURL string) string and
implement it using net/url (parse the rawURL and return u.Port(), returning ""
on parse error) so it correctly handles IPv6 and edge cases; then remove the
duplicate definitions and have exit_fork.go, revert_fork.go, restart_fork.go,
and fork_status.go use the shared portFromURL function from the same package.
- Around line 136-140: Replace the direct fmt.Printf call in the use case with
collecting the warning into the use case result: add a Warnings []string field
to ExitForkResult, change cleanupFork/ExitFork to append the formatted warning
(e.g., fmt.Sprintf("failed to stop anvil for '%s': %v", entry.Network, err)) to
that slice instead of printing, and ensure callers (the presenter/handler) read
ExitForkResult.Warnings and log or display them; specifically update the block
that calls uc.anvilManager.Stop(ctx, instance) to push the message into
ExitForkResult.Warnings when err != nil and remove the fmt.Printf usage.
In `@internal/usecase/fork_status.go`:
- Around line 72-76: state.Forks is a map and iterating over it produces
non-deterministic ordering; update the loop in the function that builds the
slice (where uc.buildStatusEntry is called) to first collect the map keys
(network names), sort them (lexicographically or by desired network order), then
iterate the sorted keys to call uc.buildStatusEntry(ctx, entry, currentNetwork)
and append to entries so the resulting []ForkStatusEntry has deterministic,
sorted order for CLI output.
- Around line 142-160: The helper function loadDeploymentIDs is shared by
show_deployment.go and fork_status.go and should be moved into a common utility
file: create a new file (e.g., fork_utils.go) in the same package and cut/paste
the loadDeploymentIDs function there, preserving its signature and behavior;
update imports if needed (json, os) and ensure both show_deployment.go and
fork_status.go continue to call loadDeploymentIDs (no API change), keeping the
function unexported unless you intend to use it outside the package.
In `@internal/usecase/revert_fork.go`:
- Around line 99-101: The code currently ignores errors from os.RemoveAll when
deleting snapshotDir; update the removal to capture the returned error from
os.RemoveAll(snapshotDir) and emit a non-fatal log (e.g., uc.logger.Warnf or
uc.log.Warn depending on available logger) including snapshotDir,
uc.cfg.ProjectRoot, entry.Network and topSnapshot.Index to aid debugging; do not
change the revert outcome—only log the failure (or return a warning value) so
orphaned directories or permission issues are visible.
- Around line 100-101: Replace direct filesystem calls to os.RemoveAll in
revert_fork.go with the ForkFileManager abstraction: add a new method to the
ForkFileManager interface such as RemoveSnapshotDir(ctx context.Context, network
string, index int64) (or reuse an existing CleanupForkDir variant) and call
uc.fileManager.RemoveSnapshotDir(ctx, entry.Network, topSnapshot.Index) instead
of computing snapshotDir and calling os.RemoveAll; do the same replacement in
the revertAll code path (lines referenced around 148-149) so all snapshot
directory deletions go through the ForkFileManager implementation.
In `@internal/usecase/run_script.go`:
- Around line 302-308: Extract the duplicated AnvilInstance construction into a
helper function (e.g., buildAnvilInstanceFromFork) that takes a
*domain.ForkEntry and returns *domain.AnvilInstance; replace the inline
constructions in checkForkHealth and takePreRunSnapshot with calls to
buildAnvilInstanceFromFork(fork) so Port, Name, ChainID, PidFile and LogFile are
created consistently (use portFromURL and fmt.Sprintf as in the originals inside
the helper).
In `@internal/usecase/show_deployment.go`:
- Around line 114-115: The call to loadDeploymentIDs is valid only because both
files share the same package; confirm loadDeploymentIDs remains unexported and
accessible, then eliminate the duplicated filepath.Join pattern by adding a
helper function getSnapshotBackupPath(dataDir, networkName, snapshotIndex
string) string and replace the inline construction in show_deployment.go (where
backupPath is built from uc.config.DataDir, "priv", "fork", networkName,
"snapshots", "0", "deployments.json") and other files to call
getSnapshotBackupPath(uc.config.DataDir, networkName, "0") before calling
loadDeploymentIDs; keep the helper name and parameters consistent across callers
so future changes centralize the path format.
In `@scripts/ralph/progress.txt`:
- Line 276: Update the wording in the sentence describing the isForkDeployment
method: change the phrase "exact same pattern" to "same pattern" so the sentence
reads that isForkDeployment follows the same pattern as computeForkDeploymentIDs
in ListDeployments but checks a single deployment ID instead of computing the
full set; locate the sentence referencing the isForkDeployment method and
computeForkDeploymentIDs and make this simple text edit.
In `@test/integration/fork_test.go`:
- Around line 340-353: The RPC helpers ethGetCode and ethGetBalance use the
package-level http.Post which lacks a timeout; add a reusable http.Client with a
timeout (e.g., rpcClient := &http.Client{Timeout: 5 * time.Second}) and replace
calls to http.Post with rpcClient.Post in the ethGetCode and ethGetBalance
helpers (and the other helper at the cited lines) so RPC calls fail fast if the
Anvil instance is unresponsive.
In `@test/testdata/project/.gitignore`:
- Line 15: The .gitignore contains overlapping rules: `.treb` and `.treb/priv/`;
decide which behavior you want and remove the redundant entry — either keep
`.treb` to ignore the entire directory or keep only `.treb/priv/` to ignore only
the private subtree; update the file by deleting the unneeded rule (`.treb` or
`.treb/priv/`) so the ignore rules are unambiguous.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (52)
internal/adapters/anvil/manager.gointernal/adapters/anvil/manager_test.gointernal/adapters/forge/forge.gointernal/adapters/forge/forge_test.gointernal/adapters/fs/fork_file_manager.gointernal/adapters/fs/fork_file_manager_test.gointernal/adapters/fs/fork_state_store.gointernal/adapters/fs/fork_state_store_test.gointernal/adapters/providers.gointernal/app/app.gointernal/app/wire.gointernal/app/wire_gen.gointernal/cli/config.gointernal/cli/fork.gointernal/cli/list.gointernal/cli/prune.gointernal/cli/register.gointernal/cli/render/deployment.gointernal/cli/render/deployments.gointernal/cli/render/fork.gointernal/cli/render/script.gointernal/cli/root.gointernal/cli/show.gointernal/cli/sync.gointernal/cli/verify.gointernal/config/rpc_env.gointernal/config/rpc_env_test.gointernal/domain/anvil.gointernal/domain/config/local.gointernal/domain/fork.gointernal/domain/fork_test.gointernal/usecase/diff_fork.gointernal/usecase/enter_fork.gointernal/usecase/exit_fork.gointernal/usecase/fork_history.gointernal/usecase/fork_status.gointernal/usecase/list_deployments.gointernal/usecase/ports.gointernal/usecase/remove_config.gointernal/usecase/restart_fork.gointernal/usecase/revert_fork.gointernal/usecase/run_script.gointernal/usecase/set_config.gointernal/usecase/show_deployment.goscripts/ralph/prd.jsonscripts/ralph/progress.txttest/integration/fork_test.gotest/integration/list_test.gotest/testdata/project/.gitignoretest/testdata/project/script/setup/SetupFork.s.soltest/testdata/project/script/setup/SetupForkFailing.s.soltreb-sol
Comment on lines
+119
to
+132
| func newMockRPCServer(t *testing.T, handler func(req rpcRequest) rpcResponse) *httptest.Server { | ||
| t.Helper() | ||
| return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| var req rpcRequest | ||
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | ||
| t.Fatalf("failed to decode RPC request: %v", err) | ||
| } | ||
| resp := handler(req) | ||
| w.Header().Set("Content-Type", "application/json") | ||
| if err := json.NewEncoder(w).Encode(resp); err != nil { | ||
| t.Fatalf("failed to encode RPC response: %v", err) | ||
| } | ||
| })) | ||
| } |
There was a problem hiding this comment.
Avoid t.Fatalf inside HTTP handler goroutine.
Calling t.Fatalf from a goroutine other than the test goroutine can cause unexpected behavior—the test may not fail cleanly. Consider using t.Error and returning early, or use a channel to communicate errors back to the test goroutine.
🛡️ Proposed fix
func newMockRPCServer(t *testing.T, handler func(req rpcRequest) rpcResponse) *httptest.Server {
t.Helper()
return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
var req rpcRequest
if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
- t.Fatalf("failed to decode RPC request: %v", err)
+ t.Errorf("failed to decode RPC request: %v", err)
+ http.Error(w, "bad request", http.StatusBadRequest)
+ return
}
resp := handler(req)
w.Header().Set("Content-Type", "application/json")
if err := json.NewEncoder(w).Encode(resp); err != nil {
- t.Fatalf("failed to encode RPC response: %v", err)
+ t.Errorf("failed to encode RPC response: %v", err)
+ return
}
}))
}📝 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.
Suggested change
| func newMockRPCServer(t *testing.T, handler func(req rpcRequest) rpcResponse) *httptest.Server { | |
| t.Helper() | |
| return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| var req rpcRequest | |
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | |
| t.Fatalf("failed to decode RPC request: %v", err) | |
| } | |
| resp := handler(req) | |
| w.Header().Set("Content-Type", "application/json") | |
| if err := json.NewEncoder(w).Encode(resp); err != nil { | |
| t.Fatalf("failed to encode RPC response: %v", err) | |
| } | |
| })) | |
| } | |
| func newMockRPCServer(t *testing.T, handler func(req rpcRequest) rpcResponse) *httptest.Server { | |
| t.Helper() | |
| return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| var req rpcRequest | |
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | |
| t.Errorf("failed to decode RPC request: %v", err) | |
| http.Error(w, "bad request", http.StatusBadRequest) | |
| return | |
| } | |
| resp := handler(req) | |
| w.Header().Set("Content-Type", "application/json") | |
| if err := json.NewEncoder(w).Encode(resp); err != nil { | |
| t.Errorf("failed to encode RPC response: %v", err) | |
| return | |
| } | |
| })) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/adapters/anvil/manager_test.go` around lines 119 - 132, In
newMockRPCServer's HTTP handler, avoid calling t.Fatalf from the handler
goroutine; instead create an error channel (e.g., errCh := make(chan error, 1)
in newMockRPCServer or accept a testing-safe reporter) and on JSON decode/encode
errors send the error into that channel and return early (or call t.Error/t.Log
on the main test goroutine by reading the channel), then let the test read from
errCh after exercising the server and fail the test there; update the handler's
json.Decode and json.Encode error paths to send errors to the channel (or call
t.Error) rather than calling t.Fatalf directly so failures are reported from the
test goroutine.
Comment on lines
+318
to
+321
| // Fork mode: override RPC env vars so foundry.toml ${VAR} resolves to the fork URL | ||
| for k, v := range config.ForkEnvOverrides { | ||
| env[k] = v | ||
| } |
There was a problem hiding this comment.
Avoid logging raw fork override env values (RPC secret leakage risk).
This change injects fork RPC overrides into the subprocess env; those values are currently logged unredacted at Line 67 ("env", env), which can expose provider keys/tokens in debug logs.
🔐 Proposed fix (log env keys only)
- f.log.Debug("Running forge script", "args", args, "env", env)
+ f.log.Debug("Running forge script", "args", args, "envKeys", envKeys(env))+func envKeys(env []string) []string {
+ keys := make([]string, 0, len(env))
+ for _, kv := range env {
+ if idx := strings.IndexByte(kv, '='); idx > 0 {
+ keys = append(keys, kv[:idx])
+ }
+ }
+ sort.Strings(keys)
+ return keys
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/adapters/forge/forge.go` around lines 318 - 321, The loop that
merges config.ForkEnvOverrides into env (using config.ForkEnvOverrides and the
env map in forge.go) risks leaking RPC secrets because the code later logs the
entire env with values; change the logging to never include raw secret
values—either log only the keys or build a redacted copy of env where values for
any key present in config.ForkEnvOverrides (and any likely secret keys) are
replaced with "<redacted>" before calling the logger. Keep the assignment
env[k]=v as-is but ensure the logger call that currently logs env uses the
redacted map or a keys-only list instead of the real env values.
Comment on lines
+340
to
+347
| network := "" | ||
| if len(args) > 0 { | ||
| network = args[0] | ||
| } | ||
|
|
||
| params := usecase.ForkHistoryParams{ | ||
| Network: network, | ||
| } |
There was a problem hiding this comment.
Inconsistent network fallback in runForkHistory.
Unlike runForkRevert, runForkRestart, and runForkExit, the runForkHistory function doesn't fall back to the configured network when no argument is provided. This is inconsistent with the other fork commands.
🐛 Proposed fix
func runForkHistory(cmd *cobra.Command, args []string) error {
app, err := getApp(cmd)
if err != nil {
return err
}
ctx := cmd.Context()
network := ""
if len(args) > 0 {
network = args[0]
+ } else {
+ // Use current configured network
+ if app.Config.Network != nil {
+ network = app.Config.Network.Name
+ }
}
params := usecase.ForkHistoryParams{
Network: network,
}📝 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.
Suggested change
| network := "" | |
| if len(args) > 0 { | |
| network = args[0] | |
| } | |
| params := usecase.ForkHistoryParams{ | |
| Network: network, | |
| } | |
| network := "" | |
| if len(args) > 0 { | |
| network = args[0] | |
| } else { | |
| // Use current configured network | |
| if app.Config.Network != nil { | |
| network = app.Config.Network.Name | |
| } | |
| } | |
| params := usecase.ForkHistoryParams{ | |
| Network: network, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cli/fork.go` around lines 340 - 347, runForkHistory currently sets
network to empty when no arg is provided, causing inconsistent behavior with
runForkRevert/runForkRestart/runForkExit; change the logic so it defaults to the
configured network (same as the other fork commands). Initialize network from
the command config (e.g., cfg.Network) and only override it when len(args) > 0,
then pass that network into usecase.ForkHistoryParams.Network so runForkHistory
behaves consistently with the other fork handlers.
Comment on lines
+103
to
+105
| cmd.Flags().BoolVar(&forkOnly, "fork", false, "Show only fork-added deployments") | ||
| cmd.Flags().BoolVar(&noFork, "no-fork", false, "Show only pre-fork deployments") | ||
| cmd.Flags().BoolVar(&jsonOutput, "json", false, "Output in JSON format") |
There was a problem hiding this comment.
Missing mutual exclusivity validation for --fork and --no-fork flags.
The --fork and --no-fork flags are logically mutually exclusive, but there's no validation preventing both from being specified simultaneously. This could lead to confusing behavior.
🛡️ Proposed fix to add flag validation
Add validation in the RunE function after parsing flags:
// Convert string type to domain type
+ if forkOnly && noFork {
+ return fmt.Errorf("cannot specify both --fork and --no-fork")
+ }
+
var deploymentType models.DeploymentType🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cli/list.go` around lines 103 - 105, The flags forkOnly and noFork
are mutually exclusive but not validated; in the command's RunE handler (the
function that executes after cmd.Flags() parsing), add a check that if forkOnly
&& noFork then return a user-facing error (e.g., fmt.Errorf("flags --fork and
--no-fork are mutually exclusive")) so the command fails fast; locate the flags
defined with cmd.Flags().BoolVar(&forkOnly, "fork", ...) and
cmd.Flags().BoolVar(&noFork, "no-fork", ...) and add this validation at the
start of the RunE function (or use Cobra's MarkFlagsMutuallyExclusive on those
two flag names) to prevent both being specified together.
Comment on lines
+141
to
+146
| data, err := json.MarshalIndent(entries, "", " ") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| fmt.Println(string(data)) | ||
| return nil |
There was a problem hiding this comment.
JSON output writes to stdout instead of cmd.OutOrStdout().
The renderListJSON function uses fmt.Println to write output, while the color-based renderer at line 92 uses cmd.OutOrStdout(). This inconsistency could cause issues with output redirection or testing.
🔧 Proposed fix to use consistent output writer
-func renderListJSON(result *usecase.DeploymentListResult) error {
+func renderListJSON(out io.Writer, result *usecase.DeploymentListResult) error {
entries := make([]listJSONEntry, 0, len(result.Deployments))
for _, dep := range result.Deployments {
entry := listJSONEntry{
ID: dep.ID,
ContractName: dep.ContractName,
Address: dep.Address,
Namespace: dep.Namespace,
ChainID: dep.ChainID,
Label: dep.Label,
Type: string(dep.Type),
}
if result.ForkDeploymentIDs != nil && result.ForkDeploymentIDs[dep.ID] {
entry.Fork = true
}
entries = append(entries, entry)
}
data, err := json.MarshalIndent(entries, "", " ")
if err != nil {
return err
}
- fmt.Println(string(data))
+ fmt.Fprintln(out, string(data))
return nil
}And update the call site:
// JSON output
if jsonOutput {
- return renderListJSON(result)
+ return renderListJSON(cmd.OutOrStdout(), result)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cli/list.go` around lines 141 - 146, The JSON renderer
renderListJSON currently prints via fmt.Println; change it to write to the Cobra
command's output writer by using cmd.OutOrStdout() as the io.Writer (e.g., get w
:= cmd.OutOrStdout() and write the json.MarshalIndent result with fmt.Fprintln
or w.Write), keeping the same error handling and return values so output
redirection/testing behaves like the color renderer that already uses
cmd.OutOrStdout().
Comment on lines
+120
to
+125
| entry := fmt.Sprintf("%s=%s", envVarName, value) | ||
|
|
||
| // Check if already exists | ||
| if strings.Contains(string(existing), envVarName+"=") { | ||
| return nil // Already present | ||
| } |
There was a problem hiding this comment.
Duplicate detection may produce false positives.
The check strings.Contains(string(existing), envVarName+"=") could match partial variable names. For example, if checking for MY_VAR= and the file contains MY_VAR_EXTRA=value, it would incorrectly consider MY_VAR as already present.
🛡️ Proposed fix using line-based matching
// Check if already exists
- if strings.Contains(string(existing), envVarName+"=") {
- return nil // Already present
+ lines := strings.Split(string(existing), "\n")
+ for _, line := range lines {
+ if strings.HasPrefix(line, envVarName+"=") {
+ return nil // Already present
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/config/rpc_env.go` around lines 120 - 125, The current duplicate
check uses strings.Contains(existing, envVarName+"=") which can yield false
positives (matches MY_VAR_EXTRA). In the function handling
envVarName/entry/existing, replace that check with a line-based match: split
existing by lines (or use a regexp anchored at line start) and verify any line
begins with envVarName+"=" (or matches `^envVarName=`) before returning; only
treat as present when a full variable name match is found. Update the same
function that builds entry to use this new check to avoid partial-name
collisions.
Comment on lines
+74
to
+78
| // Find available port | ||
| port, err := getAvailablePort() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to find available port: %w", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the enter_fork.go file to understand the port allocation logic
cat -n internal/usecase/enter_fork.go | head -100Repository: trebuchet-org/treb-cli
Length of output: 3753
🏁 Script executed:
# Look for the getAvailablePort function definition
rg "func.*getAvailablePort" -A 15Repository: trebuchet-org/treb-cli
Length of output: 2083
🏁 Script executed:
# Check how AnvilManager.Start uses the port parameter
rg "Start\(" internal/adapters -A 10Repository: trebuchet-org/treb-cli
Length of output: 7345
🏁 Script executed:
# Examine lines 242-250 to see if similar pattern exists
sed -n '240,260p' internal/usecase/enter_fork.goRepository: trebuchet-org/treb-cli
Length of output: 699
🌐 Web query:
Anvil Foundry port 0 automatic port selection support
💡 Result:
Foundry’s Anvil exposes --port as a plain u16 CLI argument (default 8545)—so 0 is a valid value to pass from the CLI/parser’s point of view. [1]
However, Anvil doesn’t document any special “auto port selection” behavior (or a machine-readable way to retrieve an OS-assigned ephemeral port) in its user-facing docs/README—so in practice, if you need “automatic port selection” and you must know the chosen port, the common approach is to pick a free port yourself (via a small pre-bind / free-port check) and then start Anvil with that explicit port. [2], [3]
Sources: [1] (Anvil CLI args show port: u16) [2] (Foundry/Anvil docs entry point) [3] (Example wrapper that finds a free port then launches Anvil)
Real race condition in port allocation: getAvailablePort() closes the listener before Anvil starts.
The getAvailablePort() function creates a binding-release-reuse gap: it binds to port 0 (OS assigns an ephemeral port), retrieves it, closes the listener, and returns the port number. Between listener.Close() and uc.anvilManager.Start() (line 96), another process could bind to the same port.
While Anvil accepts --port 0 as a CLI parameter, it does not support automatic port selection—it requires an explicit port number. Consider:
- Passing the open listener directly to
AnvilManager.Startif feasible - Retrying with a new port if binding fails
- Accepting the low practical risk on localhost (the time window is milliseconds)
Also applies to: 242-250
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/usecase/enter_fork.go` around lines 74 - 78, getAvailablePort()
currently binds to an ephemeral port, closes the listener, and returns the port
number, creating a race where another process can take the port before
uc.anvilManager.Start(...) binds it; fix by eliminating the bind-release gap:
modify uc.enterFork flow to either pass the open net.Listener (from
getAvailablePort or a new getAvailableListener) into AnvilManager.Start
(preferential) or, if Start cannot accept a listener, wrap uc.anvilManager.Start
in a retry loop that attempts to Start with a fresh port on EADDRINUSE until
success (use a small backoff and limit retries); update
getAvailablePort/getAvailableListener and AnvilManager.Start signatures
accordingly and ensure the code paths at the other occurrence (lines around
242-250) use the same approach.
Comment on lines
+184
to
+188
| localCfg, err := uc.localConfig.Load(ctx) | ||
| if err != nil { | ||
| // No config file - skip silently | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
Error swallowing may hide real configuration issues.
The current code treats all errors from uc.localConfig.Load() as "no config file", but this could mask actual problems like permission errors or malformed configuration files.
Consider distinguishing between "file not found" (skip silently) and other errors (surface to user).
🛡️ Proposed fix
localCfg, err := uc.localConfig.Load(ctx)
if err != nil {
- // No config file - skip silently
- return false, nil
+ // Only skip if config file doesn't exist; surface other errors
+ if os.IsNotExist(err) {
+ return false, nil
+ }
+ return false, fmt.Errorf("failed to load local config: %w", err)
}Note: This assumes LocalConfigRepository.Load returns the underlying os error. If it wraps errors differently, you may need to use errors.Is() or check for a specific error type.
📝 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.
Suggested change
| localCfg, err := uc.localConfig.Load(ctx) | |
| if err != nil { | |
| // No config file - skip silently | |
| return false, nil | |
| } | |
| localCfg, err := uc.localConfig.Load(ctx) | |
| if err != nil { | |
| // Only skip if config file doesn't exist; surface other errors | |
| if os.IsNotExist(err) { | |
| return false, nil | |
| } | |
| return false, fmt.Errorf("failed to load local config: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/usecase/enter_fork.go` around lines 184 - 188, The current handler
swallows all errors from uc.localConfig.Load(ctx); change it to distinguish
"file not found" vs other errors by checking the error with errors.Is(err,
fs.ErrNotExist) (or os.IsNotExist via the underlying error) and only treat
not-found as a silent skip (return false, nil), while returning other errors up
(return false, err); update the code around uc.localConfig.Load(ctx) and related
localCfg usage accordingly, using errors.Is to handle wrapped errors from
LocalConfigRepository.Load.
Comment on lines
+95
to
+131
| func (uc *ListDeployments) computeForkDeploymentIDs(ctx context.Context) map[string]bool { | ||
| state, err := uc.forkState.Load(ctx) | ||
| if err != nil { | ||
| return nil | ||
| } | ||
|
|
||
| // Determine network name | ||
| networkName := "" | ||
| if uc.config.Network != nil { | ||
| networkName = uc.config.Network.Name | ||
| } | ||
| if networkName == "" { | ||
| return nil | ||
| } | ||
|
|
||
| if !state.IsForkActive(networkName) { | ||
| return nil | ||
| } | ||
|
|
||
| // Load current deployment IDs | ||
| currentPath := filepath.Join(uc.config.DataDir, "deployments.json") | ||
| currentIDs := loadDeploymentIDs(currentPath) | ||
|
|
||
| // Load initial backup deployment IDs (snapshot 0) | ||
| backupPath := filepath.Join(uc.config.DataDir, "priv", "fork", networkName, "snapshots", "0", "deployments.json") | ||
| backupIDs := loadDeploymentIDs(backupPath) | ||
|
|
||
| // Compute fork-added IDs (in current but not in backup) | ||
| forkIDs := make(map[string]bool) | ||
| for id := range currentIDs { | ||
| if !backupIDs[id] { | ||
| forkIDs[id] = true | ||
| } | ||
| } | ||
|
|
||
| return forkIDs | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and check the exact content at the specified lines
cat -n internal/usecase/list_deployments.go | sed -n '95,131p'Repository: trebuchet-org/treb-cli
Length of output: 1255
🏁 Script executed:
# Search for loadDeploymentIDs function definition
ast-grep --pattern 'func loadDeploymentIDs($_) $_ {
$$$
}'Repository: trebuchet-org/treb-cli
Length of output: 1105
🏁 Script executed:
# Also search with rg as a fallback to find any reference to loadDeploymentIDs
rg 'loadDeploymentIDs' --type go -B 2 -A 5Repository: trebuchet-org/treb-cli
Length of output: 3602
Return empty map instead of nil to match loadDeploymentIDs error handling.
The loadDeploymentIDs helper function (defined in fork_status.go, same package) already handles missing or corrupted files gracefully by returning an empty map. However, when forkState.Load(ctx) fails at line 97, this method returns nil instead. This inconsistency could cause nil pointer dereference if the caller doesn't explicitly check for nil. For consistency and safety, return make(map[string]bool) instead of nil when the fork state load fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/usecase/list_deployments.go` around lines 95 - 131, In
computeForkDeploymentIDs, when uc.forkState.Load(ctx) returns an error you
currently return nil; change that to return an empty map (make(map[string]bool))
to match loadDeploymentIDs' behavior and avoid potential nil derefs. Update the
early return in the error branch after uc.forkState.Load(ctx) to return
make(map[string]bool); keep other logic (networkName checks and active-fork
check) unchanged.
The submodule was pointing to commit 204a0a6 on the feat/fork-helper branch which hasn't been merged to treb-sol main yet, causing CI checkout failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Points to 6a497e0 which includes the merged ForkHelper contract. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The args slice is constructed internally, not from user input. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Config golden files: add fork.setup to available keys list - Show golden files: add --no-fork flag to help output - Increase command timeout from 30s to 60s for CI reliability 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
2 participants
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
treb forkcommand suite (enter,exit,status,history,revert,restart,diff) for testing deployment scripts against local long-running Anvil forks of live networks${VAR_NAME}expansion are overridden in the forge subprocess environment only, sovm.rpcUrl()andvm.createFork()resolve to the fork transparently.treb/priv/fork/<network>/snapshots/) enables full revert of both on-chain state and registry fileslistandshowcommands display fork-mode deployments in a separate sectionverify,sync,prune,register, andconfig set networkfrom running in fork modefork restartfor recoveryTest plan
test/integration/fork_test.gocover all fork commandstreb fork enter <network>, run scripts,treb fork status,treb fork revert,treb fork exittreb listshows fork deployments separatelyverify/sync/prune/registerin fork mode🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation