perf: optimize contract indexing and add performance instrumentation#23
Merged
perf: optimize contract indexing and add performance instrumentation#23
Conversation
📝 WalkthroughWalkthroughAdds timing and logging around Forge builds and contract indexing, introduces an internal artifact-processing function that parses Foundry JSON artifacts to index contracts by path:name and by name, and guards/verifies Forge builds based on existing Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller as RepositoryIndexer
participant Repo as ContractsRepository
participant FS as Filesystem
participant Forge as ForgeAdapter
participant Log as Logger
Caller->>Repo: Index()
Note over Repo: start timer
Repo->>FS: Check out/ exists and has artifacts?
alt Artifacts exist
Repo->>Log: Debug: skip forge build
else Missing
Repo->>Log: Debug: running forge build
Repo->>Forge: Build()
Forge-->>Repo: output, err (Forge logs duration)
alt Build failed
Repo->>Log: Error with duration
Repo-->>Caller: error
end
Repo->>FS: Verify out/ exists
alt Missing
Repo-->>Caller: error (out missing)
end
end
loop For each artifact JSON in out/
Repo->>FS: Read artifact
Repo->>Repo: processArtifact(artifact)
Note right of Repo: parse JSON, extract name/source<br/>index by path:name and by name
end
Repo->>Log: Info: indexing complete (count, duration)
Repo-->>Caller: result
sequenceDiagram
autonumber
actor Caller
participant Forge as ForgeAdapter
participant Shell as Shell
participant Log as Logger
Caller->>Forge: Build()
Note over Forge: start := time.Now()
Forge->>Shell: Run forge build command
Shell-->>Forge: combined output, err
alt Error
Forge->>Log: Error with duration and output
Forge-->>Caller: output, err
else Success
Forge->>Log: Success with duration
Forge-->>Caller: output, nil
end
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/adapters/repository/contracts/repository.go (1)
258-273: GetContractByArtifact matches name and “path:name”, not the artifact path.This likely returns wrong contracts. Compare against ArtifactPath instead.
Outside the changed hunk, update the method as:
func (i *Repository) GetContractByArtifact(ctx context.Context, artifact string) *models.Contract { if err := i.Index(); err != nil { panic(err) } i.mu.RLock() defer i.mu.RUnlock() for _, c := range i.contracts { if c.ArtifactPath == artifact { return c } } return nil }
🧹 Nitpick comments (6)
internal/adapters/forge/forge.go (1)
38-56: Add timeout/cancellation to prevent hung builds.Use CommandContext with a reasonable timeout and log if the build timed out. Keeps your instrumentation and return types unchanged.
func (f *ForgeAdapter) Build() error { - start := time.Now() + start := time.Now() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) + defer cancel() f.log.Debug("running forge build", "dir", f.projectRoot) - cmd := exec.Command("forge", "build") + cmd := exec.CommandContext(ctx, "forge", "build") cmd.Dir = f.projectRoot output, err := cmd.CombinedOutput() duration := time.Since(start) if err != nil { - f.log.Error("forge build failed", "error", err, "output", string(output), "duration", duration) + f.log.Error("forge build failed", "error", err, "output", string(output), "duration", duration, "timeout", ctx.Err() == context.DeadlineExceeded) // Only print error details if build actually failed return fmt.Errorf("forge build failed: %w\nOutput: %s", err, string(output)) } f.log.Debug("forge build completed successfully", "duration", duration) // Don't print anything on success - let the caller handle UI return nil }internal/adapters/repository/contracts/repository.go (5)
124-142: Add timeout/cancellation and include output in error log for runForgeBuild.Prevents hung builds and improves diagnostics. Keeps behavior otherwise.
func (i *Repository) runForgeBuild() error { start := time.Now() i.log.Debug("running forge build for contract indexing", "dir", i.projectRoot) // Check if we need to rebuild by looking for a cache indicator // For now, always build without --force to improve performance // The --force flag was causing significant slowdowns - cmd := exec.Command("forge", "build") + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) + defer cancel() + cmd := exec.CommandContext(ctx, "forge", "build") cmd.Dir = i.projectRoot output, err := cmd.CombinedOutput() duration := time.Since(start) if err != nil { - i.log.Error("forge build failed", "error", err, "duration", duration) - return fmt.Errorf("forge build failed: %w\nOutput: %s", err, string(output)) + i.log.Error("forge build failed", "error", err, "duration", duration, "timeout", ctx.Err() == context.DeadlineExceeded, "output", string(output)) + return fmt.Errorf("forge build failed: %w\nOutput: %s", err, string(output)) } i.log.Debug("forge build completed", "duration", duration) return nil }
236-237: Avoid panics on invalid regex patterns.regexp.MustCompile will panic on user input. Prefer Compile and handle the error gracefully.
- pathRegex = regexp.MustCompile(*query.PathPattern) + if rx, err := regexp.Compile(*query.PathPattern); err == nil { + pathRegex = rx + } else { + i.log.Debug("invalid path regex; ignoring", "error", err) + }
166-175: CompilationTarget iteration assumes exactly one entry. Add a defensive check/log.If the artifact is malformed, silently skipping hides issues; consider a debug log for visibility.
25-31: Bytecode hash index never populated.You declare bytecodeHashIndex but never fill it; indexing by bytecode can be useful for lookup by deployed bytecode.
I can add this in processArtifact if you expose the hash on models.Artifact.
62-86: Consider delegating builds to ForgeAdapter for consistency.If feasible, wire Repository to use ForgeAdapter.Build() to keep logging, timeouts, and behavior centralized.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/adapters/forge/forge.go(1 hunks)internal/adapters/repository/contracts/repository.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-26T10:13:23.572Z
Learnt from: CR
PR: trebuchet-org/treb-cli#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-26T10:13:23.572Z
Learning: Applies to cli/pkg/forge/**/*.go : Implement Forge command execution and integration under cli/pkg/forge/
Applied to files:
internal/adapters/forge/forge.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Integration Tests
- GitHub Check: Test
- GitHub Check: Integration Tests (macOS)
- GitHub Check: Lint
🔇 Additional comments (3)
internal/adapters/forge/forge.go (1)
45-54: Instrumentation LGTM.Duration capture and structured logging on both paths look good.
internal/adapters/repository/contracts/repository.go (2)
52-55: Index fast-path LGTM.The in-memory cache short-circuit on subsequent calls is correct and keeps the lock scope tight.
45-61: Timing instrumentation LGTM.Start/stop timing and structured “contracts_found” metric are helpful.
Comment on lines
88
to
91
| // Verify the 'out' directory exists | ||
| if _, err := os.Stat(outDir); os.IsNotExist(err) { | ||
| return fmt.Errorf("out directory not found after forge build") | ||
| return fmt.Errorf("out directory not found after attempting to index") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Post-build verification should ensure artifacts exist, not just the directory.
Return a clearer error if no artifacts were produced.
- // Verify the 'out' directory exists
- if _, err := os.Stat(outDir); os.IsNotExist(err) {
- return fmt.Errorf("out directory not found after attempting to index")
- }
+ // Verify artifacts exist after potential build
+ if ok, _ := hasArtifacts(outDir); !ok {
+ return fmt.Errorf("no artifacts found in out directory after indexing: %s", outDir)
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/adapters/repository/contracts/repository.go around lines 88–91, the
post-build verification currently only checks that the out directory exists;
change it to verify that the directory contains produced artifact files and
return a clearer error when none are found. Replace the os.Stat-only check with
a read of the directory contents (or a glob for expected artifact patterns),
ensure there is at least one file matching the artifact criteria (and optionally
that files are non-zero size), and on failure return an error that includes the
outDir path and states that no build artifacts were produced (e.g., "no
artifacts found in out directory: <path>").
Comment on lines
116
to
120
| i.indexed = true | ||
| duration := time.Since(start) | ||
| i.log.Debug("contract indexing completed", "duration", duration, "contracts_found", len(i.contracts)) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Don’t set indexed=true when Walk fails (risk of stale/partial index).
As written, a failed walk still marks the repo indexed, causing subsequent calls to skip re-indexing.
- i.indexed = true
+ if err == nil {
+ i.indexed = true
+ }
duration := time.Since(start)
i.log.Debug("contract indexing completed", "duration", duration, "contracts_found", len(i.contracts))
return err📝 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
| i.indexed = true | |
| duration := time.Since(start) | |
| i.log.Debug("contract indexing completed", "duration", duration, "contracts_found", len(i.contracts)) | |
| return err | |
| } | |
| if err == nil { | |
| i.indexed = true | |
| } | |
| duration := time.Since(start) | |
| i.log.Debug("contract indexing completed", "duration", duration, "contracts_found", len(i.contracts)) | |
| return err | |
| } |
🤖 Prompt for AI Agents
In internal/adapters/repository/contracts/repository.go around lines 116 to 120,
the code sets i.indexed = true even when the walk returned an error, which can
leave the index flagged as complete despite a failed/partial run; change the
logic so i.indexed is only set to true when err == nil (i.e., after a successful
walk), and on error keep i.indexed false (and return the error as before),
ensuring the debug log still records duration and contracts found only after a
successful run or log the failure separately before returning.
🛠️ Refactor suggestion
Promote unique contract names to simple keys to honor “name or path:name” lookups.
You populate contractNames but never create name-only entries in contracts when unique.
- i.indexed = true
+ // Promote unique names to simple keys
+ for name, list := range i.contractNames {
+ if len(list) == 1 {
+ i.contracts[name] = list[0]
+ } else {
+ delete(i.contracts, name)
+ }
+ }
+ i.indexed = true📝 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
| i.indexed = true | |
| duration := time.Since(start) | |
| i.log.Debug("contract indexing completed", "duration", duration, "contracts_found", len(i.contracts)) | |
| return err | |
| } | |
| // Promote unique names to simple keys | |
| for name, list := range i.contractNames { | |
| if len(list) == 1 { | |
| i.contracts[name] = list[0] | |
| } else { | |
| delete(i.contracts, name) | |
| } | |
| } | |
| i.indexed = true | |
| duration := time.Since(start) | |
| i.log.Debug("contract indexing completed", "duration", duration, "contracts_found", len(i.contracts)) | |
| return err | |
| } |
🤖 Prompt for AI Agents
In internal/adapters/repository/contracts/repository.go around lines 116 to 120,
the code builds contractNames but never promotes unique simple names into the
contracts map, so "name" lookups fail for uniquely-named contracts; after
populating contractNames, iterate the name->slice mapping and for any name with
exactly one entry, add a name-only key to the contracts map pointing to that
single contract (ensuring you don't overwrite existing full-path keys),
otherwise leave ambiguous names alone; implement this promotion before setting
i.indexed = true and logging so name-only lookups succeed.
…uild usage"" This reverts commit a60ed04.
solofberlin
force-pushed
the
feat/performance-optimization
branch
from
109d289 to
949cbc2
Compare
- Check for actual .json artifacts (excluding build-info) instead of any directory entry when deciding whether to skip forge build - Only mark index as complete when walk succeeds, allowing retry on failure - Post-build verification now checks for artifacts, not just directory existence Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The build-skip optimization (checking if out/ has artifacts) broke the gen→run workflow: after `treb gen deploy Counter` creates a new script, `treb run` would skip forge build because artifacts already existed, missing the uncompiled script. Forge's own cache (no --force) handles the fast-path when nothing changed, so always building is correct. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/adapters/repository/contracts/repository.go (2)
62-72:⚠️ Potential issue | 🟠 MajorPost-build validation is too weak; verify actual artifacts, not just
out/existence.This can succeed with no artifact JSONs (e.g., only debug files), leaving an empty/stale index without surfacing a hard failure.
Proposed fix
- // Verify the 'out' directory exists after build + // Verify artifact JSONs exist after build outDir := filepath.Join(i.projectRoot, "out") - if _, err := os.Stat(outDir); os.IsNotExist(err) { - return fmt.Errorf("out directory not found after forge build") + if ok, scanErr := hasArtifacts(outDir); scanErr != nil { + return fmt.Errorf("failed scanning artifacts in %s: %w", outDir, scanErr) + } else if !ok { + return fmt.Errorf("no artifacts found in out directory after forge build: %s", outDir) }func hasArtifacts(outDir string) (bool, error) { found := false err := filepath.Walk(outDir, func(path string, info os.FileInfo, err error) error { if err != nil { return err } if info.IsDir() { return nil } if filepath.Ext(path) == ".json" && !strings.Contains(path, "build-info") { found = true } return nil }) return found, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapters/repository/contracts/repository.go` around lines 62 - 72, The post-build check only tests that the out directory exists; update the logic after runForgeBuild() to verify actual compiled contract JSON artifacts by adding a helper (e.g., hasArtifacts(outDir string) (bool, error)) that walks outDir and returns true only if it finds files with extension ".json" excluding "build-info" files; call hasArtifacts after constructing outDir in the same function (where runForgeBuild is invoked) and return a descriptive error (including any walk error) if no artifacts are found so the build failure surfaces correctly.
176-184:⚠️ Potential issue | 🟠 MajorUnique-name indexing is incomplete, so
namelookups can fail.You collect
contractNames, buti.contractsnever gets the simplenamekey for unique contracts (and never removes it when names become ambiguous).Proposed fix
// Also index by name alone if unique if existingList, exists := i.contractNames[info.Name]; exists { // Multiple contracts with same name i.contractNames[info.Name] = append(existingList, info) - // Remove simple key if it exists + delete(i.contracts, info.Name) } else { // First contract with this name i.contractNames[info.Name] = []*models.Contract{info} + i.contracts[info.Name] = info }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapters/repository/contracts/repository.go` around lines 176 - 184, The current unique-name indexing only updates i.contractNames and never sets or removes the simple i.contracts[name] entry, causing name lookups to fail; inside the block handling info.Name (the shown if/else around i.contractNames), when creating the first entry (else branch) also set i.contracts[info.Name] = info so the simple lookup works, and in the branch where you append to existingList (duplicate names) remove the simple key (delete i.contracts[info.Name] or set it to nil) once the name becomes ambiguous (i.e., when existingList length transitions from 1 to 2) so the simple map no longer points to a single contract; ensure these updates occur atomically alongside changes to i.contractNames.
🧹 Nitpick comments (1)
internal/adapters/repository/contracts/repository.go (1)
97-102: Avoid “completed” wording when indexing returns an error.Current log message is emitted even when walk fails, which makes debug traces misleading.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapters/repository/contracts/repository.go` around lines 97 - 102, The current code always emits i.log.Debug("contract indexing completed", ...) even when err != nil, which is misleading; update the logic in the indexing routine (the block that sets i.indexed and computes duration using start) so that you only log a "completed" success message when err == nil and instead log an error or failure message (including err and duration) when err != nil; adjust the i.indexed assignment and the i.log.Debug/i.log.Error calls accordingly so success and failure are logged distinctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/adapters/repository/contracts/repository.go`:
- Around line 62-72: The post-build check only tests that the out directory
exists; update the logic after runForgeBuild() to verify actual compiled
contract JSON artifacts by adding a helper (e.g., hasArtifacts(outDir string)
(bool, error)) that walks outDir and returns true only if it finds files with
extension ".json" excluding "build-info" files; call hasArtifacts after
constructing outDir in the same function (where runForgeBuild is invoked) and
return a descriptive error (including any walk error) if no artifacts are found
so the build failure surfaces correctly.
- Around line 176-184: The current unique-name indexing only updates
i.contractNames and never sets or removes the simple i.contracts[name] entry,
causing name lookups to fail; inside the block handling info.Name (the shown
if/else around i.contractNames), when creating the first entry (else branch)
also set i.contracts[info.Name] = info so the simple lookup works, and in the
branch where you append to existingList (duplicate names) remove the simple key
(delete i.contracts[info.Name] or set it to nil) once the name becomes ambiguous
(i.e., when existingList length transitions from 1 to 2) so the simple map no
longer points to a single contract; ensure these updates occur atomically
alongside changes to i.contractNames.
---
Nitpick comments:
In `@internal/adapters/repository/contracts/repository.go`:
- Around line 97-102: The current code always emits i.log.Debug("contract
indexing completed", ...) even when err != nil, which is misleading; update the
logic in the indexing routine (the block that sets i.indexed and computes
duration using start) so that you only log a "completed" success message when
err == nil and instead log an error or failure message (including err and
duration) when err != nil; adjust the i.indexed assignment and the
i.log.Debug/i.log.Error calls accordingly so success and failure are logged
distinctly.
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
This PR adds performance instrumentation and optimizes the contract indexing process to avoid unnecessary forge builds.
Changes
🕐 Added timing instrumentation to track performance of expensive operations
ForgeAdapter.Build()logs duration of forge build operationsRepository.Index()logs duration of full contract indexingrunForgeBuild()logs duration of forge build subprocess🚀 Optimized contract indexing to skip unnecessary rebuilds
outdirectory exists and has content before triggering forge buildforge buildwhen artifacts are missing or directory is empty🔍 Debug visibility with
TREB_LOG_LEVEL=debugenvironment variablePerformance Impact
Testing shows dramatic performance improvements for repeated operations:
Testing
Future Improvements
--force-rebuildflag for when users want to bypass the cache🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores