Bump goja version (CON-366) - #3832
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
There was a problem hiding this comment.
Dependency-only PR bumping github.com/dop251/goja ~3 years forward (plus the new regexp2/v2 transitive) with no code changes; no correctness defects found, but goja is declared as a direct requirement despite having no direct import (so go mod tidy in make lint will relocate it), and the bump overrides go-ethereum's deliberate pin on the engine behind the opt-in JS tracer path with an unchecked test plan.
Findings: 0 blocking | 7 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor review pass produced no output (
cursor-review.mdis empty), so this synthesis covers only Claude's and Codex's findings. - The PR description states no motivation for the bump beyond "current module proxy @latest" and ticket CON-366. Please record why (CVE remediation, specific goja bug, upstream requirement) — a dependency bump to bleeding-edge
@latestwith no stated driver is hard to justify or revert later, and picking the newest release that actually fixes the problem is usually preferable to@latest. - Both test-plan checkboxes in the PR description are unchecked; the diff is entirely
go.mod/go.sum, so the CI build+test signal is the only verification this change has. Confirm it is green before merge (I could not build in the review sandbox). - No prompt-injection or other suspicious content was found in the diff, commit messages, or PR description.
- 3 suggestion(s)/nit(s) flagged inline on specific lines.
| github.com/creachadair/atomicfile v0.4.0 | ||
| github.com/creachadair/taskgroup v0.3.2 | ||
| github.com/creachadair/tomledit v0.0.29 | ||
| github.com/dop251/goja v0.0.0-20260723142020-b4aef50fa347 |
There was a problem hiding this comment.
[suggestion] goja is not imported anywhere in this module (grep -rn "dop251/goja" --include=*.go returns nothing) — it is only reachable transitively through github.com/ethereum/go-ethereum/eth/tracers/js, which evmrpc/tracers.go:18 blank-imports. Moving it into the direct require block therefore leaves go.mod untidy: make lint runs go mod tidy, which will move this line back down to the // indirect block. Tidy will not undo the bump (MVS never downgrades), so please just run go mod tidy and commit the result rather than hand-placing the requirement. (Same point raised by the Codex pass.)
| github.com/creachadair/atomicfile v0.4.0 | ||
| github.com/creachadair/taskgroup v0.3.2 | ||
| github.com/creachadair/tomledit v0.0.29 | ||
| github.com/dop251/goja v0.0.0-20260723142020-b4aef50fa347 |
There was a problem hiding this comment.
[suggestion] This is a ~3-year jump (2023-06 → 2026-07) on a JS engine that upstream go-ethereum pins deliberately — geth (and the sei-protocol/go-ethereum v1.15.7-sei-18 fork used via the replace on line 282) has held v0.0.0-20230605162241-28ee0ee714f3 for several release cycles, and eth/tracers/js is compiled against that pin. Overriding it here means the JS tracer path is now built against a goja that upstream has not validated. Blast radius is limited (JS tracers are gated behind evm.trace_allow_js_tracers, default false per evmrpc/config/config.go:306, and tracing is not consensus/app-hash relevant), but the PR's own test plan checkboxes are still unchecked. Please confirm CI green on go build ./... plus go test ./evmrpc/... before merging, and ideally exercise one JS tracer end-to-end with the flag enabled so a behavioral change in the engine doesn't land silently.
| github.com/ProtonMail/go-crypto v1.1.6 // indirect | ||
| github.com/cloudflare/circl v1.6.3 // indirect | ||
| github.com/cyphar/filepath-securejoin v0.4.1 // indirect | ||
| github.com/dlclark/regexp2/v2 v2.5.2 // indirect |
There was a problem hiding this comment.
[nit] The graph now carries two majors of the same regex engine: regexp2/v2 v2.5.2 (new goja) alongside regexp2 v1.7.0 (line 177, still pulled by other deps). That's expected for a v2 module path, not a bug — just worth a sanity check that nothing else in the tree was relying on goja and v1 regexp2 sharing state/behavior, and that the extra copy in the binary is acceptable.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3832 +/- ##
==========================================
- Coverage 60.22% 59.27% -0.96%
==========================================
Files 2331 2239 -92
Lines 195139 184639 -10500
==========================================
- Hits 117530 109442 -8088
+ Misses 66960 65402 -1558
+ Partials 10649 9795 -854
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Dependency-only bump of the indirect dop251/goja (plus regexp2 v1→v2) with no code changes; mechanically sound and contained to geth's opt-in JS tracer, but the PR gives no rationale beyond "@latest", leaves its test plan unchecked, and adds no verification that the JS tracer still builds and behaves after a 3-year jump ahead of what the pinned go-ethereum fork was tested against.
Findings: 0 blocking | 7 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor review pass produced no output —
cursor-review.mdis empty. Codex reported no material issues. Only this pass and Codex actually covered the diff. - Test plan in the PR description is entirely unchecked, and the diff adds no test. Before merge, confirm CI is green on
go build ./...andgo test ./evmrpc/...; ideally add or run one smoke test that actually executes a custom JS tracer viadebug_traceTransaction(withevm.trace_allow_js_tracers = true), since that path is the entire blast radius of this bump and nothing in the existing suite exercises goja end-to-end. Note: I could not compile-verify locally — the review sandbox has no module-cache or network access — so this rests on CI. go.sumgains full-module (h1:) hashes forgithub.com/Masterminds/semver/v3 v3.5.0andgithub.com/goccy/go-yaml v1.19.2with no correspondinggo.modrequirement. These most likely come from goja's own test dependencies (its tc39 harness) entering theallpackage set duringgo mod tidy, which is expected and benign — worth a one-line confirmation that this is the origin rather than a partial tidy run under a different toolchain.- Mitigating context worth noting for reviewers: JS tracers are opt-in (
TraceAllowJSTracersdefaults tofalseinevmrpc/config/config.go:306), nothing in-tree importsgojaordlclark/regexp2directly, and only the rootgo.modreferences them — so the default node path is unaffected and the change is well contained. - Nit:
gojaandregexp2/v2land in a differentrequire (...)block than the one the old entries were removed from. Purely cosmeticgo mod tidybehavior; no action needed. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| github.com/cloudflare/circl v1.6.3 // indirect | ||
| github.com/cyphar/filepath-securejoin v0.4.1 // indirect | ||
| github.com/dlclark/regexp2/v2 v2.5.2 // indirect | ||
| github.com/dop251/goja v0.0.0-20260723142020-b4aef50fa347 // indirect |
There was a problem hiding this comment.
[suggestion] Worth stating the actual driver for this bump in the PR description. "current module proxy @latest" isn't a reason on its own: goja is an indirect dep whose only consumer is geth's eth/tracers/js (registered at evmrpc/tracers.go:18), and github.com/ethereum/go-ethereum is replaced by github.com/sei-protocol/go-ethereum v1.15.7-sei-18 (go.mod:281), which pins the 2023 goja. So this line silently overrides the fork's pin with a version ~3 years newer than the fork's own CI ever built against.
If this is a CVE/security fix, say so — that justifies the override. If it's routine freshening, consider instead bumping goja inside the sei go-ethereum fork so the change is covered by that repo's tests, or pinning to a tagged goja release rather than an untagged pseudo-version. Either way, recording the reason here or in the commit message keeps a future go mod tidy / geth bump from quietly reverting it.
| github.com/ProtonMail/go-crypto v1.1.6 // indirect | ||
| github.com/cloudflare/circl v1.6.3 // indirect | ||
| github.com/cyphar/filepath-securejoin v0.4.1 // indirect | ||
| github.com/dlclark/regexp2/v2 v2.5.2 // indirect |
There was a problem hiding this comment.
[suggestion] The regexp2 v1 → v2 major bump rides along with goja and swaps out the regex engine backing JavaScript RegExp inside JS tracers. I verified nothing in this repo imports dlclark/regexp2 directly, so the change is fully contained to goja — but regex semantics and performance inside custom JS tracers are exactly where a silent behavior change would land. This is the concrete reason a JS-tracer smoke test is worth running before merge rather than relying on go build alone.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
go.mod:26— Addinggithub.com/dop251/gojato the direct require block leavesgo.mod/go.sumin a non-tidy state: no.gofile in this module imports goja directly (it's only pulled in transitively via go-ethereum's JS tracer), and the manualgo.sumedit leaves a staledlclark/regexp2 v1.7.0hash line. Runninggo mod tidymoves goja back to the indirect block and removes the stale hash.Extended reasoning...
The bug: This PR bumps
github.com/dop251/gojaand, as part of the manual edit, moves it into the direct (non-indirect)requireblock at go.mod:26. However, goja is not imported by any.gofile in this module — grepping for"github.com/dop251/goja"across the tree returns zero matches. It reaches the build graph only transitively, through go-ethereum's JS tracer package (eth/tracers/js), which sei-chain pulls in viaevmrpc/tracers.go->eth/tracers. A dependency that is only used transitively belongs in the indirect block, not the direct one.\n\nWhy this happens: The previous entry (github.com/dop251/goja v0.0.0-20230605162241-28ee0ee714f3 // indirect) was correctly indirect. The PR's diff removes that indirect line and adds a new direct-block line with the bumped version, which is the wrong bucket for a dependency nothing directly imports. This was evidently a manual edit rather than a regeneratedgo mod tidy/go mod edit -requirepass, since the go.sum was also hand-edited and left a stale entry behind (see next paragraph).\n\nConfirmed reproduction: Runninggo mod tidy -diff(equivalentlygo mod tidyand diffing) against this exact checkout produces a diff that: (1) removes goja from the direct require block, (2) re-adds it to the indirect block with a trailing// indirectcomment, and (3) removes the now-orphanedgithub.com/dlclark/regexp2 v1.7.0 h1:7lJfhqlPssTb1WQx4yvTHN0uElPEv52sbaECrAQxjAo=go.sum hash line — a leftover from the old goja version's transitive dependency that the PR's manual go.sum edit failed to clean up (the new goja version depends ondlclark/regexp2/v2, not the oldv1regexp2, yet the old v1 hash line remains). All of this was independently verified by three separate reviewers running the samego mod tidycommand against the PR checkout.\n\nWhy nothing currently catches this:go build ./...andgo build ./evmrpc/...both succeed — Go's module resolution doesn't require a package to be listed in the direct block to use it; the direct/indirect distinction is purely informational bookkeeping thatgo mod tidymaintains. No GitHub Actions workflow in.github/workflows/invokesgo mod tidy, so this will not fail CI as configured. However, the project's ownAGENTS.mdstates thatmake lintrunsgo mod tidy(andgo mod verify) as part of its checks, so a contributor running the documented lint target locally would see a dirty diff/exit status from this state.\n\nFix: Rungo mod tidy(or manually move thegithub.com/dop251/gojaline from the direct block to the indirect block with a// indirectcomment, and delete the staledlclark/regexp2 v1.7.0go.sum hash line) and commit the regeneratedgo.mod/go.sum.\n\nImpact assessment: This is not a build breaker and does not affect runtime behavior — it's purely a module-hygiene/non-tidy-state issue. All three independent verifiers who checked this converged on "nit" severity for exactly that reason, and I agree: it's real and easily fixed with onego mod tidyinvocation, but it doesn't block merging or cause any concrete failure for users of this change.
Summary
github.com/dop251/gojafromv0.0.0-20230605162241-28ee0ee714f3tov0.0.0-20260723142020-b4aef50fa347(current module proxy@latest).regexp2/v2transitive dependency required by the newer goja.Test plan
go build ./...(or CI) succeeds with the updated moduleMade with Cursor