GitHub Actions annotation and code frame caret: use the first frame that has a file, not a builtin frame on top

src/jsc/VirtualMachine.rs

@@ -5600,14 +5600,7 @@ impl VirtualMachine {
         let mut top_frame_is_builtin = false;
         if self.hide_bun_stackframes {
             for (i, frame) in frames.iter().enumerate() {
-                if frame.source_url.has_prefix_comptime(b"bun:")
-                    || frame.source_url.has_prefix_comptime(b"node:")
-                    || frame.source_url.is_empty()
-                    || frame.source_url.eql_comptime("native")
-                    || frame.source_url.eql_comptime("unknown")
-                    || frame.source_url.eql_comptime("[unknown]")
-                    || frame.source_url.has_prefix_comptime(b"[source:")
-                {
+                if !frame.has_user_source() {
                     top_frame_is_builtin = true;
                     continue;
                 }
@@ -6123,10 +6116,7 @@ impl VirtualMachine {
                 let mut top_frame: Option<&crate::ZigStackFrame> = frames.first();
                 if self.hide_bun_stackframes {
                     for frame in frames {
-                        if frame.position.is_invalid()
-                            || frame.source_url.has_prefix_comptime(b"bun:")
-                            || frame.source_url.has_prefix_comptime(b"node:")
-                        {
+                        if frame.position.is_invalid() || !frame.has_user_source() {
                             continue;
                         }
                         top_frame = Some(frame);
@@ -6549,29 +6539,26 @@ impl VirtualMachine {
         let name = &exception.name;
         let message = &exception.message;
         let frames = exception.stack.frames();
-        let top_frame = frames.first();
+        let location_frame = frames
+            .iter()
+            .find(|frame| frame.has_user_source() && !frame.position.is_invalid());
         let dir = bun_core::env_var::GITHUB_WORKSPACE::get()
             .unwrap_or_else(|| bun_bundler::bun_fs::FileSystem::instance().top_level_dir);
         bun_core::Output::flush();
 
         let writer = bun_core::Output::error_writer();
 
-        let mut has_location = false;
-        if let Some(frame) = top_frame {
-            if !frame.position.is_invalid() {
-                let source_url = frame.source_url.to_utf8();
-                let file = bun_paths::resolve_path::relative(dir, source_url.slice());
-                let _ = write!(
-                    writer,
-                    "\n::error file={},line={},col={},title=",
-                    bun_core::fmt::github_action_property(file),
-                    frame.position.line.one_based(),
-                    frame.position.column.one_based(),
-                );
-                has_location = true;
-            }
-        }
-        if !has_location {
+        if let Some(frame) = location_frame {
+            let source_url = frame.source_url.to_utf8();
+            let file = bun_paths::resolve_path::relative(dir, source_url.slice());
+            let _ = write!(
+                writer,
+                "\n::error file={},line={},col={},title=",
+                bun_core::fmt::github_action_property(file),
+                frame.position.line.one_based(),
+                frame.position.column.one_based(),
+            );
+        } else {
             let _ = writer.write_all(b"\n::error title=");
         }
 
@@ -6612,7 +6599,7 @@ impl VirtualMachine {
             let _ = writer.write_all(b"::");
         }
 
-        if top_frame.is_some() {
+        if !frames.is_empty() {
             // SAFETY: per-thread VM.
             let vm = VirtualMachine::get();
             let origin = if vm.is_from_devserver {

src/jsc/ZigStackFrame.rs

@@ -75,6 +75,22 @@ impl ZigStackFrame {
         jsc_stack_frame_index: -1,
     };
 
+    /// Whether `source_url` names a source the user can open. False for JSC's
+    /// JS builtins (no URL, or `native`/`unknown` once parsed back out of
+    /// `error.stack`), for Bun's own `bun:`/`node:` modules, and for sources
+    /// JSC could not attribute. The code frame and the GitHub Actions
+    /// annotation point at the first frame for which this is true.
+    pub(crate) fn has_user_source(&self) -> bool {
+        let url = &self.source_url;
+        !(url.is_empty()
+            || url.has_prefix_comptime(b"bun:")
+            || url.has_prefix_comptime(b"node:")
+            || url.eql_comptime("native")
+            || url.eql_comptime("unknown")
+            || url.eql_comptime("[unknown]")
+            || url.has_prefix_comptime(b"[source:"))
+    }
+
     pub fn name_formatter(&self, enable_color: bool) -> NameFormatter {
         NameFormatter {
             function_name: self.function_name,

test/cli/test/bun-test.test.ts

@@ -719,6 +719,78 @@ describe("bun test", () => {
       });
       expect(stderr).toMatch(/::error title=error: Test \"time out\" timed out after \d+ms::/);
     });
+    // The frame on top of these stacks has no file to annotate: a JS builtin
+    // (`reduce`), the `native` / `unknown` placeholders such frames turn into
+    // once error.stack has been read, or one of bun's own `node:*` modules.
+    // The annotation has to point at the first frame below it that is in the
+    // test file. The test callbacks are async so the error reaches the
+    // reporter with its own stack; a synchronous throw is reported with the
+    // frames of the throw site instead, which would not have these on top.
+    test.each([
+      {
+        label: "a JS builtin",
+        body: `[].reduce((a, b) => a);`,
+        callee: "reduce",
+        title: "TypeError: reduce of empty array with no initial value",
+      },
+      {
+        label: "a JS builtin after error.stack was read",
+        body: `try { [].reduce((a, b) => a); } catch (e) { void e.stack; throw e; }`,
+        callee: "reduce",
+        title: "TypeError: reduce of empty array with no initial value",
+      },
+      {
+        label: "a node: module",
+        body: `new EventEmitter().emit("error");`,
+        callee: "emit",
+        title: "error: Unhandled error. (undefined)",
+      },
+      {
+        label: "a node: module after error.stack was read",
+        body: `try { new EventEmitter().emit("error"); } catch (e) { void e.stack; throw e; }`,
+        callee: "emit",
+        title: "error: Unhandled error. (undefined)",
+      },
+    ])("should annotate the first frame in the test file when the top frame is $label", ({ body, callee, title }) => {
+      const lines = [
+        `import { test } from "bun:test";`,
+        `import { EventEmitter } from "node:events";`,
+        `test("fail", async () => {`,
+        `  ${body}`,
+        `});`,
+      ];
+      const line = lines.findIndex(l => l.includes(body)) + 1;
+      const col = lines[line - 1].indexOf(callee) + 1;
+      const stderr = runTest({
+        input: [{ filename: "top-frame-has-no-file.test.ts", contents: lines.join("\n") }],
+        env: {
+          GITHUB_ACTIONS: "true",
+        },
+      });
+      const annotation = stderr.split("\n").find(l => l.startsWith("::error"));
+      expect(annotation).toStartWith("::error file=");
+      expect(annotation!.replace(/^::error file=(?:[^,]*[\\/])?/, "")).toStartWith(
+        `top-frame-has-no-file.test.ts,line=${line},col=${col},title=${title}::`,
+      );
+    });
+    test("should annotate without a location when no frame has a file", () => {
+      const stderr = runTest({
+        input: `
+          import { test } from "bun:test";
+          test("fail", async () => {
+            const err = new Error("boom");
+            err.stack = "Error: boom\\n    at reduce (native:1:11)";
+            throw err;
+          });
+        `,
+        env: {
+          GITHUB_ACTIONS: "true",
+        },
+      });
+      const annotation = stderr.split("\n").find(l => l.startsWith("::error"));
+      expect(annotation).toStartWith("::error title=error: boom::");
+      expect(annotation).toContain("at reduce (");
+    });
   });
   describe(".each", () => {
     test("should run tests with test.each", () => {

test/js/bun/test/stack.test.ts

@@ -149,3 +149,30 @@ test("Async functions frame should be included in stack trace", async () => {
         at async <anonymous> (file:NN:NN)"
   `);
 });
+
+// `reduce` throws from inside JSC's builtin, so the first frame has a
+// line:column inside the builtin's source and no file. The code frame shows
+// this file, so its caret has to be placed by this file's frame as well. Once
+// error.stack has been read, the frames are parsed back out of that string
+// and the builtin frame turns into the `native` placeholder.
+test.each([
+  ["fresh error", false],
+  ["error whose .stack has been read", true],
+])("code frame caret is placed by the first frame with a file, not a builtin frame on top: %s", (_, readStack) => {
+  let err: unknown;
+  try {
+    const emptyArray: number[] = [];
+    emptyArray.reduce((a, b) => a);
+  } catch (e) {
+    err = e;
+  }
+  if (readStack) void (err as Error).stack;
+
+  const lines = Bun.inspect(err).split("\n");
+  const header = lines.indexOf("TypeError: reduce of empty array with no initial value");
+  expect(header).toBeGreaterThanOrEqual(2);
+  const [sourceLine, caretLine] = lines.slice(header - 2, header);
+  expect(sourceLine).toEndWith("emptyArray.reduce((a, b) => a);");
+  expect(caretLine.trim()).toBe("^");
+  expect(caretLine.indexOf("^")).toBe(sourceLine.indexOf("reduce"));
+});