Skip to content

node:vm: report compileFunction body lines relative to lineOffset when it is 0 - #38240

Open
robobun wants to merge 9 commits into
mainfrom
farm/7174a843/vm-compile-function-line-offset
Open

node:vm: report compileFunction body lines relative to lineOffset when it is 0#38240
robobun wants to merge 9 commits into
mainfrom
farm/7174a843/vm-compile-function-line-offset

node:vm: report the body's own columns on a compileFunction's first line

0c34c9a
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Aug 14, 2026 in 33m 7s

Code review found 2 potential issues

Found 3 candidates, confirmed 2. See review comments for details.

Details

Severity Count
🔴 Important 0
🟡 Nit 2
🟣 Pre-existing 0
Severity File:Line Issue
🟡 Nit src/jsc/bindings/ZigException.cpp:192-193 Uncaught-error caret misaligned by columnOffset on compileFunction body line 1
🟡 Nit test/js/node/vm/vm.test.ts:404-412 Subprocess test pipes stdout without draining it and asserts exitCode before content

Annotations

Check warning on line 193 in src/jsc/bindings/ZigException.cpp

See this annotation in the file changed.

@claude claude / Claude Code Review

Uncaught-error caret misaligned by columnOffset on compileFunction body line 1

The uncaught-error source preview strips `wrapperTextLength` from the displayed line but the caret column comes from `getAdjustedPositionForBytecode`, which subtracts `wrapperColumns` — with `columnOffset > 0` these differ, so the `^` lands `columnOffset` columns past the throw. `handleException` avoids this by shifting the caret by `startColumnZeroBased + wrapperPrefixLength` (i.e. the same quantity as the strip); the fix here is to also subtract `wrapperTextLength - wrapperColumns` (== columnO

Check warning on line 412 in test/js/node/vm/vm.test.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

Subprocess test pipes stdout without draining it and asserts exitCode before content

The `run` helper sets `stdout: "pipe"` but only awaits `proc.stderr.text()` and `proc.exited` — stdout is never drained (REVIEW.md's subprocess rule requires draining every piped stream in the same `Promise.all`, or drop `stdout: "pipe"` since nothing is asserted on it). It also runs `expect(exitCode).toBe(1)` before returning stderr for the content assertions, so an unexpected exit code fails with just "expected 1, received N" and no stderr shown; CLAUDE.md/REVIEW.md ask for content-before-exit