feat(vscode): add terminal debugging support via console configuration#29217
Open
flyor2017 wants to merge 1 commit intooven-sh:mainfrom
Open
feat(vscode): add terminal debugging support via console configuration#29217flyor2017 wants to merge 1 commit intooven-sh:mainfrom
flyor2017 wants to merge 1 commit intooven-sh:mainfrom
Conversation
Add support for launching Bun debugger in integrated or external terminal via the 'console' setting in launch.json. This allows debugging programs that require terminal input or need to run in a real terminal environment. Changes: - Add runInTerminal reverse request support in debug adapter - Handle 'integratedTerminal' and 'externalTerminal' console options - Pass proper environment variables for terminal debugging Fixes oven-sh#15706
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Author
|
收件成功。---------------------------------------------------------This is an automatic reply, confirming that your e-mail was received.Thank you
|
Contributor
WalkthroughImplemented support for the Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bun-debug-adapter-protocol/src/debugger/adapter.ts`:
- Around line 387-396: The reverseRequest method currently returns a Promise
that only resolves via the cb passed to emit("Adapter.reverseRequest", ...),
causing hangs if no handler responds; modify reverseRequest (method name:
reverseRequest<T extends keyof DAP.RequestMap>) to detect if this.emit(...)
returns false and immediately reject the Promise with an appropriate error, and
also add a short configurable timeout (setTimeout) that rejects the Promise if
cb is not invoked within the timeout window; ensure you wire both resolve and
reject into the emitted callback (replace cb: resolve with cb: (res)=>{
clearTimeout(timer); resolve(res) } and call reject on emit-failure or timeout,
and clear the timer on resolve/reject to avoid leaks.
🪄 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: 9f1eb443-94dc-4dea-bec7-b8de48bad5ab
📒 Files selected for processing (3)
packages/bun-debug-adapter-protocol/src/debugger/adapter.tspackages/bun-vscode/package.jsonpackages/bun-vscode/src/features/debug.ts
Comment on lines
+387
to
396
| protected reverseRequest<T extends keyof DAP.RequestMap>(command: T, args?: DAP.RequestMap[T]): Promise<DAP.Response> { | ||
| return new Promise(resolve => { | ||
| this.emit("Adapter.reverseRequest", { | ||
| type: "request", | ||
| seq: 0, | ||
| command, | ||
| arguments: args, | ||
| cb: resolve, | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Fail fast when no reverse-request handler answers.
This Promise only settles through cb. If emit("Adapter.reverseRequest", ...) returns false, or a handler forgets to invoke cb, terminal launches hang forever.
Suggested fix
protected reverseRequest<T extends keyof DAP.RequestMap>(command: T, args?: DAP.RequestMap[T]): Promise<DAP.Response> {
- return new Promise(resolve => {
- this.emit("Adapter.reverseRequest", {
+ return new Promise((resolve, reject) => {
+ const timeoutId = setTimeout(() => reject(new Error(`Reverse request timed out: ${command}`)), 15_000);
+ const sent = this.emit("Adapter.reverseRequest", {
type: "request",
seq: 0,
command,
arguments: args,
- cb: resolve,
+ cb: response => {
+ clearTimeout(timeoutId);
+ resolve(response);
+ },
});
+
+ if (!sent) {
+ clearTimeout(timeoutId);
+ reject(new Error(`No reverse-request handler registered for ${command}`));
+ }
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| protected reverseRequest<T extends keyof DAP.RequestMap>(command: T, args?: DAP.RequestMap[T]): Promise<DAP.Response> { | |
| return new Promise(resolve => { | |
| this.emit("Adapter.reverseRequest", { | |
| type: "request", | |
| seq: 0, | |
| command, | |
| arguments: args, | |
| cb: resolve, | |
| }); | |
| }); | |
| protected reverseRequest<T extends keyof DAP.RequestMap>(command: T, args?: DAP.RequestMap[T]): Promise<DAP.Response> { | |
| return new Promise((resolve, reject) => { | |
| const timeoutId = setTimeout(() => reject(new Error(`Reverse request timed out: ${command}`)), 15_000); | |
| const sent = this.emit("Adapter.reverseRequest", { | |
| type: "request", | |
| seq: 0, | |
| command, | |
| arguments: args, | |
| cb: response => { | |
| clearTimeout(timeoutId); | |
| resolve(response); | |
| }, | |
| }); | |
| if (!sent) { | |
| clearTimeout(timeoutId); | |
| reject(new Error(`No reverse-request handler registered for ${command}`)); | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bun-debug-adapter-protocol/src/debugger/adapter.ts` around lines 387
- 396, The reverseRequest method currently returns a Promise that only resolves
via the cb passed to emit("Adapter.reverseRequest", ...), causing hangs if no
handler responds; modify reverseRequest (method name: reverseRequest<T extends
keyof DAP.RequestMap>) to detect if this.emit(...) returns false and immediately
reject the Promise with an appropriate error, and also add a short configurable
timeout (setTimeout) that rejects the Promise if cb is not invoked within the
timeout window; ensure you wire both resolve and reject into the emitted
callback (replace cb: resolve with cb: (res)=>{ clearTimeout(timer);
resolve(res) } and call reject on emit-failure or timeout, and clear the timer
on resolve/reject to avoid leaks.
Author
There was a problem hiding this comment.
The handler for Adapter.reverseRequest already has a timeout mechanism.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
Let me check the handler in debug.ts to confirm the existing timeout mechanism.
🏁 Script executed:
#!/bin/bash
# Look for the Adapter.reverseRequest handler in debug.ts
fd -e ts debug.ts --exec grep -n "reverseRequest\|timeout\|Timeout\|setTimeout" {} +
# Also show broader context around the reverseRequest handler
fd -e ts debug.ts --exec grep -n -A 20 -B 5 "reverseRequest" {} +
Length of output: 2814
@flyor2017, you're right — the handler in packages/bun-vscode/src/features/debug.ts (Line 413–414) already passes a 5-second timeout as the third argument to sendRequest:
this.adapter.on("Adapter.reverseRequest", ({ command, arguments: args, cb }) =>
this.sendRequest(command, args, 5000, (resp) => cb(resp as DAP.Response))
);The VS Code DAP SDK's sendRequest will invoke the callback with a timeout/error response after 5 s, which in turn calls cb and settles the Promise — so hangs are already prevented in normal VS Code usage.
The only remaining edge case is if emit("Adapter.reverseRequest", ...) returns false (no listener registered at all), in which case cb would never be called and the Promise would hang indefinitely. However, since the handler is always registered during session setup before #launch is called, this is a very unlikely scenario in practice. You can disregard that part of my original comment.
✏️ Learnings added
Learnt from: flyor2017
Repo: oven-sh/bun PR: 29217
File: packages/bun-debug-adapter-protocol/src/debugger/adapter.ts:387-396
Timestamp: 2026-04-12T09:36:03.461Z
Learning: In `packages/bun-vscode/src/features/debug.ts` (oven-sh/bun), the `Adapter.reverseRequest` event handler already applies a built-in 5-second timeout via `this.sendRequest(command, args, 5000, cb)`. The VS Code DAP SDK invokes the callback with a failure response on timeout, which settles the Promise returned by `reverseRequest()` in `adapter.ts`. Do not flag `reverseRequest` as lacking a timeout in this codebase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: robobun
Repo: oven-sh/bun PR: 28754
File: src/bun.js/web_worker.zig:449-453
Timestamp: 2026-04-01T19:29:31.324Z
Learning: In oven-sh/bun `src/bun.js/web_worker.zig`, the non-concurrent `unref()` call inside `notifyNeedTermination()` (via `setRefInternal(false)`) is a known pre-existing issue, flagged by the `// TODO(190n) delete` comment. The `onUnhandledRejection` handler intentionally bypasses `notifyNeedTermination()` and instead inlines the concurrent-safe sequence (`setRequestedTerminate()` + `parent_poll_ref.unrefConcurrently()` + `vm.eventLoop().wakeup()`). Do not flag the `notifyNeedTermination()` non-concurrent unref path as a new issue introduced by PRs that touch `onUnhandledRejection`.
Learnt from: robobun
Repo: oven-sh/bun PR: 28686
File: test/regression/issue/12306.test.ts:50-55
Timestamp: 2026-04-02T18:42:38.406Z
Learning: In `test/regression/issue/12306.test.ts` (oven-sh/bun PR `#28686`), the `done` Promise only rejects via the client `error` event — a genuine test failure. In that case, bun:test catches the unhandled rejection and reports the error, and the test process exits immediately, making the skipped `server.close()` harmless. Do not flag the missing try/finally around `await done` in this test as a resource-leak risk; the process-exit path is the correct and sufficient cleanup signal for this regression test.
Learnt from: robobun
Repo: oven-sh/bun PR: 28732
File: test/regression/issue/13184.test.ts:16-20
Timestamp: 2026-04-01T16:21:59.448Z
Learning: In `test/regression/issue/13184.test.ts` (oven-sh/bun PR `#28732`), do NOT add a `socket.destroyed` assertion inside `testServerCloseCompletes`. The test's regression signal is `server.close()` completing without hanging: if the socket is not destroyed, `_connections` is never decremented and `server.close()` times out. A direct `expect(socket.destroyed).toBe(true)` assertion was tried and caused intermittent failures under ASAN builds. The `server.close()` timeout failure is the correct and sufficient signal for this regression.
Learnt from: robobun
Repo: oven-sh/bun PR: 28397
File: src/js/node/_http_client.ts:286-295
Timestamp: 2026-03-21T18:16:39.742Z
Learning: In `src/js/node/_http_client.ts`, the `startFetchViaSocket()` function's `createConnection` invocation intentionally supports only the synchronous return pattern (`socket = createConnection(connectOptions)`). The callback-style async socket creation (`createConnection(opts, (err, sock) => {...})`) used by some custom tunneling agents is out of scope for PR `#28397`. This is not a regression — `createConnection` was completely ignored before this PR. Callback-style support is a follow-up enhancement. Do not flag the absence of async callback handling in `startFetchViaSocket()` during reviews of this code path.
Learnt from: robobun
Repo: oven-sh/bun PR: 28827
File: test/js/bun/udp/udp_socket_recv_flags.test.ts:23-29
Timestamp: 2026-04-03T23:34:56.306Z
Learning: In `test/js/bun/udp/udp_socket_recv_flags.test.ts` and similar UDP socket test files in oven-sh/bun (e.g., `test/js/bun/udp/`), the `sendRec` + `setTimeout(sendRec, 10)` pattern is an intentional, suite-wide convention for retrying UDP sends until the socket is closed. This is NOT a violation of the "do not use setTimeout in tests" guideline — that guideline targets timing-based assertions (using setTimeout as a proxy for awaiting a condition), not send-retry loops that drive socket traffic until the test resolves. Do not flag this pattern in UDP send tests.
Learnt from: robobun
Repo: oven-sh/bun PR: 28748
File: src/cli.zig:998-1002
Timestamp: 2026-04-02T21:02:25.201Z
Learning: In oven-sh/bun `src/cli/run_command.zig`, adding `bootFromStdin` to the `.RunAsNodeCommand` path (i.e., when `argv0 == "node"` and `ctx.positionals.len == 0`) causes REPL test deadlocks. Tests like `test-repl-dynamic-import.js`, `test-repl-close.js`, and `as-node.test.ts` spawn bun-as-node with a piped stdin and write incrementally; `streamRemaining()` blocks until EOF, so the process hangs waiting for input that never ends. Node shim stdin bootstrapping requires an incremental (non-blocking) read approach and is intentionally deferred to a separate PR. Do NOT flag the absence of `bootFromStdin` in the `RunAsNodeCommand` / `execAsIfNode` no-positionals path as a missing feature parity issue — it was deliberately reverted (commit 79adac4 in PR `#28748`) to avoid these deadlocks.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 28863
File: scripts/build/deps/webkit.ts:149-161
Timestamp: 2026-04-04T19:43:49.607Z
Learning: When reviewing Node/TypeScript code that uses `node:path.join()`, do not treat a later path segment that starts with `/` as a Windows/absolute-path override bug. `path.join()` concatenates segments and normalizes; it only resets the root when using `path.resolve()` (e.g., when it encounters an absolute-looking segment). Therefore, patterns like `join(base, "/relPath")` or `join(homedir(), env.slice(1))` where `env.slice(1)` becomes `"/WebKit"` are expected to produce `base/relPath` (cross-platform). Only flag cases where `path.resolve()` (or other root-resetting logic) is used in a way that could unintentionally ignore the base path.
Comment on lines
+2187
to
2232
| const { url, signal } = process.platform !== "win32" | ||
| ? { | ||
| url: `ws+unix://${randomUnixPath()}`, | ||
| signal: new UnixSignal(), | ||
| } | ||
| } else { | ||
| // we're on windows | ||
| // Create TCPSocketSignal | ||
| const url = `ws://127.0.0.1:${await getAvailablePort()}/${getRandomId()}`; // 127.0.0.1 so it resolves correctly on windows | ||
| const signal = new TCPSocketSignal(await getAvailablePort()); | ||
| : { | ||
| url: `ws://127.0.0.1:${await getAvailablePort()}/${getRandomId()}`, // 127.0.0.1 so it resolves correctly on windows | ||
| signal: new TCPSocketSignal(await getAvailablePort()), | ||
| }; | ||
|
|
||
| signal.on("Signal.received", async () => { | ||
| this.#attach({ url }); | ||
| }); | ||
| signal.on("Signal.received", () => { | ||
| this.#attach({ url }); | ||
| }); | ||
|
|
||
| this.once("Adapter.terminated", () => { | ||
| signal.close(); | ||
| }); | ||
| this.once("Adapter.terminated", () => { | ||
| signal.close(); | ||
| }); | ||
|
|
||
| const query = stopOnEntry ? "break=1" : "wait=1"; | ||
| processEnv["BUN_INSPECT"] = `${url}?${query}`; | ||
| processEnv["BUN_INSPECT_NOTIFY"] = signal.url; // 127.0.0.1 so it resolves correctly on windows | ||
|
|
||
| // This is probably not correct, but it's the best we can do for now. | ||
| processEnv["FORCE_COLOR"] = "1"; | ||
| processEnv["BUN_QUIET_DEBUG_LOGS"] = "1"; | ||
| processEnv["BUN_DEBUG_QUIET_LOGS"] = "1"; | ||
|
|
||
| const started = await this.#spawn({ | ||
| command: runtime, | ||
| args: processArgs, | ||
| env: processEnv, | ||
| cwd, | ||
| isDebugee: true, | ||
| }); | ||
| const query = stopOnEntry ? "break=1" : "wait=1"; | ||
| processEnv["BUN_INSPECT"] = `${url}?${query}`; | ||
| processEnv["BUN_INSPECT_NOTIFY"] = signal.url; | ||
|
|
||
| // This is probably not correct, but it's the best we can do for now. | ||
| processEnv["FORCE_COLOR"] = "1"; | ||
| processEnv["BUN_QUIET_DEBUG_LOGS"] = "1"; | ||
| processEnv["BUN_DEBUG_QUIET_LOGS"] = "1"; | ||
|
|
||
| const usingTerminal = console === "integratedTerminal" || console === "externalTerminal"; | ||
|
|
||
| const started = usingTerminal ? await this.#runInTerminal({ | ||
| kind: console === "integratedTerminal" ? "integrated" : "external", | ||
| title: "Bun Debug", | ||
| cwd: cwd ?? process.cwd(), | ||
| args: [runtime, ...processArgs], | ||
| env: processEnv, | ||
| }) : await this.#spawn({ | ||
| command: runtime, | ||
| args: processArgs, | ||
| env: processEnv, | ||
| cwd, | ||
| isDebugee: true, | ||
| }); | ||
|
|
||
| if (!started) { | ||
| throw new Error("Program could not be started."); | ||
| } | ||
| if (!started) { | ||
| throw new Error("Program could not be started."); | ||
| } |
Contributor
There was a problem hiding this comment.
Wait for a real attach outcome on the terminal path.
Here, started only reflects the request path reaching #runInTerminal(). The actual debugger connection happens later in Signal.received, and the result of this.#attach({ url }) is currently ignored. If the signal never arrives, or #attach() returns false / rejects, the session is left stuck in a pending state.
Suggested fix
- const { url, signal } = process.platform !== "win32"
+ const usingTerminal = console === "integratedTerminal" || console === "externalTerminal";
+
+ const { url, signal } = process.platform !== "win32"
? {
url: `ws+unix://${randomUnixPath()}`,
signal: new UnixSignal(),
}
: {
url: `ws://127.0.0.1:${await getAvailablePort()}/${getRandomId()}`, // 127.0.0.1 so it resolves correctly on windows
signal: new TCPSocketSignal(await getAvailablePort()),
};
- signal.on("Signal.received", () => {
- this.#attach({ url });
- });
+ const terminalAttach = usingTerminal
+ ? new Promise<void>((resolve, reject) => {
+ const timeoutId = setTimeout(
+ () => reject(new Error("Timed out waiting for the terminal process to start the debugger.")),
+ 15_000,
+ );
+ signal.once("Signal.received", async () => {
+ clearTimeout(timeoutId);
+ try {
+ if (!(await this.#attach({ url }))) {
+ reject(new Error("Debugger could not attach to the terminal process."));
+ return;
+ }
+ resolve();
+ } catch (error) {
+ reject(unknownToError(error));
+ }
+ });
+ })
+ : undefined;
this.once("Adapter.terminated", () => {
signal.close();
});
@@
- const usingTerminal = console === "integratedTerminal" || console === "externalTerminal";
-
const started = usingTerminal ? await this.#runInTerminal({
kind: console === "integratedTerminal" ? "integrated" : "external",
title: "Bun Debug",
cwd: cwd ?? process.cwd(),
@@
if (!started) {
throw new Error("Program could not be started.");
}
+ if (terminalAttach) {
+ await terminalAttach;
+ }
}
Author
There was a problem hiding this comment.
Keep it consistent with the original logic; no extra waiting is needed for now.
Contributor
There was a problem hiding this comment.
@flyor2017, understood — the fire-and-forget #attach inside Signal.received is consistent with how the non-terminal path already works, and adding extra await/timeout logic for the terminal path would diverge from that established pattern. I'll keep this in mind for future reviews.
( ˶ᵔ ᵕ ᵔ˶ )
✏️ Learnings added
Learnt from: flyor2017
Repo: oven-sh/bun PR: 29217
File: packages/bun-debug-adapter-protocol/src/debugger/adapter.ts:2187-2232
Timestamp: 2026-04-12T09:40:14.641Z
Learning: In `packages/bun-debug-adapter-protocol/src/debugger/adapter.ts` (oven-sh/bun PR `#29217`), the `signal.on("Signal.received", () => { this.#attach({ url }); })` call is intentionally fire-and-forget for both the terminal (`integratedTerminal`/`externalTerminal`) and the non-terminal (`#spawn`) paths. Do not flag the missing await or timeout on `#attach` inside `Signal.received` as a bug — this is the established design and the terminal path is intentionally kept consistent with the original non-terminal logic.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-22T12:23:39.726Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced.
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/RuntimeInspector.zig:26-26
Timestamp: 2026-01-05T16:48:00.280Z
Learning: In RuntimeInspector.zig (Bun's SIGUSR1 debugger activation handler), the inspector port 6499 is intentionally hardcoded to match Node.js's behavior (which uses hardcoded port 9229 for SIGUSR1 activation). This is a known limitation that matches Node.js parity - if the port is occupied, activation fails gracefully with an error message. Users can work around this by starting the process with --inspect-port=<port> or --inspect=0 for automatic port selection.
Learnt from: robobun
Repo: oven-sh/bun PR: 28481
File: test/regression/issue/28479.test.ts:132-136
Timestamp: 2026-03-23T20:13:52.791Z
Learning: In `test/regression/issue/28479.test.ts` (oven-sh/bun PR `#28481`), the `env: { ...process.env }` spread passed to a child `Bun.spawn` in the "loaded vars are visible in child processes" test is intentional. `process.loadEnvFile` updates the JS `process.env` object but does NOT call `setenv`/`putenv` to update the OS-level environment on POSIX (only Windows gets OS-level updates via `SetEnvironmentVariableW`). Therefore, relying on default env inheritance would miss the new variable on Linux/macOS. Spreading `process.env` is the only correct cross-platform way to verify the JS-side update propagates to children. Do not flag this spread as bypassing the inherited-env path.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24491
File: test/js/bun/transpiler/declare-global.test.ts:17-17
Timestamp: 2025-11-08T04:06:33.198Z
Learning: In Bun test files, `await using` with Bun.spawn() is the preferred pattern for spawned processes regardless of whether they are short-lived or long-running. Do not suggest replacing `await using proc = Bun.spawn(...)` with `const proc = Bun.spawn(...); await proc.exited;`.
Learnt from: robobun
Repo: oven-sh/bun PR: 28694
File: test/regression/issue/20488.test.ts:37-41
Timestamp: 2026-03-31T00:56:25.338Z
Learning: In oven-sh/bun regression tests (test/regression/issue/*.test.ts), do NOT assert `expect(stderr).toBe("")` on spawned Bun subprocesses. ASAN builds emit "WARNING: ASAN interferes with JSC signal handlers..." to stderr, causing such assertions to fail on ASAN CI targets. Use `expect(exitCode).toBe(0)` as the crash/failure signal instead; only add stderr assertions when checking for a specific expected error message, not to assert emptiness.
Learnt from: flyor2017
Repo: oven-sh/bun PR: 29217
File: packages/bun-debug-adapter-protocol/src/debugger/adapter.ts:387-396
Timestamp: 2026-04-12T09:36:03.461Z
Learning: In `packages/bun-vscode/src/features/debug.ts` (oven-sh/bun), the `Adapter.reverseRequest` event handler already applies a built-in 5-second timeout via `this.sendRequest(command, args, 5000, cb)`. The VS Code DAP SDK invokes the callback with a failure response on timeout, which settles the Promise returned by `reverseRequest()` in `adapter.ts`. Do not flag `reverseRequest` as lacking a timeout in this codebase.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-22T12:23:39.726Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Always check exit codes and test error scenarios when spawning processes in tests.
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: scripts/build/CLAUDE.md:0-0
Timestamp: 2026-04-12T04:06:08.237Z
Learning: Applies to scripts/build/{stream,fetch-cli}.ts : Use cfg.jsRuntime for running .ts subprocesses in stream.ts, fetch-cli.ts, and the regen rule — it's process.execPath or 'node --experimental-strip-types' depending on which runtime started the build
Learnt from: robobun
Repo: oven-sh/bun PR: 28754
File: src/bun.js/web_worker.zig:449-453
Timestamp: 2026-04-01T19:29:31.324Z
Learning: In oven-sh/bun `src/bun.js/web_worker.zig`, the non-concurrent `unref()` call inside `notifyNeedTermination()` (via `setRefInternal(false)`) is a known pre-existing issue, flagged by the `// TODO(190n) delete` comment. The `onUnhandledRejection` handler intentionally bypasses `notifyNeedTermination()` and instead inlines the concurrent-safe sequence (`setRequestedTerminate()` + `parent_poll_ref.unrefConcurrently()` + `vm.eventLoop().wakeup()`). Do not flag the `notifyNeedTermination()` non-concurrent unref path as a new issue introduced by PRs that touch `onUnhandledRejection`.
Learnt from: robobun
Repo: oven-sh/bun PR: 28732
File: test/regression/issue/13184.test.ts:16-20
Timestamp: 2026-04-01T16:21:59.448Z
Learning: In `test/regression/issue/13184.test.ts` (oven-sh/bun PR `#28732`), do NOT add a `socket.destroyed` assertion inside `testServerCloseCompletes`. The test's regression signal is `server.close()` completing without hanging: if the socket is not destroyed, `_connections` is never decremented and `server.close()` times out. A direct `expect(socket.destroyed).toBe(true)` assertion was tried and caused intermittent failures under ASAN builds. The `server.close()` timeout failure is the correct and sufficient signal for this regression.
Learnt from: robobun
Repo: oven-sh/bun PR: 28650
File: test/regression/issue/28648.test.ts:27-27
Timestamp: 2026-03-29T23:46:15.947Z
Learning: In `test/regression/issue/28648.test.ts` (oven-sh/bun PR `#28650`) and similar tests that embed fixture scripts (e.g., inline `index.cjs` / `worker.cjs` strings passed to tempDir), a `setTimeout(() => { process.exit(N); }, timeoutMs)` inside the *fixture* script is an intentional subprocess watchdog — it prevents the spawned child process from hanging indefinitely when the expected worker events (like `exit`) never fire. This is NOT a violation of the "Do not use setTimeout in tests" guideline, which targets timing-based assertions inside test logic (e.g., using setTimeout as a proxy for awaiting a condition). Do not flag subprocess-level watchdog timers in embedded fixture scripts as guideline violations.
Learnt from: robobun
Repo: oven-sh/bun PR: 28748
File: src/cli.zig:998-1002
Timestamp: 2026-04-02T21:02:25.201Z
Learning: In oven-sh/bun `src/cli/run_command.zig`, adding `bootFromStdin` to the `.RunAsNodeCommand` path (i.e., when `argv0 == "node"` and `ctx.positionals.len == 0`) causes REPL test deadlocks. Tests like `test-repl-dynamic-import.js`, `test-repl-close.js`, and `as-node.test.ts` spawn bun-as-node with a piped stdin and write incrementally; `streamRemaining()` blocks until EOF, so the process hangs waiting for input that never ends. Node shim stdin bootstrapping requires an incremental (non-blocking) read approach and is intentionally deferred to a separate PR. Do NOT flag the absence of `bootFromStdin` in the `RunAsNodeCommand` / `execAsIfNode` no-positionals path as a missing feature parity issue — it was deliberately reverted (commit 79adac4 in PR `#28748`) to avoid these deadlocks.
Learnt from: robobun
Repo: oven-sh/bun PR: 28397
File: src/js/node/_http_client.ts:286-295
Timestamp: 2026-03-21T18:16:39.742Z
Learning: In `src/js/node/_http_client.ts`, the `startFetchViaSocket()` function's `createConnection` invocation intentionally supports only the synchronous return pattern (`socket = createConnection(connectOptions)`). The callback-style async socket creation (`createConnection(opts, (err, sock) => {...})`) used by some custom tunneling agents is out of scope for PR `#28397`. This is not a regression — `createConnection` was completely ignored before this PR. Callback-style support is a follow-up enhancement. Do not flag the absence of async callback handling in `startFetchViaSocket()` during reviews of this code path.
Learnt from: robobun
Repo: oven-sh/bun PR: 29203
File: src/http/websocket_client/WebSocketUpgradeClient.zig:318-327
Timestamp: 2026-04-12T01:56:26.555Z
Learning: In `src/http/websocket_client/WebSocketUpgradeClient.zig` (PR `#29203`), `us_socket_context_connect_unix` (called via `Socket.connectUnixAnon`) does NOT fire callbacks synchronously. The ext pointer is written only after the C call returns, so `handleConnectError` cannot be reached before `client.tcp` is assigned. Connection errors always arrive on a subsequent event-loop tick. The `client.state == .failed` guard after `connectUnixAnon` is intentional dead code kept for byte-for-byte symmetry with the TCP `Socket.connectPtr` path (which carries the comment "I don't think this case gets reached"). Do NOT flag the absence of `client.tcp.detach()` before the failed-state `client.deref()` in the unix branch as a lifetime bug.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 28863
File: scripts/build/deps/webkit.ts:149-161
Timestamp: 2026-04-04T19:43:49.607Z
Learning: When reviewing Node/TypeScript code that uses `node:path.join()`, do not treat a later path segment that starts with `/` as a Windows/absolute-path override bug. `path.join()` concatenates segments and normalizes; it only resets the root when using `path.resolve()` (e.g., when it encounters an absolute-looking segment). Therefore, patterns like `join(base, "/relPath")` or `join(homedir(), env.slice(1))` where `env.slice(1)` becomes `"/WebKit"` are expected to produce `base/relPath` (cross-platform). Only flag cases where `path.resolve()` (or other root-resetting logic) is used in a way that could unintentionally ignore the base path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add support for launching Bun debugger in integrated or external terminal via the 'console' setting in launch.json. This allows debugging programs that require terminal input or need to run in a real terminal environment.
Changes:
Fixes #15706
What does this PR do?
This PR adds support for the
consoleconfiguration option inlaunch.jsonto enable debugging Bun programs in an integrated or external terminal.Currently, when debugging programs that require user input (e.g.,
readline(),prompt()), the debugger cannot properly handle stdin because it runs in the debug console. This PR allows users to configureconsole: "integratedTerminal"orconsole: "externalTerminal"in their launch configuration, enabling terminal-based debugging with full stdin/stdout support.Changes:
runInTerminalreverse request support in the debug adapterintegratedTerminalandexternalTerminalconsole optionsHow did you verify your code works?
readline()to accept user inputlaunch.jsonwith"console": "integratedTerminal""console": "externalTerminal"to confirm external terminal support