fix(repl): tab completion now resolves the target object from dot expressions#27519
fix(repl): tab completion now resolves the target object from dot expressions#27519
Conversation
…ressions
Previously, REPL tab completion always completed against `globalThis`,
even for dot expressions like `{}.to[TAB]` or `[1,2,3].pus[TAB]`. This
was because the target expression before the dot was never evaluated.
Now, when a dot is detected before the word being completed, the
expression before the dot is evaluated and used as the completion target.
Also removes a redundant prototype chain walk in `getCompletions` since
`getPropertyNames` with `DontEnumPropertiesMode::Include` already
traverses the full prototype chain.
Closes #27518
Co-Authored-By: Claude <noreply@anthropic.com>
|
Updated 5:54 AM PT - Feb 28th, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 27519That installs a local version of the PR into your bun-27519 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a C binding Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://cdn.statically.io/img/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 the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/27518.test.ts`:
- Around line 110-122: The test destructures an unused allOutput from the
withTerminalRepl callback; in the test "array dot-completion does not show
global properties" remove allOutput from the parameter list (i.e., change async
({ send, waitFor, allOutput }) => to async ({ send, waitFor }) =>) so only used
helpers remain; update the callback signature in this test (inside
withTerminalRepl) and ensure no other references to allOutput exist in the test
body.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/headers.hsrc/repl.zigtest/regression/issue/27518.test.ts
Co-Authored-By: Claude <noreply@anthropic.com>
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 `@test/regression/issue/27518.test.ts`:
- Around line 76-79: The current cleanup masks REPL-exit regressions by racing
proc.exited with a fixed Bun.sleep and silently killing the process; replace
that pattern so the test explicitly waits for the child to exit within a short
timeout, fails if it doesn't, and asserts the exit code. Specifically, remove
the Promise.race([proc.exited, Bun.sleep(2000)]) usage and instead await
proc.exited with a timeout wrapper (use the same test harness timeout helper or
create one), after which assert proc.exitCode (or proc.status) equals the
expected value (e.g., 0); if the timeout expires and the process is still
running (proc.killed is false), call proc.kill but also cause the test to fail
with a clear message indicating the REPL did not exit. Use the identifiers send,
proc.exited, Bun.sleep only for timeout logic and proc.kill/proc.killed and
proc.exitCode in your assertions.
- Around line 46-67: The waitFor function can hang because it awaits a waiter
promise that only resolves on new data, so if no data comes it never re-checks
the deadline; modify the loop in waitFor to race the resolveWaiter promise
against a timeout: create the waiter Promise that assigns resolveWaiter, create
a timeout Promise (using setTimeout) that rejects (or resolves) after the
computed remaining ms, then await Promise.race([waiterPromise, timeoutPromise]);
ensure you clear the timer and null out resolveWaiter in both paths so
resolveWaiter is not leaked; reference the waitFor function, the resolveWaiter
variable, deadline/remaining computation, and received/cursor handling.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/regression/issue/27518.test.ts
Race the waiter promise against a setTimeout so the deadline check fires even when no new terminal data arrives. Simplify cleanup to force-kill without asserting exit code since PTY-mode REPL may not terminate within the grace period. Co-Authored-By: Claude <noreply@anthropic.com>
![claude[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — standard REPL completion pattern, clean implementation with good test coverage.
Extended reasoning...
Overview
This PR fixes REPL tab completion to evaluate the expression before a dot (e.g., {}.to[TAB]) to determine the correct completion target, rather than always completing against globalThis. It adds a new C++ function Bun__REPL__evaluateCompletionTarget that silently evaluates expressions without modifying _ or _error, updates the Zig tab completion logic to use it, removes a duplicate prototype chain walk in getCompletions (since getPropertyNames with DontEnumPropertiesMode::Include already traverses prototypes), and adds a regression test with 4 cases.
Security risks
The new evaluateCompletionTarget function evaluates arbitrary JavaScript during tab completion, which could trigger side effects. However, this is the standard approach used by Node.js REPL and other REPLs — REPL users already have full code execution capability, so this introduces no new attack surface. The function properly clears exceptions on failure and returns undefined.
Level of scrutiny
Medium scrutiny. The changes touch production C++ and Zig code in the REPL subsystem, but the REPL is an interactive developer tool (not a server-side code path). The pattern of evaluating dot-expression targets for completion is well-established across JavaScript REPLs. The C++ code correctly uses exception scoping and the Zig code handles allocation failures gracefully.
Other factors
- The CI failures shown in the timeline are all pre-existing
vendor/tinycc/tcc.hclang warnings andCMakeLists.txtcmake warnings, unrelated to this PR. - CodeRabbit found only trivial nits (unused destructuring, waitFor timeout race), both already addressed in follow-up commits.
- The bug hunting system found no issues.
- The test file is correctly placed in
test/regression/issue/27518.test.tsper CLAUDE.md conventions. - The parenthesization of expressions before evaluation (wrapping
{}as({})) is a standard technique to disambiguate object literals from block statements.
Address review feedback: the cleanup now sends Ctrl+C to clear pending input before .exit, explicitly asserts the exit code is 0, and fails the test with a clear message if the REPL doesn't exit in time. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/27518.test.ts`:
- Around line 65-75: Replace the ad-hoc setTimeout-based race with Bun.sleep by
removing the timer variable and the wrapper promise and instead using
Bun.sleep(remaining) in the Promise.race; keep the existing resolveWaiter
assignment and post-race cleanup (set resolveWaiter = null) and remove
clearTimeout(timer) since Bun.sleep needs no explicit cancellation. Locate the
block that declares timer and calls setTimeout, and update the Promise.race to
use Bun.sleep(remaining) so the test uses Bun's deadline-based sleep
consistently with the other tests.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/regression/issue/27518.test.ts
…d clear stale exceptions Two bugs fixed based on review feedback: 1. Expression extraction now scans backwards from the dot to find only the dot-expression chain (identifiers, dots, balanced brackets and parens) instead of taking the entire line prefix. This prevents side effects from evaluating assignments like `x = obj.ba<TAB>` and avoids syntax errors from `let x = foo.ba<TAB>`. 2. Added global.clearException() before the early return when getCompletions returns undefined, preventing stale exceptions from leaking into subsequent evaluations (e.g. from a Proxy with a throwing ownKeys trap). Also adds a regression test verifying tab completion doesn't cause side effects when used after an assignment operator. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/regression/issue/27518.test.ts (1)
65-75: 🛠️ Refactor suggestion | 🟠 MajorReplace
setTimeoutwithBun.sleep.The timeout mechanism uses
setTimeoutwhich violates the coding guideline for test files.Bun.sleep(remaining)can be used directly inPromise.racewithout needing wrapper promises or explicit cleanup.🔧 Suggested fix
- let timer: ReturnType<typeof setTimeout> | undefined; await Promise.race([ new Promise<void>(resolve => { resolveWaiter = resolve; }), - new Promise<void>(resolve => { - timer = setTimeout(resolve, remaining); - }), + Bun.sleep(remaining), ]); - clearTimeout(timer); resolveWaiter = null;As per coding guidelines: "Do not use
setTimeoutin tests; insteadawaitthe condition to be met".
,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/regression/issue/27518.test.ts` around lines 65 - 75, The test uses setTimeout with timer, clearTimeout and resolveWaiter to race a wait condition; replace that with Bun.sleep to follow test guidelines: use Promise.race([new Promise(resolve => { resolveWaiter = resolve; }), Bun.sleep(remaining)]) and remove the timer variable, clearTimeout call, and explicit timer cleanup so resolveWaiter and remaining are the only symbols used for the race; ensure you null out resolveWaiter after the race as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/regression/issue/27518.test.ts`:
- Around line 65-75: The test uses setTimeout with timer, clearTimeout and
resolveWaiter to race a wait condition; replace that with Bun.sleep to follow
test guidelines: use Promise.race([new Promise(resolve => { resolveWaiter =
resolve; }), Bun.sleep(remaining)]) and remove the timer variable, clearTimeout
call, and explicit timer cleanup so resolveWaiter and remaining are the only
symbols used for the race; ensure you null out resolveWaiter after the race as
before.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/repl.zigtest/regression/issue/27518.test.ts
Two CI failures fixed: 1. ASAN: Changed getCompletions from DECLARE_THROW_SCOPE to DECLARE_TOP_EXCEPTION_SCOPE and explicitly clear exceptions on all error paths. The throw scope was causing assertion failures because the Zig caller doesn't have a JSC exception scope, so JSC's exception check verification would fire when getLength was called on the result. 2. macOS: The "variable dot-completion" test was matching on the terminal echo of the definition input rather than the completion output. Fixed by waiting for the REPL evaluation output (which contains the property values) before sending the tab completion, ensuring the cursor advances past all echoed characters. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the ad-hoc setTimeout-based race with Bun.sleep(remaining) for consistency with the rest of the test patterns. Bun.sleep needs no explicit cancellation so the timer variable and clearTimeout call are removed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The waitFor helper was setting cursor = all.length which consumed ALL received data on match, not just data up to the match point. When the evaluation output and prompt arrived in the same chunk (common on fast platforms like Alpine/Debian aarch64), the subsequent waitFor for the prompt would find nothing because the cursor had already skipped past it. Fix: advance cursor only to the end of the matched text. This also fixes the ASAN failure where waitFor(/xyz(One|Two)/) matched on xyzOne before xyzTwo arrived — now we wait for "xyzTwo" specifically to ensure both completions are in the output. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
LGTM — all previously identified bugs were addressed, including the expression extraction fix and the clearException fix.
Extended reasoning...
Overview
This PR fixes REPL tab completion to resolve the target object from dot expressions rather than always completing against globalThis. It touches 4 files: bindings.cpp (new evaluateCompletionTarget function, refactored getCompletions), headers.h (declaration), repl.zig (new findExpressionStart helper, modified handleTab), and a new regression test file.
Previous Review
In my prior review I raised two bugs:
- [Red/Critical] Expression extraction using
line[0 .. word_start - 1]caused side effects (e.g.,x = obj.ba[TAB]would evaluate the assignment). This was fixed by addingfindExpressionStartwhich scans backwards from the dot to extract only the dot-expression chain. - [Yellow/Medium] Missing
global.clearException()before the early return whencompletions.isUndefined(). This was fixed both in Zig (addedglobal.clearException()) and C++ (switched fromDECLARE_THROW_SCOPEtoDECLARE_TOP_EXCEPTION_SCOPEwith explicitscope.clearException()calls).
Both fixes are verified in the current diff, and a new test case (dot-completion after assignment does not cause side effects) explicitly validates the side-effect fix.
Security Risks
The evaluateCompletionTarget function evaluates arbitrary JS expressions as a side effect of pressing Tab. This is by design (matching Chrome DevTools behavior) and is scoped to the extracted dot-expression chain only. The findExpressionStart parser ensures only the minimal expression is evaluated. No new injection vectors or auth concerns.
Level of Scrutiny
Medium scrutiny. REPL tab completion is a developer-facing feature, not a core runtime path. The worst case for edge cases in findExpressionStart (e.g., strings inside brackets) is falling back to globalThis completions, not a crash or data loss.
Other Factors
All CodeRabbit issues (unused destructuring, waitFor deadlock, exit code assertions, setTimeout→Bun.sleep) were also addressed. The test file follows CLAUDE.md conventions (regression test path, bunExe/bunEnv, await using, exit code assertions). No outstanding reviewer comments remain.
1 participant
Summary
{}.to[TAB]completes againstObject.prototype, notglobalThis)getPropertyNameswithDontEnumPropertiesMode::Includealready walks the full prototype chain, so the manual prototype chain walk was producing duplicate completionsBun__REPL__evaluateCompletionTargetwhich evaluates expressions silently without modifying_erroror_Test plan
test/regression/issue/27518.test.tswith 4 test cases:test/js/bun/repl/repl.test.ts)USE_SYSTEM_BUN=1)Closes #27518
🤖 Generated with Claude Code