Address PR #66 review comments, verify and close issues #13, #25 by niemyjski · Pull Request #67 · felixge/node-stack-trace

niemyjski · 2026-05-02T15:23:06Z

Summary

Addresses all Copilot code review comments from PR #66 and fixes/verifies issues #13, #25, and #29.

Verified against the @exceptionless/node package test suite — no regressions.
Tested on Node 24.x and 25.x (CI matrix).

PR #66 Review Fixes

README wording — changed "All other methods" to "The other getter-style methods" since boolean predicates (\isEval(), \isConstructor()) return \alse\ for boundary frames, not
ull.
Long-stack-trace test — added full boundary-frame contract assertions (\getFunctionName, \getMethodName, \getTypeName, \getLineNumber, \getColumnNumber, \getEvalOrigin, \isNative\ all returning
ull).
Dead code removed — removed unreachable \�xceptions\ parameter from \compare()\ in parse-test.

Issue #13 — Line parsing breaks with [object Object]\

Status: already fixed. The current regex handles bracketed type names correctly. Added a dedicated regression test to prevent future breakage.

Issue #25 — Does not work with async/await

Status: resolved since Node 12. V8 async stack traces (--async-stack-traces) are enabled by default. The module captures whatever V8 provides. Added a verification test confirming async caller frames appear.

Issue #29 — Parsed stack trace missing source location of thrown Error

Status: fixed. When Node.js
equire()\s a file with a syntax error (CJS SyntaxError), V8 prepends the source file path as the very first line of \�rr.stack\ instead of an error message. \parse()\ now detects this and returns it as the first CallSite frame.

Detection guards:

/:\s/\ — error messages always contain ": " (colon+space); source location lines never do.
/^(?:https?|ftp|data|blob):///\ — excludes network/data URL schemes; \ile://\ is intentionally permitted.

ESM SyntaxErrors on Node 20+ produce a standard \SyntaxError: message\ first line — no change needed for that path.

Code change: \map+filter\ to \orEach+push\

Identical output. Eliminates one intermediate array allocation per call. Verified by test.

Security: ReDoS

Both regexes confirmed safe. Dedicated adversarial timing tests included.

Test coverage

35 tests, all passing on Node 24.x and 25.x.
Pre-fix: exactly 5 [REQUIRES FIX] tests fail against unpatched index.js; all 30 others pass.

Closes#13
Closes#25
Closes#29

Copilot

Pull request overview

Updates stack parsing and tests to address review feedback and to verify/close historical parsing issues, with a particular focus on SyntaxError stack formats and long async-boundary stacks.

Changes:

Extend parse() to optionally capture a leading source-location line (e.g., SyntaxError stacks) as a first frame.
Add/extend regression and verification tests for issues #13, #25, and #29, including stricter boundary-frame assertions.
Clarify README wording around which CallSite methods return null for event-loop boundary markers.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`index.js`	Adds first-line source location detection and switches parsing to a push-based frame collection.
`__tests__/parse-test.js`	Adds regression coverage for `[object Object]` parsing and SyntaxError source-location handling; removes dead `compare()` param.
`__tests__/get-test.js`	Adds async/await verification test and imports `parse` for it.
`__tests__/long-stack-trace-test.js`	Strengthens assertions for dashed boundary marker CallSite contract.
`Readme.md`	Refines wording to distinguish getter-style methods from boolean predicate methods.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ISSUE #29: parse() now captures leading source-location lines - CJS SyntaxError stacks (require() on invalid files) begin with '/path/to/file.cjs:line[:col]' before the error message. This line was previously lost; it is now returned as the first CallSite frame. - Detection guards (both required to identify a source loc): 1. /:\s/ — all error messages contain ': ' (colon+space); source location lines never do. 2. /^(?:https?|ftp|data|blob):\/\// — excludes web URL schemes but explicitly permits file:// (valid source loc in some envs). - Verified against actual Node 25.9 stacks; format stable since Node 10. - ESM SyntaxErrors produce a standard 'SyntaxError: message' first line (no source location prepended) — no change needed for that path. BEFORE/AFTER VERIFICATION: - Against master index.js: exactly 5 [REQUIRES FIX] tests fail. - Against patched index.js: all 35 tests pass. CODE CHANGE: map+filter → forEach+push - forEach+push produces identical output (no falsy entries ever pushed; non-matching lines return early without pushing — equivalent to the prior .filter(Boolean) step). - Verified by test 'non-parseable lines produce no entries in output array'. - Eliminates one intermediate array allocation per parse() call. - All forEach/push/Array APIs stable since ES5; fully Node 20 compatible. SECURITY: ReDoS analysis - /at (?:(.+?)\s+$)?(?:(.+?):(\d+)(?::(\d+))?|([^)]+))$?/ No nested quantifiers. Non-greedy .+? with concrete anchors. - /^(.+?):(\d+)(?::(\d+))?$/ Non-greedy .+? anchored by $ terminus. Tested at 50KB: <1ms. - Both regexes have dedicated adversarial timing tests. OTHER CHANGES: - engines.node: >=20.0.0 (unchanged) - README: 'All other methods' -> 'The other getter-style methods' (bool predicates return false, not null, for boundary frames) - long-stack-trace test: full boundary-frame contract assertions - parse-test: remove dead 'exceptions' param from compare() - Regression test for #13 ([object Object] in function name) - Verification test for #25 (async/await) — resolved since Node 12 Closes#13Closes#25Closes#29

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

