Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces systematic context propagation throughout the codebase for improved cancellation handling, particularly in block syncing and execution paths. It also enhances linter configuration, updates tests to accommodate context-aware APIs, and includes minor code hygiene improvements like replacing string literals with HTTP method constants and parameter renaming for clarity. Changes
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 1m 41s —— View job PR Review: chore: add stricter lintingTasks
SummaryThis PR adds a meaningful set of new linters and propagates Issues1. Linter overlap:
|
| Linter | Has settings? | Impact |
|---|---|---|
errcheck |
Yes — check-type-assertions, check-blank |
Settings never applied |
gocritic |
Yes — enabled-tags, disabled-checks |
Settings never applied |
containedctx |
No | Just silently disabled |
nolintlint |
No | Useful to catch stale nolints |
testifylint |
No | Good for test hygiene |
unparam |
No | Catches dead parameters |
Recommendation: Remove errcheck, gocritic, containedctx, nolintlint, testifylint, and unparam from the disable list so the enables actually take effect. If some are intentionally still disabled (e.g., wrapcheck, thelper, tparallel — those are fine), document why.
2. Resource leak in full_node_test.go
File: node/full_node_test.go lines 47-60
The resp variable is shared across both HTTP requests (metrics + pprof). Both defer closures capture resp by reference. When the function returns:
- The second defer runs and closes
resp(which now points to the pprof response) ✓ - The first defer runs —
respis still the pprof response, so it closes it a second time
Result: the first response body (metrics) is never closed, leaking an HTTP connection.
// Lines 47-60 — defer captures resp by reference, resp gets reassigned at line 60
defer func() {
err := resp.Body.Close() // will close pprof body, not metrics body
...
}()
...
resp, err = http.DefaultClient.Do(req) // resp is now pprof response
defer func() {
err := resp.Body.Close() // closes pprof body
...
}()Fix: Use distinct variable names (respMetrics, respPprof), or explicitly close the first body before reassigning resp.
3. noctx suppression for scripts/test.go is a workaround, not a fix
File: scripts/test.go line 32
The //nolint:noctx comment leaves the command without a timeout and susceptible to hanging indefinitely. The correct fix is exec.CommandContext with a bounded context:
testCtx, cancel := context.WithTimeout(context.Background(), 30*time.Minute)
defer cancel()
cmd := exec.CommandContext(testCtx, "go", "test", "./...", "-cover")This is a straightforward improvement that removes the nolint comment and adds proper cancellation.
Minor Notes
Context propagation refactor in syncer.go (lines 167-202):
The CodeRabbit comment about raftRetriever.Start(ctx) vs raftRetriever.Start(s.ctx) is a false positive — at that point in Start(), the local ctx variable IS s.ctx (they're set to the same value on lines 167-168). No issue here.
pkg/telemetry/tracing.go — clamp parameter rename:
Renaming min/max to minFloat/maxFloat is correct — predeclared linter flags the shadowing of Go's built-in min/max functions (added in Go 1.21). Good catch.
tools/local-da/rpc.go — Included context propagation:
Properly propagates the caller's context instead of context.Background(). Small but meaningful correctness improvement.
pkg/p2p/client.go — GetNetworkInfo optimization:
Caching c.host.Addrs() in a variable avoids duplicate method calls. Good.
HTTP method string constants:
Replacing "POST", "OPTIONS" etc. with http.MethodPost, http.MethodOptions is exactly what usestdlibvars enforces. Clean.
mustGETRequest helper in server_test.go:
Nice DRY refactor — centralizes request construction and error handling across multiple test cases.
Executor context propagation (executor.go):
Switching e.raftNode.Broadcast(ctx, ...), e.headerBroadcaster.WriteToStoreAndBroadcast(ctx, ...), etc. to use the caller-provided context rather than e.ctx is the right approach — it lets the caller control the deadline for individual block production operations without being tied to the executor's lifecycle context.
nolint:contextcheck annotations in node shutdown paths:
Using context.Background() for shutdown contexts is intentional and correct — the parent context is already cancelled by this point. The annotations are well-justified and the comments explain the rationale.
Verdict
Good hygiene PR. The three issues above (linter overlap, response body leak, noctx workaround) are worth fixing before merge. The context propagation work is solid and the linter additions are a meaningful improvement.
![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: bc3584e | Previous: f8fa22e | Ratio |
|---|---|---|---|
SpamoorSmoke - BlockExecutor.ApplyBlock (avg) |
69702.07272727272 us |
5063.507692307692 us |
13.77 |
SpamoorSmoke - BlockExecutor.ProduceBlock (avg) |
70609.61818181818 us |
5962.0615384615385 us |
11.84 |
SpamoorSmoke - Engine.GetPayload (avg) |
29839.49090909091 us |
609.3692307692307 us |
48.97 |
SpamoorSmoke - Engine.NewPayload (avg) |
36845.181818181816 us |
1468.2307692307693 us |
25.09 |
SpamoorSmoke - Executor.ExecuteTxs (avg) |
69689.67272727273 us |
5050.523076923077 us |
13.80 |
SpamoorSmoke - Executor.GetTxs (avg) |
9984.714285714286 us |
5084.875 us |
1.96 |
SpamoorSmoke - TxPool.GetTxs (avg) |
9848.57142857143 us |
4996.625 us |
1.97 |
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
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
alpe
left a comment
alpe
left a comment
There was a problem hiding this comment.
Good findings. Please double check the .golangci.yml for linters in enabled/disabled at the same time.
I am approving this already for a fast dev cycle
| - examples$ | ||
| disable: | ||
| - containedctx | ||
| - errcheck |
There was a problem hiding this comment.
As coderabbit pointed out, some linters are in enabled and disabled at the same time. for example errcheck.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.golangci.yml (1)
39-54:⚠️ Potential issue | 🟠 MajorRemove
errcheckandgocriticfrom thedisablelist to activate their configured settings.Lines 39-54 define strict settings for
errcheckandgocritic, but both linters are explicitly disabled at lines 85-86. This creates inert configuration: their settings are unreachable and ignored by golangci-lint. Remove them from the disable list to align with the stricter linting objective.Suggested fix
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 39 - 54, Remove "errcheck" and "gocritic" from the linter disable list so their earlier configuration blocks are applied: locate the top-level disable array (the list that currently contains "errcheck" and "gocritic") and delete those two entries, ensuring no duplicate entries remain elsewhere and that the errcheck and gocritic configuration blocks (check-type-assertions, check-blank, enabled-tags, disabled-checks) remain unchanged; after editing, run golangci-lint config or the CI lint job to verify the configuration is accepted.
🧹 Nitpick comments (1)
.golangci.yml (1)
87-87: Disablingnolintlintweakens guard rails around//nolintusage.Given the PR goal (stricter AI-era guard rails), consider enabling
nolintlintwith explicit requirements (specific linter + explanation).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.golangci.yml at line 87, Enable and configure the nolintlint linter instead of disabling it: re-enable the nolintlint entry (symbol "nolintlint") under the "linters" list and add a "linters-settings" -> "nolintlint" block that requires a specific linter name and a human explanation for every //nolint usage (use keys like "require-specific" or "require-specific-linter" and "require-explanation" or similar supported options), ensuring the config enforces explicit linter target and justification for any //nolint occurrences.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.golangci.yml:
- Around line 39-54: Remove "errcheck" and "gocritic" from the linter disable
list so their earlier configuration blocks are applied: locate the top-level
disable array (the list that currently contains "errcheck" and "gocritic") and
delete those two entries, ensuring no duplicate entries remain elsewhere and
that the errcheck and gocritic configuration blocks (check-type-assertions,
check-blank, enabled-tags, disabled-checks) remain unchanged; after editing, run
golangci-lint config or the CI lint job to verify the configuration is accepted.
---
Nitpick comments:
In @.golangci.yml:
- Line 87: Enable and configure the nolintlint linter instead of disabling it:
re-enable the nolintlint entry (symbol "nolintlint") under the "linters" list
and add a "linters-settings" -> "nolintlint" block that requires a specific
linter name and a human explanation for every //nolint usage (use keys like
"require-specific" or "require-specific-linter" and "require-explanation" or
similar supported options), ensuring the config enforces explicit linter target
and justification for any //nolint occurrences.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 203b8c17-7010-490b-ba35-fc939182bc35
📒 Files selected for processing (3)
.golangci.ymlblock/internal/executing/executor.goblock/internal/syncing/syncer_backoff_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- block/internal/syncing/syncer_backoff_test.go
- block/internal/executing/executor.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3132 +/- ##
==========================================
- Coverage 60.78% 60.78% -0.01%
==========================================
Files 113 113
Lines 11693 11692 -1
==========================================
- Hits 7108 7107 -1
+ Misses 3782 3781 -1
- Partials 803 804 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
spec.md (1)
237-243: Consider varying repeated sentence starts for readability.Three consecutive bullets start with “Log”, which is stylistically repetitive. Low priority, but easy cleanup.
Suggested edit
- Log each received snapshot at DEBUG level: - `snapshot_id` - `tx_count` - `receipt_lag_ms` -- Log stream connect/disconnect at INFO level. -- Log snapshot drops at WARN level. +- Record stream connect/disconnect at INFO level. +- Emit snapshot-drop events at WARN level.Based on learnings: All code must pass linting using just lint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec.md` around lines 237 - 243, Edit the three bullet points that start with the word "Log" to vary their sentence starts for readability—e.g., change "Log each received snapshot at DEBUG level: `snapshot_id`..." to "For each received snapshot, emit a DEBUG-level log including `snapshot_id`, `tx_count`, and `receipt_lag_ms`", change "Log stream connect/disconnect at INFO level." to "Emit INFO-level logs on stream connect and disconnect.", and change "Log snapshot drops at WARN level." to "Record snapshot drops at WARN level."; ensure the revised phrasing still conveys the same logging levels and fields and that the file passes linting with the project's lint command.block/internal/syncing/syncer_benchmark_test.go (1)
46-47: JoinprocessLoopper benchmark iteration to avoid goroutine carry-over.Line 46 starts
processLoopoutsidefixt.s.wg, so Line 54 doesn’t explicitly wait for it. Inb.Loop(), this can introduce iteration bleed and benchmark noise.As per coding guidelines "Be mindful of goroutine leaks".♻️ Proposed fix
- go fixt.s.processLoop(fixt.s.ctx) + loopDone := make(chan struct{}) + go func() { + defer close(loopDone) + fixt.s.processLoop(fixt.s.ctx) + }() fixt.s.startSyncWorkers(fixt.s.ctx) @@ fixt.s.cancel() fixt.s.wg.Wait() + <-loopDone🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/syncing/syncer_benchmark_test.go` around lines 46 - 47, processLoop is launched as a goroutine outside the fixture waitgroup so iterations leak goroutines and pollute b.Loop(); wrap its start so it is tracked by fixt.s.wg and joined after each benchmark iteration—for example, call fixt.s.wg.Add(1) before starting processLoop and run it as go func() { defer fixt.s.wg.Done(); fixt.s.processLoop(fixt.s.ctx) } or otherwise ensure processLoop reports done into fixt.s.wg and that fixt.s.wg.Wait() is called after b.Loop(); keep startSyncWorkers(fixt.s.ctx) unchanged but ensure both workers and processLoop are waited on to avoid goroutine carry-over.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spec.md`:
- Line 104: Update the phrase "A snapshot represents the best ordered selection
from the pool at that moment." to hyphenate the compound adjective as
"best-ordered selection" so the sentence reads "A snapshot represents the
best-ordered selection from the pool at that moment." to satisfy linting and
grammar rules.
---
Nitpick comments:
In `@block/internal/syncing/syncer_benchmark_test.go`:
- Around line 46-47: processLoop is launched as a goroutine outside the fixture
waitgroup so iterations leak goroutines and pollute b.Loop(); wrap its start so
it is tracked by fixt.s.wg and joined after each benchmark iteration—for
example, call fixt.s.wg.Add(1) before starting processLoop and run it as go
func() { defer fixt.s.wg.Done(); fixt.s.processLoop(fixt.s.ctx) } or otherwise
ensure processLoop reports done into fixt.s.wg and that fixt.s.wg.Wait() is
called after b.Loop(); keep startSyncWorkers(fixt.s.ctx) unchanged but ensure
both workers and processLoop are waited on to avoid goroutine carry-over.
In `@spec.md`:
- Around line 237-243: Edit the three bullet points that start with the word
"Log" to vary their sentence starts for readability—e.g., change "Log each
received snapshot at DEBUG level: `snapshot_id`..." to "For each received
snapshot, emit a DEBUG-level log including `snapshot_id`, `tx_count`, and
`receipt_lag_ms`", change "Log stream connect/disconnect at INFO level." to
"Emit INFO-level logs on stream connect and disconnect.", and change "Log
snapshot drops at WARN level." to "Record snapshot drops at WARN level."; ensure
the revised phrasing still conveys the same logging levels and fields and that
the file passes linting with the project's lint command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c3d72464-4bdf-4224-bcbe-80ef517fb127
📒 Files selected for processing (9)
block/internal/executing/executor.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/light.gospec.md
🚧 Files skipped from review as they are similar to previous changes (3)
- node/light.go
- node/failover.go
- node/full.go
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/failover_e2e_test.go`:
- Around line 227-230: The retry closure currently hard-fails on transient DA
RPC errors because queryLastDAHeight calls require.NoError and
verifyDABlocksCollect uses context.Background(), preventing cancellation; change
queryLastDAHeight to a non-fatal variant that returns (height, error) instead of
calling require.NoError, update the EventuallyWithT closures to call that
variant and use assert.NoError(collect, err) so retries continue on transient
errors, and modify verifyDABlocksCollect to accept a context.Context parameter
and pass t.Context() from the test so in-flight RPCs are cancelled when the test
context is done.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8f4c416-e1e4-4472-a31e-764bedc9be0f
📒 Files selected for processing (1)
test/e2e/failover_e2e_test.go
| require.EventuallyWithT(t, func(collect *assert.CollectT) { | ||
| lastDA := queryLastDAHeight(t, env.SequencerJWT, env.Endpoints.GetDAAddress()) | ||
| verifyDABlocksCollect(collect, 1, lastDA, env.SequencerJWT, env.Endpoints.GetDAAddress(), genesisHeight, state.LastBlockHeight) | ||
| }, 3*must(time.ParseDuration(DefaultDABlockTime)), 500*time.Millisecond) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, let's check the exact file and see the relevant sections
echo "=== Lines 227-230 (EventuallyWithT usage) ==="
sed -n '225,232p' test/e2e/failover_e2e_test.go
echo ""
echo "=== Lines 512-515 (second EventuallyWithT) ==="
sed -n '510,517p' test/e2e/failover_e2e_test.go
echo ""
echo "=== Check queryLastDAHeight function ==="
rg -n -A8 'func queryLastDAHeight' test/e2e/failover_e2e_test.go
echo ""
echo "=== Check verifyDABlocksCollect function signature and context usage ==="
rg -n -A15 'func verifyDABlocksCollect' test/e2e/failover_e2e_test.goRepository: evstack/ev-node
Length of output: 2351
Retry loop hard-fails on transient DA errors and verifyDABlocksCollect uses non-cancellable context.
The EventuallyWithT closures at lines 227-230 and 512-515 call queryLastDAHeight(t, ...), which uses require.NoError to hard-fail on any RPC error (including transient ones), bypassing retry logic. Additionally, verifyDABlocksCollect uses context.Background() instead of a cancellable context, leaving blocked RPC calls untethered from test cancellation.
To fix: Replace queryLastDAHeight with a soft-assertion variant that returns errors (not using require.NoError), propagate t.Context() into verifyDABlocksCollect, and use assert.NoError(collect, err) in the retry closure to allow retries on transient failures.
Violations: "Use context.Context for cancellation" and "Ensure tests are deterministic".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/failover_e2e_test.go` around lines 227 - 230, The retry closure
currently hard-fails on transient DA RPC errors because queryLastDAHeight calls
require.NoError and verifyDABlocksCollect uses context.Background(), preventing
cancellation; change queryLastDAHeight to a non-fatal variant that returns
(height, error) instead of calling require.NoError, update the EventuallyWithT
closures to call that variant and use assert.NoError(collect, err) so retries
continue on transient errors, and modify verifyDABlocksCollect to accept a
context.Context parameter and pass t.Context() from the test so in-flight RPCs
are cancelled when the test context is done.
2 participants
Overview
add stricter linters that provide stricter guard rails. This is mainly useful for when using ai to write code
Summary by CodeRabbit
Release Notes
Chores
Tests