Skip to content

wtf-bindings: use WTF ASSERT() so debug builds compile #30992

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 4 commits into
base: main
Choose a base branch
from
Open

wtf-bindings: use WTF ASSERT() so debug builds compile #30992

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2aa7a4b
wtf-bindings: use WTF ASSERT() in uv__tty_make_raw
robobun May 18, 2026
4262bbc
terminal.test: pick ICANON per-platform (0x100 on darwin)
robobun May 18, 2026
77974b9
terminal.test: drop stray BSD from ICANON comment
robobun May 18, 2026
4911963
ci: retrigger (darwin-14-aarch64 expired in queue)
robobun May 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/jsc/bindings/wtf-bindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ extern "C" int uv_tty_reset_mode(void)

static void uv__tty_make_raw(struct termios* tio)
{
assert(tio != NULL);
ASSERT(tio != NULL);

#if defined __sun || defined __MVS__
/*
Expand Down
29 changes: 29 additions & 0 deletions test/js/bun/terminal/terminal.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,35 @@

expect(() => terminal.setRawMode(true)).toThrow("Terminal is closed");
});

// Regression for #30988: wtf-bindings.cpp used libc `assert` without
// including <cassert>. After the WebKit upgrade in #30705 the header
// stopped reaching this translation unit transitively, so debug builds
// failed to compile. Building and running this test file exercises
// Bun__ttySetMode (the same translation unit) on a real PTY fd and
// proves the file compiled.
test.skipIf(isWindows)("setRawMode toggles on a real PTY fd", async () => {
await using terminal = new Bun.Terminal({});
// ICANON is 0x2 on Linux/BSD but 0x100 on Darwin (bit 0x2 is ECHOE there,
// which Bun__ttySetMode mode=1 does not clear, so a flat 0x2 would fail
// on macOS CI). ECHO is 0x8 on every POSIX we build for.

Check warning on line 292 in test/js/bun/terminal/terminal.test.ts

View check run for this annotation

Claude / Claude Code Review

Comment groups BSD on wrong side of ICANON value 

Tiny nit: the comment says "ICANON is 0x2 on Linux/BSD but 0x100 on Darwin", but BSD is on the wrong side — FreeBSD/NetBSD/OpenBSD all use `0x100` (Darwin inherited BSD termios), only Linux uses `0x2`. The review comment this responds to even said "on macOS/BSD, ICANON is 0x100". No functional impact since Bun only targets linux/darwin/windows; consider just dropping "/BSD" or moving it to the Darwin side.
Comment thread
robobun marked this conversation as resolved.
Outdated
const ICANON = process.platform === "darwin" ? 0x100 : 0x2;
const ECHO = 0x8;

const beforeLflag = terminal.localFlags;
expect(beforeLflag & ICANON).not.toBe(0);
expect(beforeLflag & ECHO).not.toBe(0);

terminal.setRawMode(true);
const rawLflag = terminal.localFlags;
expect(rawLflag & ICANON).toBe(0);
expect(rawLflag & ECHO).toBe(0);

terminal.setRawMode(false);
const restoredLflag = terminal.localFlags;
expect(restoredLflag & ICANON).not.toBe(0);
expect(restoredLflag & ECHO).not.toBe(0);
});
Comment on lines +296 to +309
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check default ECHO flag state in Bun.Terminal on POSIX

bun -e '
const t = new Bun.Terminal({});
const ECHO = 0x8;
const echoEnabled = (t.localFlags & ECHO) !== 0;
console.log("ECHO enabled by default:", echoEnabled);
t.close();
'

Repository: oven-sh/bun

Length of output: 99

🏁 Script executed:

cd /tmp && wc -l test/js/bun/terminal/terminal.test.ts 2>/dev/null || find . -path "*test/js/bun/terminal*" -name "*.ts" 2>/dev/null

Repository: oven-sh/bun

Length of output: 37

🏁 Script executed:

head -20 test/js/bun/terminal/terminal.test.ts

Repository: oven-sh/bun

Length of output: 769

🏁 Script executed:

# Check the enableEcho helper and surrounding context (lines 1-10)
sed -n '1,20p' test/js/bun/terminal/terminal.test.ts

Repository: oven-sh/bun

Length of output: 769

🏁 Script executed:

# Read the entire test file to verify the claims
cat test/js/bun/terminal/terminal.test.ts

Repository: oven-sh/bun

Length of output: 35985

🏁 Script executed:

# Let's search for the enableEcho function and its usages
rg -n "enableEcho" test/js/bun/terminal/terminal.test.ts -A 2 -B 2

Repository: oven-sh/bun

Length of output: 1367

🏁 Script executed:

# Also check for ECHO and ICANON constants
rg -n "const ECHO|const ICANON" test/js/bun/terminal/terminal.test.ts

Repository: oven-sh/bun

Length of output: 198

🏁 Script executed:

# Check lines 290-315 for the disputed test
sed -n '290,315p' test/js/bun/terminal/terminal.test.ts

Repository: oven-sh/bun

Length of output: 999

🏁 Script executed:

# Read specific line ranges
head -30 test/js/bun/terminal/terminal.test.ts

Repository: oven-sh/bun

Length of output: 992

🏁 Script executed:

cat -n test/js/bun/terminal/terminal.test.ts | sed -n '1,50p'

Repository: oven-sh/bun

Length of output: 1766

🏁 Script executed:

python3 << 'EOF'
import os
filepath = 'test/js/bun/terminal/terminal.test.ts'
if os.path.exists(filepath):
    with open(filepath, 'r') as f:
        lines = f.readlines()
        print(f"Total lines: {len(lines)}")
        # Show lines 1-20
        print("\n=== Lines 1-20 ===")
        for i in range(min(20, len(lines))):
            print(f"{i+1:3d}: {lines[i]}", end='')
else:
    print(f"File not found: {filepath}")
EOF

Repository: oven-sh/bun

Length of output: 907

🏁 Script executed:

git ls-files | grep terminal | grep test

Repository: oven-sh/bun

Length of output: 189

🏁 Script executed:

ls -la test/js/bun/ 2>&1 | head -20

Repository: oven-sh/bun

Length of output: 1221

🏁 Script executed:

# Check if the directory exists
ls -d test/js/bun/terminal 2>&1 || echo "Directory not found"
# List all test files
ls test/js/bun/*.test.ts 2>&1 | head

Repository: oven-sh/bun

Length of output: 350

🏁 Script executed:

# Search for the test mentioned in the review
rg "setRawMode" . --type ts --type tsx -l

Repository: oven-sh/bun

Length of output: 83

🏁 Script executed:

# Find all test files with "terminal" in the name
find . -name "*terminal*" 2>/dev/null | grep -E "\.(ts|tsx|js)$"

Repository: oven-sh/bun

Length of output: 195

🏁 Script executed:

# Also search for ECHO constant definitions
rg "const ECHO" --type ts --type tsx

Repository: oven-sh/bun

Length of output: 83

🏁 Script executed:

# Check if there's a Bun.Terminal test file
rg "Bun.Terminal" test --type ts --type tsx -l

Repository: oven-sh/bun

Length of output: 83

🏁 Script executed:

# Try to find any reference to the Terminal class or test
rg "class.*Terminal|function.*Terminal|new.*Terminal" test --type ts --type tsx -l | head

Repository: oven-sh/bun

Length of output: 83

🏁 Script executed:

# Search for any Terminal-related test
fd -e test.ts -e test.tsx -e test.js test/ | xargs grep -l "Terminal" 2>/dev/null | head

Repository: oven-sh/bun

Length of output: 381

Verify ECHO flag initial state—contradicts existing helper documentation.

Lines 298 and 308 assert that the ECHO flag is enabled in the default terminal state and after disabling raw mode:

expect(beforeLflag & ECHO).not.toBe(0);  // expects ECHO enabled initially
...
expect(restoredLflag & ECHO).not.toBe(0); // expects ECHO enabled after restore

However, the enableEcho helper (line 4) documents that "echo is disabled by default to avoid duplication", and all existing tests explicitly call enableEcho(terminal) before exercising echo behavior (e.g., lines 483, 507, 532, 553, 571, 776, 829, 945), confirming that Bun.Terminal initializes with ECHO off.

The test should instead expect:

expect(beforeLflag & ECHO).toBe(0);      // ECHO disabled initially
expect(rawLflag & ECHO).toBe(0);         // ECHO still disabled in raw mode
expect(restoredLflag & ECHO).toBe(0);    // ECHO remains disabled after restore

While the test's primary goal (proving wtf-bindings.cpp compiled by exercising Bun__ttySetMode on a real PTY fd) is sound, these incorrect assertions will cause spurious test failures.

🤖 Prompt for 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.

In `@test/js/bun/terminal/terminal.test.ts` around lines 296 - 309, The assertions
about the ECHO bit are incorrect given that Bun.Terminal initializes with echo
off; update the expectations in the test around terminal.localFlags so they
assert ECHO is cleared (toBe(0)) for beforeLflag, rawLflag, and restoredLflag
instead of asserting it is set, leaving the ICANON checks unchanged; locate the
assertions near the calls to terminal.setRawMode(true)/setRawMode(false) and
adjust the three expect(... & ECHO) lines accordingly (references:
terminal.localFlags, terminal.setRawMode, constants ICANON and ECHO, helper
enableEcho).

});

describe("termios flags", () => {
Expand Down
Loading