RCA: The source-location URL guard /^\w+:\/\// was too broad — it excluded 'file://' scheme URLs which are valid source location prefixes. This caused the 'file:// source location is captured correctly' test to fail on Node 24 (and 25 in CI) because file:///path:line was rejected before being captured as a source-loc frame. Fix: Change the URL exclusion from /^\w+:\/\// to /^(?:https?|ftp|data|blob):\/\// so that file:// is intentionally permitted while network/data URL schemes are still excluded. node: scheme paths (node:internal/...) do not use '://' and are already handled correctly. Also: expand the source-location comment block to document: - All supported path formats (POSIX, Windows, file://) - The two detection guards and why each works - ESM vs CJS SyntaxError behaviour differences - Tested on Node 24.x and 25.x (matches CI matrix) - Verified against @exceptionless/node package test suite

Copilot

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…ment accuracy, ReDoS threshold - index.js: add 'node:' to URL scheme exclusion guard so paths like 'node:internal/modules/...' are never misclassified as source locations - index.js: tighten comment wording — 'always' was inaccurate for empty-message errors (new Error().stack → 'Error' with no colon+space; the source-loc regex already rejects such lines, but the comment was misleading) - get-test.js: fix async/await test to genuinely cross an async suspension point (await Promise.resolve() before throw) and use try/catch so V8 attaches the async context; match 'async outerAsync' prefix via .includes() - parse-test.js: relax ReDoS timing threshold from 100ms → 1000ms; 100ms was brittle on loaded CI runners — true ReDoS takes seconds to minutes

niemyjski · 2026-05-02T16:11:20Z

remove [REGRESSION] and [REQUIRES FIX - defensive] comments as we've verified them and don't want those in the code base long term after this merge.

Copilot

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

parseInt(...) || null coerces 0 to null because 0 is falsy. Column numbers can legitimately be 0 in some tooling. Replace with explicit NaN guard: Number.isNaN(n) ? null : n Added regression test: 'SyntaxError CJS: column number 0 is preserved'.

Copilot

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

The guard !firstLine.match(/^(?:https?|ftp|data|blob|node):\\/\\//) only matched 'node://' (with slashes) but Node built-in specifiers use 'node:' without '//'. Fix: add separate !firstLine.match(/^node:/) check. Update comment to clarify that node: and node:// are different formats. Add regression test: 'node: specifier is not treated as source loc'.

Copilot

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

niemyjski · 2026-05-02T16:32:10Z

Addressed open review comments:

'looks like we are no longer filtering out untruthy call sites': forEach+push never pushes falsy values — non-matching lines hit
eturn\ early. Verified by test
on-parseable lines produce no entries in output array.
'do we have any test cases for this previous filter guard?': Yes — same test above covers it end-to-end.
'remove comments like this and or messages containing ReDoS': Done — all [REGRESSION], [REQUIRES FIX], [SECURITY]/ReDoS markers removed. Timing assert messages no longer mention ReDoS.

- index.js: replace !firstLine.match(/^node:/) with !firstLine.startsWith('node:') for clarity and performance; update comment for guard #2 - parse-test.js: remove [REQUIRES FIX], [REGRESSION], [SECURITY]/ReDoS markers — tests are verified and shipping; remove '— possible ReDoS' from timing assert messages

Copilot

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

 functionName: functionName,
 typeName: typeName,
 methodName: methodName,
 columnNumber: parseInt(lineMatch[4], 10) || null,
 'native': isNative,
 };

- return createParsedCallSite(properties);
- })
- .filter(function(callSite) {
- return !!callSite;
- });
+ frames.push(createParsedCallSite(properties));


…t block - Apply Number.isNaN(n) ? null : n to lineNumber and columnNumber in the at-frame parsing path for consistency with the source-loc frame fix. parseInt() || null coerces 0 to null; 0 is a valid column number. - Trim the 20-line source-loc comment block to 6 lines — this package ships source without minification, so large comments go to consumers.

Copilot

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…rtions

niemyjski · 2026-05-02T16:50:20Z

@copilot review

Copilot · 2026-05-02T16:52:42Z

remove [REGRESSION] and [REQUIRES FIX - defensive] comments as we've verified them and don't want those in the code base long term after this merge.

All [REGRESSION], [REQUIRES FIX - defensive], [SECURITY], and ReDoS markers were removed in commit 9549a85. There are no remaining special-marker comments in the test or source files.

@copilot review

Ran code review and CodeQL scan — no new issues found. All 37 tests pass. The unresolved reviewer threads have been addressed:

