Repository navigation
Conversation
CallSite#getColumnNumber() was returning a 0-based column while V8 (and therefore Node.js) returns 1-based. This was also inconsistent with CallSite#toString(), which already prints the 1-based column. source-map-support's wrapCallSite does frame.getColumnNumber() - 1 to convert to a 0-based index for originalPositionFor(). A 0-based input yields -1 for any frame at column 1 and the consumer throws 'Column must be greater than or equal to 0, got -1'. Both getColumnNumber() and getLineNumber() now return null when no position is available (native frames), matching V8 and @types/node's number | null signature. toJSON().columnNumber is brought in line with getColumnNumber(). Fixes #17303 Fixes #18250
WalkthroughChangesCallSite location semantics
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
No additional open issues found beyond those already referenced in the PR. 🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — correct V8-compat fix with fail-before/pass-after tests. The inline nit on toJSON() for native frames is pre-existing and non-blocking.
Extended reasoning...
Overview
Two-file change: src/jsc/bindings/CallSitePrototype.cpp switches CallSite#getColumnNumber() from zeroBasedInt() to oneBasedInt() (matching V8's Stack Trace API and Bun's own toString()), makes both getLineNumber()/getColumnNumber() return null for OrdinalNumber::beforeFirst() (native frames) instead of clamped 1/0, and updates toJSON().columnNumber to oneBasedInt(). test/js/node/v8/capture-stack-trace.test.js adds two tests covering the 1-based contract (subprocess, so column 1 is observable un-shifted by the CJS wrapper) and the native-frame null return.
Security risks
None. Pure read-only accessors on stack-trace metadata; no user input parsing, no allocation sizing, no auth/crypto surface.
Level of scrutiny
Low-to-moderate. The diff is ~15 lines of C++ in well-understood accessors. I verified:
formatAsString(backingtoString()) already usesoneBasedInt()at CallSite.cpp:169-174 and already treatszeroBasedInt() < 0as no-position — so the getters now agree with it.- No live internal Bun code reads the JS-level getters (the only
src/jshit is commented out ininternal/assert/utils.ts). - Existing test assertions on
getLineNumber()/getColumnNumber()all target script frames and checktypeof === "number"or compare against the same-basis reference, so nothing regresses. source-map-support'swrapCallSitedoesgetColumnNumber() - 1, and its native-frame branch guards ongetFileName()before touching column, so returningnullthere is safe.
Other factors
Tests follow harness conventions (spread bunEnv, concurrent pipe drain, exact-value assertions, issue-URL comment) and the PR description shows fail-before with USE_SYSTEM_BUN=1. The one flagged nit — toJSON() still emits 0/0 rather than null for native frames — is a Bun-specific convenience method whose getter/toJSON mismatch pre-dates this PR; it does not affect the ecosystem bug being fixed and can land separately.
|
Updated 5:52 PM PT - Jul 22nd, 2026
❌ @robobun, your commit 3aaf2bf has 3 failures in
🧪 To try this PR locally: bunx bun-pr 35179That installs a local version of the PR into your bun-35179 --bun |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/v8/capture-stack-trace.test.js`:
- Around line 584-593: Wrap the Error.prepareStackTrace setup and
nativeFrameForTesting invocation in a try/finally block, restoring
prevPrepareStackTrace in the finally clause. Keep the existing frame capture
behavior unchanged while ensuring restoration occurs if either the helper or
callback throws.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 831c3dd0-124b-41df-b4b4-430666be70a9
📒 Files selected for processing (2)
src/jsc/bindings/CallSitePrototype.cpptest/js/node/v8/capture-stack-trace.test.js
There was a problem hiding this comment.
LGTM — the toJSON follow-up in 92b2507 addresses the earlier nit, and the native-frame test now pins it.
What was reviewed:
getColumnNumber()0→1-based andbeforeFirst() → nullfor both getters, verified against V8's Stack Trace API contract and@types/node'snumber | null.- Checked for internal consumers:
Bun__CallFrame__getLineNumberin jest.rs is a separate FFI onJSC::CallFrame*, and theassert/utils.tsreference is commented out — nothing in-tree reads the JS-level return values. - Grepped the test suite for other
getColumnNumber/getLineNumberassertions that the 0→1 shift would break; none found.
Extended reasoning...
Overview
Three-function change in src/jsc/bindings/CallSitePrototype.cpp: callSiteProtoFuncGetLineNumber, callSiteProtoFuncGetColumnNumber, and callSiteProtoFuncToJSON. getColumnNumber() switches from zeroBasedInt() (clamped to 0) to oneBasedInt(); both getters now return jsNull() when the underlying OrdinalNumber is beforeFirst() (native frames with no source position); toJSON() mirrors that. Two new tests in test/js/node/v8/capture-stack-trace.test.js cover the 1-based column (via subprocess so column 1 is observable) and the native-frame null case.
Security risks
None. No user input parsing, no allocation/size arithmetic, no lifetime changes. Pure integer-basis / sentinel-mapping change on already-held OrdinalNumber values.
Level of scrutiny
Low-to-moderate. This is a Node/V8 compat correctness fix in a small, self-contained JSC binding. The V8 Stack Trace API documents 1-based columns and null for unavailable positions; @types/node types both as number | null. Bun's own CallSite#toString() was already 1-based, so this removes an internal inconsistency rather than introducing one. The reported ecosystem breakage (source-map-support doing getColumnNumber() - 1) is exactly what this fixes.
I checked for in-tree callers that could regress on the 0→1 shift or the new null: src/js/internal/assert/utils.ts has a commented-out block that does - 1 (which would now be correct), and src/runtime/test_runner/jest.rs's Bun__CallFrame__getLineNumber is a distinct FFI export on JSC::CallFrame*, not Zig::CallSite. No .d.ts in packages/bun-types declares these, so no type surface to update.
Other factors
- My prior review flagged the
toJSONsibling not getting thebeforeFirst() → nullguard; 92b2507 applied it to bothlineNumberandcolumnNumberand extended the native-frame test to asserttoJSON()agrees with the getters. That thread is resolved. - CodeRabbit's
try/finallysuggestion was correctly declined (file-levelafterEachrestoresError.prepareStackTrace) and withdrawn. - Tests follow harness conventions (
bunExe()/bunEnv, concurrent pipe drain, exit-code asserted last, subprocess for column-position isolation) and the PR shows fail-before output underUSE_SYSTEM_BUN=1. - The ternary in
toJSONunifies toJSValue(bothjsNull()andjsNumber(int)returnJSValue), so no implicit-conversion concern forputDirect.
The hardcoded '9:29'/'11:29' values were a Bun-local edit (ed9353f) encoding the old 0-based getColumnNumber(). With getColumnNumber() now 1-based, Bun and Node.js both report column 30 for this call site via the vendored getCallSite() helper.
Revert the Bun-local addition of :getColumnNumber() to test/js/node/test/common/index.js's getCallSite() and restore test-common-must-not-call.js to the upstream line-only expectations. The 3-part format and '9:29'/'11:29' assertions were Bun modifications that encoded the 0-based getColumnNumber() bug. Column coverage now lives in test/js/node/v8/capture-stack-trace.test.js, so this file no longer needs to be coupled to CallSite column semantics.
There was a problem hiding this comment.
LGTM — the earlier toJSON() nit was applied in 92b2507 and is now covered by the native-frame test.
What was reviewed:
getColumnNumber()/getLineNumber()/toJSON()now agree on 1-based +null-for-beforeFirst(); matches V8's documented CallSite contract.- Grepped
src/andtest/for JS-levelgetLineNumber()/getColumnNumber()consumers — the only live one was the vendoredcommon/index.js, restored here to upstream's line-only form;src/js/internal/assert/utils.tsusage is commented out. test/regression/issue/29240.test.ts'scallFrame.columnNumberis the CPU-profile format, notCallSite#toJSON, so unaffected.
Extended reasoning...
Overview
Changes CallSite#getColumnNumber() from 0-based to 1-based to match V8's Stack Trace API, and makes getLineNumber()/getColumnNumber()/toJSON() return null for frames with no source position (OrdinalNumber::beforeFirst()) instead of clamping. ~15 lines of C++ in CallSitePrototype.cpp, two new tests in capture-stack-trace.test.js, and restoration of the vendored Node common/index.js getCallSite() helper to upstream's line-only form (plus its consumer in test-common-must-not-call.js).
Security risks
None. Read-only accessors on a diagnostics object; no allocation, no user-controlled input, no auth/crypto surface.
Level of scrutiny
Node/V8 compat behavior change to a user-visible API, so I checked for internal consumers that might depend on the old semantics. Bun__CallFrame__getLineNumber in bindings.cpp/jest.rs operates on the raw JSC CallFrame, not the JS CallSite prototype, so it's unrelated. The only src/js/ reference is commented-out code in internal/assert/utils.ts. The null return for native frames matches @types/node's number | null typing and V8's behavior; ecosystem consumers like source-map-support guard on getFileName() before subtracting, so the transition from clamped-0 to null on native frames is safe.
Other factors
My prior review's only finding (the toJSON() sibling not getting the beforeFirst() guard) was applied in 92b2507 and the native-frame test now asserts toJSON().{lineNumber,columnNumber} are both null. CodeRabbit's try/finally suggestion was correctly declined — the file's afterEach already restores Error.prepareStackTrace. The new tests follow harness conventions (subprocess with bunExe()/bunEnv, drain stdout/stderr/exited concurrently, assert exit code last), and the 1-based-column test pins an exact value (callerCol: 1) that fails on the unfixed build per the PR's USE_SYSTEM_BUN=1 output.
|
CI on build 78118: the diff is green. The two hard failures are pre-existing on main ( |
…ata (#104) * fix(errors): use one-based CallSite columns and consistent stack positions Adapt oven-sh#35179 and oven-sh#37396. Convert source coordinates at the public CallSite boundary, preserve missing native positions, remove the compensating util column increment, and share constructor position lookup with default stack formatting. Leave syntax-aware ordinary call locations and the WebKit pin unchanged. * fix(node): preserve metadata in fork heap and worker adapters The expanded CallSite CI selection exposes two pre-existing fork regressions, reproduced on the unchanged fork baseline. Include globalObjectCount in the fast heap adapter and retain inherited strict worker arguments without adding a duplicate. Existing API and strict-policy assertions remain unchanged and pass after the repair.
#105) #104 correctly moves constructor stack positions from the opening parenthesis after `Error` to the `new` keyword, but left five stale expectations in the WebKit upgrade and VM sourceURL tests. Both files were omitted from that PR's CI selection because they were only selected for WebKit pin changes. Update the exact constructor coordinates and derive the generated line-table expectations from `new Error(...)`. Keep all cases, exact comparisons, offset checks, line endings, and cached-bytecode checks. Select both suites on native CallSite, ErrorStack, FormatStackTraceForJS, and ZigSourceProvider changes, with selector regression coverage for Linux and macOS. Runtime code and the WebKit pin are unchanged. The complete Linux artifact bisect identifies **79bce2cfc3 (#104)** as the first failing commit. Both files pass (22 + 3 tests) at 61e5bf3, c999d9c, 1ac7fda, 72f794a, 9d0fb41, 65023a5, 32fd1a2, 02a67c2, 372d569, and 9297af6. #104 fails exactly 4 + 1 cases. Each predecessor artifact's CI merge tree was verified identical to its squash tree; #104 used its main CI artifact. Node 24.21.0 confirms every changed coordinate: | Case | Old expectation | Node and #104 | | --- | --- | --- | | VM constructor | 6:37 | 6:28 | | First-line VM constructor with offsets | 11:21 | 11:12 | | Later-line VM constructor with offsets | 12:16 | 12:7 | | Short eval constructor | 2:19 | 2:10 | | VM sourceURL constructor | hellohello.js:2:16 | hellohello.js:2:7 | The randomized line-table corpus additionally matches Node on all 512 positions across 8-/16-bit sources, line-ending variants, and parsed/cached bytecode. Existing ordinary-call differences from Node remain unchanged. Validation on Linux, using the unchanged #104 binary with the corrected tests: **444 pass, 0 fail** across the WebKit file (22), VM sourceURL (3), CI selector (14), CallSite (86), and VM (319). VM retains 3 existing skips and 60 todos. Prettier and diff checks pass. The required branch P2 review is scoped-clean. Both fork CI lanes are green on exact head `3e0a8df7b53bb683cb7f552e7ab95c76c8467653` in [run 37197000130](https://github.com/openclaw/bun/actions/runs/37197000130/attempts/3). Downloaded reports confirm both source-position files were selected and passed on Linux (22 selected files) and macOS (18 selected files). macOS required two same-head retries: attempt 1 failed the unchanged VM wall-clock timeout test; attempt 2 passed that test but failed the unchanged script-leak RSS limit at about 202 MiB against 200 MiB. Each failed test passed in the other attempt, and attempt 3 passed all 18 selected test files. Both source-position files passed in every attempt. No assertions, memory limits, selections, or runtime code were changed in response. The separate duplicate-review bot failed before review because its Anthropic credentials are not configured in the fork. Upstream status: oven-sh#35179, oven-sh#37396, and oven-sh#44559 remain open; #104 already adapts the native correction. This follow-up needs no additional runtime port.
Make JSC stack positions select the same syntax tokens as Node 24: call and constructor starts, property reads, and async `await` continuations. Keep those positions separate from exception-expression divots and debugger line tables. Both captured `StackFrame` and live `StackVisitor` readers use the metadata; executable line overrides and release-private builtin position policy are preserved. The two sparse vectors are owned by each unlinked code block, remapped through bytecode rewriting/optimization, and decoded into owned storage on both cache paths. The cache revision advances with the metadata. Property reads and calls retain distinct tokens, so `(null.x)()` can fail at `x` before reaching the call. The paired Bun adapter in [openclaw/bun#114](openclaw/bun#114) also preserves error constructors and runtime call syntax. Rewriting `new Error()` into `Error()` moved the reported column and made a returning arrow eligible for JSC proper-tail-call elision. Preserving `new` restores the frame without disabling tail calls. Callee parentheses, computed access, and opening delimiter mappings are required for the broader normal-transpile corpus; TypeScript generic/non-null suffixes and cache invalidation are covered. This continues the source-position work in [oven-sh/bun#35179](oven-sh/bun#35179), [WebKit#37396](oven-sh/bun#37396), and [WebKit#41580](oven-sh/bun#41580) by @robobun. Native Linux qualification on one c7a.24xlarge host, with the unchanged W113 lane/Docker/ICU recipe: - 1,509,853 FFI checks; 121 JSC stress configurations; interpreter/JIT/bytecode-optimizer/eager-FTL modes; owned and persistent cold/warm caches. Each cache path creates three files totaling 33,024 bytes. - Raw JSC positions improve from 13/30 to 30/30 in the call corpus and from 4/28 to 28/28 in the added cases. The live visitor matches Node at 1:22, 2:21, 3:1. Async continuation control passes. - Paired Bun: 47/47 fork-selection results with zero regressions, 343 CallSite/util/source-map tests, and 68 minifier tests. Normal transpilation matches all 30 call shapes, all 28 added cases on both stack surfaces, and 14 TypeScript/generic/non-null/astral cases. The JavaScript/TypeScript module corpus and 58 raw-eval rows match as well. - Runtime cache replacement and warm replay match all 56 added observations. Existing custom-stack generated-versus-mapped and filename/eval-origin policies are preserved. - OpenClaw loader pair 8/8, SDK under its existing native-loader policy 67 pass/1 skip, Slack ordered shared-worker sequence 26/26, and real Proxyline probe pass. The original shared SDK policy retains its same two baseline tsconfig resolver failures. - ABBA, eight samples per arm: exception creation/formatting +7.6%; fresh 350-module startup 0.998× baseline. The exception overhead is an explicit tradeoff. - Final complete engine and Bun branch reviews are scoped-clean through P2. Exact-head [CI run 37240058657](https://github.com/openclaw/WebKit/actions/runs/37240058657) is green at `6e69d757023aa75398a670f487f90fe2a57e2b99`. Qualification caught and corrected a namespace-only result-checker assumption, repeated C++ default arguments in unity builds, private-builtin position leakage, missing transpiler delimiter metadata, TypeScript non-null suffix tracking, and the omitted live-stack reader. Exact position goldens were checked against Node; the original astral inline-snapshot assertion remains intact and passes. The Bun draft remains stacked on manifest commit `cf636c2f14914b3ba4b319874312d654cc5ea064` and also needs the namespace adapter from Bun oven-sh#106. Native baseline and candidate both include that adapter because current engine main requires its API. This PR does not publish artifacts or change the artifact recipe; batch publication stays with the coordinator.
Connect Bun's stack reporting and transpiler to the syntax positions in the published OpenClaw WebKit engine integrated by #124. Calls, property reads, constructors, and async continuations use the engine's selected locations; returning built-in error constructors retain their frames and `new` syntax. Runtime transpilation preserves callee parentheses and computed access and maps call, bracket, and template delimiters back to the original source. Bump the runtime transpiler cache format to 38 so cached output from earlier versions cannot retain the old positions. Keep existing file-name and eval-origin formatting. Update the ErrorEvent inspection caret to the `new` token confirmed by Node. This continues the source-position work in oven-sh#35179, oven-sh#37396, and oven-sh#41580; thanks @robobun. Native macOS arm64 validation on candidate `8b774531c290dac5eeb35cd62b7bce517676a2e5`: - Stack, util, and source-map suites: 343 passed. - Minifier suite: 68 passed. Inspection suite: 95 passed, one existing skip. - Runtime cache replacement and marked cached-output replay pass at version 38, preserving all 56 coordinate assertions. - Rust checks: all 12 targets pass, with zero skips. Independent scoped P2 review and source lint pass. Unmodified OpenClaw `b02eab15853b6471d1869f2a2cb0f135b7aefe4d` consumers pass on the candidate and Node 24.21.0: SDK alias 67 passed with one existing skip; ordered loader pair 8 passed, with execution order, shared worker and canonical test policy verified. The plain Darwin selection has zero new regressions on the local macOS 27 host: candidate 20/21 rows versus baseline 19/21. The candidate's only failed file contains two socket-cancellation assertions reproduced unchanged with the pre-stack binary in isolation and the full selection. A baseline-only HTTP failure did not recur on the candidate. The fork-pinned Node 26.3.0 harness is used; the OpenClaw consumer controls use Node 24.21.0. No assertions or selections were weakened. Both exact-head native CI lanes pass in [run 37298205240](https://github.com/openclaw/bun/actions/runs/37298205240). The cache oracle improves from 2/56 matching positions on the baseline to 56/56 on both candidate runs. The committed production WebKit manifest remains unchanged. Formatting, source lints, JavaScript typechecking, Clippy, Miri and cargo tests pass. The Rust workflow succeeds; its explicitly advisory mordant job reports the unchanged preexisting bare-bool-argument style finding in `src/resolver/package_json.rs`, with no suppression or baseline change in this PR.
This branch has not been deployed
1 participant
Problem
CallSite#getColumnNumber()returned a 0-based column. V8 (and Node.js) return 1-based.CallSite#toString()already prints the 1-based column, so Bun was inconsistent with itself.This bit
source-map-support:wrapCallSitedoesframe.getColumnNumber() - 1to obtain a 0-based index fororiginalPositionFor(). On a Bun frame whose (1-based) column is 1, that yields-1and the consumer throws:Reported as #17303 (
medusa build) and #18250 (nestjs/swc).Repro
Cause
callSiteProtoFuncGetColumnNumberinsrc/jsc/bindings/CallSitePrototype.cppreturnedstd::max(columnNumber().zeroBasedInt(), 0). The comment next to it links tomozilla/source-map's 0-based consumer API, but the V8 Stack Trace API contract forCallSiteis 1-based, and ecosystem code subtracts 1 to reach the 0-based consumer.Fix
getColumnNumber()now returnsoneBasedInt(), matching V8 and making it consistent withtoString().getLineNumber()/getColumnNumber()now returnnullwhen the frame has no source position (native frames), matching V8 and@types/node'snumber | null. Previously they were clamped to1/0.toJSON().lineNumber/columnNumberfollow the same rules (Bun-specific method; no external consumers).Nothing in
src/reads the JS-levelgetColumnNumber()return value. One test fixture did:test/js/node/test/common/index.js'sgetCallSite()carried a Bun-local:${getColumnNumber()}suffix whose pinned value intest-common-must-not-call.jsencoded the old 0-based column. Both are restored to the upstream Node line-only form, decoupling the vendored test from CallSite column semantics; column coverage now lives incapture-stack-trace.test.js.Verification
Fail-before (
USE_SYSTEM_BUN=1):Fixes #17303
Fixes #18250
[review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file