Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpdates to linting/config and tests plus widespread context propagation changes: golangci-lint config and invocations expanded; executor and syncer now use caller-provided contexts; HTTP request handling and method constants standardized; small renames and test adjustments across the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant Producer as Producer (caller)
participant Executor as Executor
participant Raft as RaftBroadcaster
participant Store as StoreBroadcaster
Note over Producer,Executor: Block production with caller-provided ctx
Producer->>Executor: ProduceBlock(ctx, block)
Executor->>Raft: Broadcast(ctx, raftState) rgba(70,130,180,0.5)
Raft-->>Executor: BroadcastResult
Executor->>Store: WriteToStoreAndBroadcast(ctx, header) rgba(34,139,34,0.5)
Store-->>Executor: StoreAck
Executor->>Store: WriteToStoreAndBroadcast(ctx, data) rgba(34,139,34,0.5)
Store-->>Executor: DataAck
Executor-->>Producer: ProduceResult
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @tac0turtle's task in 2m 52s —— View job PR Review: chore: add stricter lintingTasks
OverviewThis PR adds stricter linting rules and fixes code to comply with them. The intent is good — linting as a guard-rail for AI-generated code is valuable — but there are several issues worth addressing, ranging from a self-defeating config to a real lifecycle bug in the syncer. 🔴 Critical Issues1. Conflicting linter config defeats the PR's goalFile: Six linters are both enabled and disabled in the same config: # Lines 15, 22, 23, 30, 38, 40 - enabled
enable:
- containedctx
- errcheck
- gocritic
- nolintlint
- testifylint
- unparam
# Lines 90–97 - then disabled!
disable:
- containedctx
- errcheck
- gocritic
- nolintlint
- testifylint
- unparamFix: Remove the conflicting entries from the 2. Syncer lifecycle bug:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Spamoor Trace Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: ed63711 | Previous: 042b75a | Ratio |
|---|---|---|---|
SpamoorSmoke - evm env (avg) |
7.78169014084507 us |
2.88339222614841 us |
2.70 |
This comment was automatically generated by workflow using github-action-benchmark.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
block/internal/syncing/syncer.go (1)
188-201:⚠️ Potential issue | 🔴 CriticalUse
s.ctxfor raftRetriever and worker initialization to ensure proper shutdown.Lines 188 and 201 pass the parent
ctxto components that aren't properly torn down. WhenStop()cancels onlys.ctx, theraftRetrievercontinues running (since it derives from the parent context), blockingwg.Wait()and leaving the goroutine alive.Suggested fix
- if err := s.raftRetriever.Start(ctx); err != nil { + if err := s.raftRetriever.Start(s.ctx); err != nil { return fmt.Errorf("start raft retriever: %w", err) } ... - s.startSyncWorkers(ctx) + s.startSyncWorkers(s.ctx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/syncing/syncer.go` around lines 188 - 201, The raft retriever and worker goroutines are started with the parent ctx causing them to outlive Stop() which only cancels s.ctx; change the start calls to use s.ctx instead of the passed-in parent ctx so they inherit the syncer's lifecycle. Specifically, call s.raftRetriever.Start(s.ctx) where s.raftRetriever.Start(ctx) is invoked and ensure s.startSyncWorkers uses s.ctx (not the parent ctx) so those workers are cancelled when Stop() cancels s.ctx, preserving proper wg.Wait() shutdown for the goroutines started in s.processLoop and startSyncWorkers.
🧹 Nitpick comments (1)
pkg/cmd/run_node_test.go (1)
667-671: Avoid invokingStartNodetwice in non-panic cases.Line 669 already executes the startup path via
runFunc(baseCtx), then Line 671 runsStartNodeagain. Consolidating to one invocation per test case will reduce flake surface.Proposed simplification
- runFunc := func(ctx context.Context) { + runFunc := func(ctx context.Context) error { currentTestLogger := zerolog.Nop() cmd.SetContext(ctx) - err := StartNode(currentTestLogger, cmd, executor, sequencer, nodeKey, ds, nodeConfig, testGenesis, node.NodeOptions{}) //nolint:contextcheck // test invokes command entrypoint directly + err := StartNode(currentTestLogger, cmd, executor, sequencer, nodeKey, ds, nodeConfig, testGenesis, node.NodeOptions{}) //nolint:contextcheck // test invokes command entrypoint directly if tc.expectedError != "" { assert.ErrorContains(t, err, tc.expectedError) } else { if !tc.expectPanic { // For the success case, we expect an error due to P2P issues, but the signer loading should work // The important thing is that we exercise the signer path resolution code assert.Error(t, err) // Will fail due to P2P, but signer loading succeeded } } + return err } if tc.expectPanic { - assert.Panics(t, func() { runFunc(baseCtx) }) + assert.Panics(t, func() { _ = runFunc(baseCtx) }) } else { - assert.NotPanics(t, func() { runFunc(baseCtx) }) - checkLogger := zerolog.Nop() - err := StartNode(checkLogger, cmd, executor, sequencer, nodeKey, ds, nodeConfig, testGenesis, node.NodeOptions{}) //nolint:contextcheck // test invokes command entrypoint directly - if tc.expectedError != "" { - assert.ErrorContains(t, err, tc.expectedError) - } + assert.NotPanics(t, func() { _ = runFunc(baseCtx) }) }As per coding guidelines "Ensure tests are deterministic".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/run_node_test.go` around lines 667 - 671, The test calls the startup path twice: runFunc(baseCtx) already exercises the node startup, then StartNode(...) is invoked again; remove the duplicate StartNode call and instead assert on the effects of runFunc(baseCtx) (or have runFunc call StartNode once and return any error/state to assert), so modify the test to only invoke runFunc(baseCtx) when expecting no panic and drop the subsequent StartNode(...) invocation (references: runFunc and StartNode).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.golangci.yml:
- Around line 12-43: The linter config enables containedctx, errcheck, gocritic,
nolintlint, testifylint and unparam earlier but then disables them later,
neutralizing the stricter rules; remove those linters from the later disable
list (containedctx, errcheck, gocritic, nolintlint, testifylint, unparam) so the
initial enables take effect, and ensure the associated per-linter settings for
errcheck and gocritic remain in their current config blocks (so their options
are honored).
In `@scripts/test.go`:
- Around line 32-33: Replace the use of exec.Command with exec.CommandContext
and a bounded context: create a context with a timeout (context.WithTimeout)
before building the command, pass that ctx to exec.CommandContext instead of
exec.Command (the current cmd := exec.Command line), keep setting cmd.Dir =
modDir as before, and ensure you defer cancel() so the process can be cancelled
to prevent hangs; also remove the nolint:noctx suppression on the original cmd
line.
---
Outside diff comments:
In `@block/internal/syncing/syncer.go`:
- Around line 188-201: The raft retriever and worker goroutines are started with
the parent ctx causing them to outlive Stop() which only cancels s.ctx; change
the start calls to use s.ctx instead of the passed-in parent ctx so they inherit
the syncer's lifecycle. Specifically, call s.raftRetriever.Start(s.ctx) where
s.raftRetriever.Start(ctx) is invoked and ensure s.startSyncWorkers uses s.ctx
(not the parent ctx) so those workers are cancelled when Stop() cancels s.ctx,
preserving proper wg.Wait() shutdown for the goroutines started in s.processLoop
and startSyncWorkers.
---
Nitpick comments:
In `@pkg/cmd/run_node_test.go`:
- Around line 667-671: The test calls the startup path twice: runFunc(baseCtx)
already exercises the node startup, then StartNode(...) is invoked again; remove
the duplicate StartNode call and instead assert on the effects of
runFunc(baseCtx) (or have runFunc call StartNode once and return any error/state
to assert), so modify the test to only invoke runFunc(baseCtx) when expecting no
panic and drop the subsequent StartNode(...) invocation (references: runFunc and
StartNode).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
.github/workflows/lint.yml.golangci.yml.just/lint.justblock/internal/da/async_block_retriever_test.goblock/internal/executing/executor.goblock/internal/submitting/da_submitter.goblock/internal/submitting/da_submitter_test.goblock/internal/syncing/syncer.goblock/internal/syncing/syncer_backoff_test.goblock/internal/syncing/syncer_benchmark_test.goblock/internal/syncing/syncer_test.gonode/failover.gonode/full.gonode/full_node_test.gonode/light.gopkg/cmd/run_node_test.gopkg/p2p/client.gopkg/p2p/client_test.gopkg/rpc/server/da_visualization_non_aggregator_test.gopkg/rpc/server/da_visualization_test.gopkg/rpc/server/http_test.gopkg/rpc/server/server_test.gopkg/store/store_adapter_test.gopkg/telemetry/tracing.goscripts/test.gotools/blob-decoder/main.gotools/local-da/rpc.go
💤 Files with no reviewable changes (2)
- block/internal/submitting/da_submitter_test.go
- pkg/p2p/client_test.go
| - asciicheck | ||
| - bidichk | ||
| - bodyclose | ||
| - containedctx | ||
| - contextcheck | ||
| - copyloopvar | ||
| - durationcheck | ||
| - errname | ||
| - errcheck | ||
| - errorlint | ||
| - gocritic | ||
| - govet | ||
| - ineffassign | ||
| - makezero | ||
| - gosec | ||
| - misspell | ||
| - nilerr | ||
| - noctx | ||
| - nolintlint | ||
| - prealloc | ||
| - predeclared | ||
| - reassign | ||
| - revive | ||
| - rowserrcheck | ||
| - sqlclosecheck | ||
| - staticcheck | ||
| - testifylint | ||
| - unconvert | ||
| - unparam | ||
| - unused | ||
| - usestdlibvars | ||
| - wastedassign |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .golangci.ymlRepository: evstack/ev-node
Length of output: 2986
🏁 Script executed:
#!/bin/bash
# Verify overlap between linters.enable and linters.disable in .golangci.yml
python - <<'PY'
import re
from pathlib import Path
lines = Path(".golangci.yml").read_text().splitlines()
in_linters = False
state = None
enable, disable = set(), set()
for line in lines:
if re.match(r'^linters:\s*$', line):
in_linters = True
state = None
continue
if in_linters and re.match(r'^[^\s].*:\s*$', line): # next top-level block
in_linters = False
state = None
if not in_linters:
continue
if re.match(r'^\s{2}enable:\s*$', line):
state = "enable"
continue
if re.match(r'^\s{2}disable:\s*$', line):
state = "disable"
continue
if re.match(r'^\s{2}[a-zA-Z0-9_-]+:\s*$', line) and not re.match(r'^\s{2}(enable|disable):\s*$', line):
state = None
continue
m = re.match(r'^\s{4}-\s+([a-zA-Z0-9_-]+)\s*$', line)
if not m:
continue
if state == "enable":
enable.add(m.group(1))
elif state == "disable":
disable.add(m.group(1))
overlap = sorted(enable & disable)
print("Enabled linters:", sorted(enable))
print("\nDisabled linters:", sorted(disable))
print("\nOverlapping linters:", overlap if overlap else "none")
PYRepository: evstack/ev-node
Length of output: 735
Conflicting linter configuration disables several newly enabled checks.
Lines 12-43 enable containedctx, errcheck, gocritic, nolintlint, testifylint, and unparam which are then disabled again on lines 90-97. This contradiction also makes the related settings on lines 45-60 (for errcheck and gocritic) ineffective, undermining the PR's stricter-linting objective.
Suggested cleanup (if stricter mode is intended)
disable:
- - containedctx
- - errcheck
- - gocritic
- - nolintlint
- - testifylint
- thelper
- tparallel
- - unparam
- wrapcheck🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.golangci.yml around lines 12 - 43, The linter config enables containedctx,
errcheck, gocritic, nolintlint, testifylint and unparam earlier but then
disables them later, neutralizing the stricter rules; remove those linters from
the later disable list (containedctx, errcheck, gocritic, nolintlint,
testifylint, unparam) so the initial enables take effect, and ensure the
associated per-linter settings for errcheck and gocritic remain in their current
config blocks (so their options are honored).
| cmd := exec.Command("go", "test", "./...", "-cover") //nolint:noctx // intentionally runs module-local test command | ||
| cmd.Dir = modDir // Set the working directory for the command |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, let's see the structure and content of scripts/test.go around lines 32-33
wc -l scripts/test.goRepository: evstack/ev-node
Length of output: 77
🏁 Script executed:
# Get the actual lines in context
head -n 50 scripts/test.go | tail -n 20Repository: evstack/ev-node
Length of output: 899
🏁 Script executed:
# Search for all exec.Command and exec.CommandContext usages
rg -n 'exec\.(Command|CommandContext)' scripts/test.goRepository: evstack/ev-node
Length of output: 180
🏁 Script executed:
# Check imports to see if context and time are already imported
head -n 20 scripts/test.goRepository: evstack/ev-node
Length of output: 474
Use exec.CommandContext to comply with context-based cancellation guidelines.
Line 32 suppresses the noctx linting, but this leaves the command vulnerable to hanging indefinitely. Apply a bounded context with timeout to ensure proper cancellation handling.
Suggested change
+import "context"
+import "time"
...
- cmd := exec.Command("go", "test", "./...", "-cover") //nolint:noctx // intentionally runs module-local test command
+ testCtx, cancel := context.WithTimeout(context.Background(), 30*time.Minute)
+ defer cancel()
+ cmd := exec.CommandContext(testCtx, "go", "test", "./...", "-cover")📝 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.
| cmd := exec.Command("go", "test", "./...", "-cover") //nolint:noctx // intentionally runs module-local test command | |
| cmd.Dir = modDir // Set the working directory for the command | |
| testCtx, cancel := context.WithTimeout(context.Background(), 30*time.Minute) | |
| defer cancel() | |
| cmd := exec.CommandContext(testCtx, "go", "test", "./...", "-cover") | |
| cmd.Dir = modDir // Set the working directory for the command |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/test.go` around lines 32 - 33, Replace the use of exec.Command with
exec.CommandContext and a bounded context: create a context with a timeout
(context.WithTimeout) before building the command, pass that ctx to
exec.CommandContext instead of exec.Command (the current cmd := exec.Command
line), keep setting cmd.Dir = modDir as before, and ensure you defer cancel() so
the process can be cancelled to prevent hangs; also remove the nolint:noctx
suppression on the original cmd line.
tac0turtle
force-pushed
the
marko/linting
branch
from
49d460c to
4400594
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
node/full_node_test.go (1)
43-67:⚠️ Potential issue | 🟠 MajorResource leak: first response body is never closed.
The
respvariable is reassigned on line 60, but the defer on lines 47-52 capturesrespby reference. When this defer executes,resppoints to the second response, so the first response body is never closed.🐛 Proposed fix using distinct variable names
- req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("http://%s/metrics", config.Instrumentation.PrometheusListenAddr), nil) - require.NoError(err, "Failed to create Prometheus metrics request") - resp, err := http.DefaultClient.Do(req) //nolint:gosec // test-only request to local instrumentation endpoint + reqMetrics, err := http.NewRequest(http.MethodGet, fmt.Sprintf("http://%s/metrics", config.Instrumentation.PrometheusListenAddr), nil) + require.NoError(err, "Failed to create Prometheus metrics request") + respMetrics, err := http.DefaultClient.Do(reqMetrics) //nolint:gosec // test-only request to local instrumentation endpoint require.NoError(err, "Failed to get Prometheus metrics") defer func() { - err := resp.Body.Close() + err := respMetrics.Body.Close() if err != nil { t.Logf("Error closing response body: %v", err) } }() - assert.Equal(http.StatusOK, resp.StatusCode, "Prometheus metrics endpoint should return 200 OK") - body, err := io.ReadAll(resp.Body) + assert.Equal(http.StatusOK, respMetrics.StatusCode, "Prometheus metrics endpoint should return 200 OK") + body, err := io.ReadAll(respMetrics.Body) require.NoError(err) assert.Contains(string(body), "# HELP", "Prometheus metrics body should contain HELP lines") - req, err = http.NewRequest(http.MethodGet, fmt.Sprintf("http://%s/debug/pprof/", config.Instrumentation.PprofListenAddr), nil) + reqPprof, err := http.NewRequest(http.MethodGet, fmt.Sprintf("http://%s/debug/pprof/", config.Instrumentation.PprofListenAddr), nil) require.NoError(err, "Failed to create pprof request") - resp, err = http.DefaultClient.Do(req) //nolint:gosec // test-only request to local instrumentation endpoint + respPprof, err := http.DefaultClient.Do(reqPprof) //nolint:gosec // test-only request to local instrumentation endpoint require.NoError(err, "Failed to get Pprof index") defer func() { - err := resp.Body.Close() + err := respPprof.Body.Close() if err != nil { t.Logf("Error closing response body: %v", err) } }() - assert.Equal(http.StatusOK, resp.StatusCode, "Pprof index endpoint should return 200 OK") - body, err = io.ReadAll(resp.Body) + assert.Equal(http.StatusOK, respPprof.StatusCode, "Pprof index endpoint should return 200 OK") + body, err = io.ReadAll(respPprof.Body) require.NoError(err) assert.Contains(string(body), "Types of profiles available", "Pprof index body should contain expected text")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/full_node_test.go` around lines 43 - 67, The test reuses the single resp variable for two HTTP calls (metrics and pprof), causing the first response body to never be closed because the deferred closure captures the later resp; fix by using distinct response variables (e.g., respMetrics and respPprof) or by closing the first response before reassigning so each http.DefaultClient.Do(...) response is closed; update the http.NewRequest/http.DefaultClient.Do calls and the corresponding defer/Close() logic around io.ReadAll to reference the new unique response variables (or explicitly close resp after reading) so no response body is leaked.
♻️ Duplicate comments (1)
.golangci.yml (1)
12-43:⚠️ Potential issue | 🟠 MajorRemove linter overlap between
enableanddisable.Lines 12-43 enable several linters that Lines 89-98 disable again (
containedctx,errcheck,gocritic,nolintlint,testifylint,unparam). This cancels stricter checks and undercuts the PR objective.Suggested cleanup
disable: - - containedctx - - errcheck - - gocritic - - nolintlint - - testifylint - thelper - tparallel - - unparam - wrapcheck#!/bin/bash # Verify overlap between linters.enable and linters.disable in .golangci.yml. python - <<'PY' import re from pathlib import Path lines = Path(".golangci.yml").read_text().splitlines() in_linters = False section = None enable, disable = set(), set() for line in lines: if re.match(r'^linters:\s*$', line): in_linters = True section = None continue if in_linters and re.match(r'^[^\s].*:\s*$', line): in_linters = False section = None if not in_linters: continue if re.match(r'^\s{2}enable:\s*$', line): section = "enable"; continue if re.match(r'^\s{2}disable:\s*$', line): section = "disable"; continue if re.match(r'^\s{2}[a-zA-Z0-9_-]+:\s*$', line) and not re.match(r'^\s{2}(enable|disable):\s*$', line): section = None; continue m = re.match(r'^\s{4}-\s+([a-zA-Z0-9_-]+)\s*$', line) if not m: continue if section == "enable": enable.add(m.group(1)) elif section == "disable": disable.add(m.group(1)) print("Overlapping linters:", sorted(enable & disable)) PYAlso applies to: 89-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.golangci.yml around lines 12 - 43, The linters listed under linters.enable (e.g., containedctx, errcheck, gocritic, nolintlint, testifylint, unparam) are duplicated in linters.disable later in the file; open .golangci.yml, locate the linters.enable and linters.disable sections and remove the overlapping entries from the disable list so the enabled checks remain effective (ensure the disable list no longer contains containedctx, errcheck, gocritic, nolintlint, testifylint, unparam), then run your lint-check script to verify no remaining overlap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.golangci.yml:
- Around line 80-84: The current golangci-lint exclusion rule ("rules" entry
with path: _test\.go and linters: [prealloc, noctx]) broadly suppresses prealloc
and noctx findings for all test files; either remove this exclusion or narrow it
to specific test filenames or directories that truly require it (modify the
"path" pattern or list explicit files) so tests remain subject to desired lint
checks, or document the justification inline; update the .golangci.yml rules
block (the "path" and "linters" entries) accordingly.
In `@block/internal/syncing/syncer_backoff_test.go`:
- Line 124: The test currently calls syncer.startSyncWorkers(t.Context()) which
uses the immutable test context; instead pass the cancelable context (ctx) that
you created and cancel with cancel() so worker lifecycles are tied to the test
teardown. Update the three calls to startSyncWorkers to use the cancelable ctx
variable (the one paired with cancel()) rather than t.Context(), ensuring
startSyncWorkers(...) receives ctx so cancel() actually stops the workers.
---
Outside diff comments:
In `@node/full_node_test.go`:
- Around line 43-67: The test reuses the single resp variable for two HTTP calls
(metrics and pprof), causing the first response body to never be closed because
the deferred closure captures the later resp; fix by using distinct response
variables (e.g., respMetrics and respPprof) or by closing the first response
before reassigning so each http.DefaultClient.Do(...) response is closed; update
the http.NewRequest/http.DefaultClient.Do calls and the corresponding
defer/Close() logic around io.ReadAll to reference the new unique response
variables (or explicitly close resp after reading) so no response body is
leaked.
---
Duplicate comments:
In @.golangci.yml:
- Around line 12-43: The linters listed under linters.enable (e.g.,
containedctx, errcheck, gocritic, nolintlint, testifylint, unparam) are
duplicated in linters.disable later in the file; open .golangci.yml, locate the
linters.enable and linters.disable sections and remove the overlapping entries
from the disable list so the enabled checks remain effective (ensure the disable
list no longer contains containedctx, errcheck, gocritic, nolintlint,
testifylint, unparam), then run your lint-check script to verify no remaining
overlap.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
.github/workflows/lint.yml.golangci.yml.just/lint.justblock/internal/da/async_block_retriever_test.goblock/internal/executing/executor.goblock/internal/submitting/da_submitter.goblock/internal/submitting/da_submitter_test.goblock/internal/syncing/syncer.goblock/internal/syncing/syncer_backoff_test.goblock/internal/syncing/syncer_benchmark_test.goblock/internal/syncing/syncer_test.gonode/failover.gonode/full.gonode/full_node_test.gonode/light.gopkg/cmd/run_node_test.gopkg/p2p/client.gopkg/p2p/client_test.gopkg/rpc/server/da_visualization_non_aggregator_test.gopkg/rpc/server/da_visualization_test.gopkg/rpc/server/http_test.gopkg/rpc/server/server_test.gopkg/store/store_adapter_test.gopkg/telemetry/tracing.goscripts/test.gotools/blob-decoder/main.gotools/local-da/rpc.go
💤 Files with no reviewable changes (2)
- block/internal/submitting/da_submitter_test.go
- pkg/p2p/client_test.go
🚧 Files skipped from review as they are similar to previous changes (14)
- node/failover.go
- pkg/store/store_adapter_test.go
- pkg/cmd/run_node_test.go
- node/full.go
- pkg/rpc/server/da_visualization_non_aggregator_test.go
- pkg/rpc/server/da_visualization_test.go
- block/internal/syncing/syncer_test.go
- .just/lint.just
- tools/blob-decoder/main.go
- scripts/test.go
- pkg/telemetry/tracing.go
- .github/workflows/lint.yml
- block/internal/syncing/syncer_benchmark_test.go
- node/light.go
| rules: | ||
| - path: _test\.go | ||
| linters: | ||
| - prealloc | ||
| - noctx |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
golangci-lint v2 linters exclusions rules path suppress enforce
💡 Result:
golangci-lint v2: exclude/suppress/enforce (paths, rules, linters)
1) Enable/disable (enforce which linters run)
version: "2"
linters:
default: standard # or: all | none | fast
enable: [gosec]
disable: [dupl]linters.default/enable/disable control which linters execute. [1]
2) Exclude findings (without skipping analysis) — linters.exclusions.*
Per-path / per-linter / per-text / per-source:
linters:
exclusions:
warn-unused: true
presets: [comments, common-false-positives, legacy, std-error-handling]
rules:
- path: '(.+)_test\.go'
linters: [gocyclo, errcheck]
- path-except: '(.+)_test\.go'
linters: [forbidigo]
- linters: [staticcheck]
text: "SA9003:"
- linters: [lll]
source: "^//go:generate "Also available:
linters:
exclusions:
paths: ["path/to/a/file.go", "path/to/a/dir/"]
paths-except: ["keep/this/path.go"]
generated: strict # or: lax | disableThese options are the v2 way to exclude issues by path/text/source/linter and to control generated-files handling. [1] (examples also shown in the “False Positives” doc) [2]
3) Suppress inline with //nolint
var x int //nolint:wsl,unused
//nolint:all
func legacy() {}Valid directive form has no spaces in //nolint:.... [2]
4) Enforce “good” nolint usage (require specific linter + explanation, disallow unused)
linters:
settings:
nolintlint:
allow-unused: false
require-specific: true
require-explanation: trueThis makes //nolint harder to misuse (and catches unused/ill-formed directives). [3]
5) Make path matching predictable (important for exclusions)
run:
relative-path-mode: gomod # or: gitroot | cfg | wdThis controls how relative paths in exclusions (and some other features) are evaluated. [1]
Sources: Configuration file options & examples [1], exclusions + nolint syntax [2], nolintlint enforcement settings [3].
🏁 Script executed:
# Find and examine the .golangci.yml file
fd -t f ".golangci.yml" -x cat -n {}Repository: evstack/ev-node
Length of output: 41
🏁 Script executed:
# Search for the .golangci.yml file in the repository
fd ".golangci" --type fRepository: evstack/ev-node
Length of output: 41
🏁 Script executed:
# If found, display the content with line numbers, focusing on the relevant section
cat -n .golangci.yml | sed -n '70,95p'Repository: evstack/ev-node
Length of output: 726
This exclusion rule suppresses prealloc and noctx linter findings in test files; confirm this is intentional.
The linters.exclusions.rules configuration with path: _test\.go and linters: [prealloc, noctx] excludes findings from these linters for all test files. If the project standards expect stricter linting in tests (deterministic tests, table-driven patterns, standard formatting), this rule should be removed or narrowed to specific test files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.golangci.yml around lines 80 - 84, The current golangci-lint exclusion rule
("rules" entry with path: _test\.go and linters: [prealloc, noctx]) broadly
suppresses prealloc and noctx findings for all test files; either remove this
exclusion or narrow it to specific test filenames or directories that truly
require it (modify the "path" pattern or list explicit files) so tests remain
subject to desired lint checks, or document the justification inline; update the
.golangci.yml rules block (the "path" and "linters" entries) accordingly.
|
|
||
| // Run sync loop | ||
| syncer.startSyncWorkers() | ||
| syncer.startSyncWorkers(t.Context()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and read the test file
fd -type f -name "syncer_backoff_test.go" -path "*/block/internal/syncing/*"Repository: evstack/ev-node
Length of output: 229
🏁 Script executed:
# Find the test file
fd "syncer_backoff_test.go"Repository: evstack/ev-node
Length of output: 104
🏁 Script executed:
# Read the test file to examine context around the mentioned lines
cat -n block/internal/syncing/syncer_backoff_test.goRepository: evstack/ev-node
Length of output: 14114
Pass the cancelable context to startSyncWorkers, not t.Context().
At lines 124, 226, and 297, cancel() only cancels ctx; the workers receive t.Context(), which cannot be cancelled. This decouples the worker lifecycle from the test's cancellation mechanism, making the tests non-deterministic and prone to timeouts or incomplete teardown.
Suggested fix
- syncer.startSyncWorkers(t.Context())
+ syncer.startSyncWorkers(ctx)
@@
- syncer.startSyncWorkers(t.Context())
+ syncer.startSyncWorkers(ctx)
@@
- syncer.startSyncWorkers(t.Context())
+ syncer.startSyncWorkers(ctx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@block/internal/syncing/syncer_backoff_test.go` at line 124, The test
currently calls syncer.startSyncWorkers(t.Context()) which uses the immutable
test context; instead pass the cancelable context (ctx) that you created and
cancel with cancel() so worker lifecycles are tied to the test teardown. Update
the three calls to startSyncWorkers to use the cancelable ctx variable (the one
paired with cancel()) rather than t.Context(), ensuring startSyncWorkers(...)
receives ctx so cancel() actually stops the workers.
1 participant
Overview
add stricter linters that provide stricter guard rails. This is mainly useful for when using ai to write code
Summary by CodeRabbit
Chores
Tests