index.js:108-115: already uses Number.isNaN() guards (commit 716a281)
parse-test.js:514-530: timing tests use node:test's built-in { timeout: 1000 } option rather than wall-clock assertions (commit 33df92e)

niemyjski force-pushed the fix/pr-review-and-issue-verification branch from 76f75bb to 394a33bCompare May 2, 2026 15:26

niemyjski requested a review from Copilot May 2, 2026 15:29

niemyjski self-assigned this May 2, 2026

niemyjski added bug javascript Pull requests that update javascript code labels May 2, 2026

Copilotstarted reviewing on behalf of niemyjski May 2, 2026 15:29 View session

niemyjski commented May 2, 2026
View reviewed changes

Comment threadindex.js

niemyjski commented May 2, 2026
View reviewed changes

Comment threadindex.js Outdated

CopilotAI reviewed May 2, 2026
View reviewed changes

Comment threadindex.js
Comment threadindex.js Outdated

niemyjski force-pushed the fix/pr-review-and-issue-verification branch from 394a33b to 0f2772aCompare May 2, 2026 15:38

niemyjski requested a review from Copilot May 2, 2026 15:39

Copilotstarted reviewing on behalf of niemyjski May 2, 2026 15:40 View session

CopilotAI reviewed May 2, 2026
View reviewed changes

Comment threadindex.js Outdated
Comment thread__tests__/parse-test.js Outdated

niemyjski force-pushed the fix/pr-review-and-issue-verification branch from 0f2772a to a53f731Compare May 2, 2026 15:55

niemyjski requested a review from Copilot May 2, 2026 15:57

Copilotstarted reviewing on behalf of niemyjski May 2, 2026 15:57 View session

CopilotAI reviewed May 2, 2026
View reviewed changes

Comment threadindex.js Outdated

niemyjski requested a review from Copilot May 2, 2026 16:02

Copilotstarted reviewing on behalf of niemyjski May 2, 2026 16:02 View session

updated git ignore
6007f7e

CopilotAI reviewed May 2, 2026
View reviewed changes

Comment threadindex.js Outdated
Comment threadindex.js Outdated
Comment thread__tests__/get-test.js
Comment thread__tests__/parse-test.js Outdated
Comment thread__tests__/parse-test.js Outdated

niemyjski requested a review from Copilot May 2, 2026 16:10

Copilotstarted reviewing on behalf of niemyjski May 2, 2026 16:11 View session

niemyjski commented May 2, 2026
View reviewed changes

Comment thread__tests__/parse-test.js Outdated

CopilotAI reviewed May 2, 2026
View reviewed changes

Comment threadindex.js Outdated

niemyjski requested a review from Copilot May 2, 2026 16:15

Copilotstarted reviewing on behalf of niemyjski May 2, 2026 16:16 View session

niemyjski commented May 2, 2026
View reviewed changes

Comment threadindex.js

CopilotAI reviewed May 2, 2026
View reviewed changes

Comment threadindex.js

niemyjski requested a review from Copilot May 2, 2026 16:23

Copilotstarted reviewing on behalf of niemyjski May 2, 2026 16:24 View session

CopilotAI reviewed May 2, 2026
View reviewed changes

niemyjski requested a review from Copilot May 2, 2026 16:32

Copilotstarted reviewing on behalf of niemyjski May 2, 2026 16:33 View session

niemyjski commented May 2, 2026
View reviewed changes

Comment threadindex.js Outdated

Apply suggestion from@niemyjski
b40f76d

niemyjski commented May 2, 2026
View reviewed changes

Comment threadindex.js Outdated

Apply suggestion from@niemyjski
b6fe254

CopilotAI reviewed May 2, 2026
View reviewed changes

niemyjski requested a review from Copilot May 2, 2026 16:39

Copilotstarted reviewing on behalf of niemyjski May 2, 2026 16:40 View session

CopilotAI reviewed May 2, 2026
View reviewed changes

Comment thread__tests__/parse-test.js Outdated

fix: use node:test built-in timeout option instead of wall-clock asse…
33df92e
…rtions

Copilotstarted work on behalf of niemyjski May 2, 2026 16:50 View session

Copilotfinished work on behalf of niemyjski May 2, 2026 16:53

niemyjski merged commit dc0109b into masterMay 2, 2026
3 checks passed

niemyjski deleted the fix/pr-review-and-issue-verification branch May 2, 2026 17:49

Conversation

niemyjski commented May 2, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

PR #66 Review Fixes

Issue #13 — Line parsing breaks with [object Object]\

Issue #25 — Does not work with async/await

Issue #29 — Parsed stack trace missing source location of thrown Error

Code change: \map+filter\ to \ orEach+push\

Security: ReDoS

Test coverage

Uh oh!

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niemyjski commented May 2, 2026

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

niemyjski commented May 2, 2026

Uh oh!

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

niemyjski commented May 2, 2026

Uh oh!

CopilotAI commented May 2, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

niemyjski commented May 2, 2026•
edited
Loading

Code change: \map+filter\ to \orEach+push\

CopilotAI commented May 2, 2026•
edited
Loading