fix(tui): restore Shift+Enter in Warp on WSL - #1109
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d50a219b08
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
d50a219 to
b3d8e0d
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3d8e0def1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
b3d8e0d to
172bdf9
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Warp's direct WSL terminal path emits Shift+Enter as bare LF, which legacy matching also accepts as Enter. Normalize that event to explicit CSI-u Shift+Enter before editor dispatch. Constraint: Preserve CR submit, bracketed paste payloads, other terminals, and SSH/tmux sessions Rejected: Reorder submit and newline matching globally | breaks terminals that emit LF for plain Enter Tested: package TUI suite; npm run check; live Warp on WSL user verification
172bdf9 to
90892c5
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
code-yeongyu
left a comment
code-yeongyu
left a comment
There was a problem hiding this comment.
B1. The PR is not safe to merge as written. The rewrite is applied in the shared ProcessTerminal forwarding path, so it affects every focused TUI component, not only the multiline editor. In Warp+WSL, raw LF previously matched the legacy Enter behavior used by single-line Input and SelectList consumers (extension prompts/dialogs, search fields, selectors); after this rewrite those consumers receive ESC[13;2u (shift+enter), while they only handle enter/raw LF and then drop the CSI-u control sequence. This regresses confirmation/submission outside the target composer. Route the workaround to the multiline editor or preserve the original LF for components that do not support Shift+Enter, and add a component-level regression test.
B2. The environment heuristic is unsafe: !== undefined treats empty and spoofed markers as proof of WSL/Warp. A launcher or wrapper exporting TERM_PROGRAM=WarpTerminal plus WSL_INTEROP=/WSL_DISTRO_NAME= (or an empty WARP marker) causes every standalone LF to be rewritten. LF is a valid legacy Enter encoding, so this false positive can silently prevent submission. Require non-empty/validated markers plus a real WSL/platform check (or an explicit opt-in), and cover empty/spoofed values in the matrix.
B3. GitHub currently reports this PR as CONFLICTING with main (mergeStateStatus: DIRTY). The conflict zones documented in packages/tui/src/changes.md do not resolve the actual conflicts: a clean merge must be produced and re-reviewed before this can land.
B4. The required branch-protection contexts Check and test and Changelog gate are not present for the current head. gh pr checks shows only a neutral Cubic review and a passing GitGuardian check. The PR body's local commands and live verification do not satisfy the required GitHub gates; after resolving the merge conflict, push a current head and wait for both required contexts to pass.
The targeted behavior may be correct for the one editor scenario, but the implementation's shared scope and unvalidated environment assumptions make the advertised "other terminals"/"other TUI consumers" safety claims untrue.
|
Addressed review 5058005408 in 0e6fd44: Warp/WSL LF normalization now occurs only inside the multiline Editor, so ProcessTerminal forwards raw LF to single-line Input/selectors; detection requires Linux plus non-empty validated WSL markers and excludes SSH/multiplexer sessions; added regression coverage for raw forwarding, empty/non-Linux/spoofed markers, and editor behavior. Merged origin/main and resolved conflict markers while retaining both change histories. TUI build and full packages/tui npm test pass (all tests green). Pushed to fix/warp-wsl-newline; GitHub head/mergeability API may take a moment to refresh. |
code-yeongyu
left a comment
code-yeongyu
left a comment
There was a problem hiding this comment.
Round-2 adversarial review of commit 0e6fd4463e10989d4dccde7476caa8f7207fbe2e.
Round-1 blocker verdicts
- B1 - behavior scope: VERIFIED, coverage incomplete.
ProcessTerminal.forwardInputSequence()now forwards raw LF, and normalization is at the baseEditor.handleInput()boundary. That covers the built-in composer (CustomEditor), the extension multi-line editor, and other built-inEditorinstances; single-lineInput/selector consumers no longer receive the rewritten CSI-u sequence. The missing real-editor regression assertion is recorded as B7 below. - B2 - detection hardening: NOT FULLY RESOLVED. Linux and whitespace/SSH/multiplexer gates are present, but the exact predicate still accepts an absent/empty Warp session ID via
TERM_PROGRAM === "WarpTerminal", and treats any string beginning/run/WSL/as valid without validating the WSL<pid>_interopshape or existence. See B5. - B3 - main conflict: VERIFIED RESOLVED. The worktree is clean,
git merge-base HEAD origin/mainisf97db4007, the conflict marker removed frompackages/coding-agent/src/modes/interactive/changes.md, and GitHub reportsMERGEABLE; the current parent includes the main-side changes. - B4 - required checks: NOT RESOLVED. Branch protection still requires
Check and testandChangelog gate, whilegh pr checks 1109reports only GitGuardian Security Checks and cubic. Local test/build claims do not satisfy these required GitHub contexts. See B6.
Remaining blockers
B5. The WSL/Warp identity predicate still has a spoofable false-positive path. With platform = "linux", { TERM_PROGRAM: "WarpTerminal", WSL_INTEROP: "/run/WSL/not-a-real-socket" } returns ESC[13;2u despite having no non-empty Warp session ID and no valid interop endpoint. { TERM_PROGRAM: "WarpTerminal", WSL_DISTRO_NAME: "spoofed", WSL_INTEROP: "spoofed" } also passes because both markers are only checked for non-emptiness. This can rewrite legacy LF submission on a Linux process with inherited or manually spoofed variables. Require the non-empty Warp ID and validate the interop marker/path shape (and cover both-marker spoofing); do not rely on startsWith("/run/WSL/") as validation.**
B6. The required branch-protection contexts are still absent for the reviewed head. gh pr checks 1109 --repo code-yeongyu/senpi shows no Check and test and no Changelog gate, although gh api .../branches/main/protection confirms both are required. Please push/wait for the current head's required contexts before merge.**
B7. The new tests do not exercise the actual Editor success path. The PR diff adds pure normalizer tests and a ProcessTerminal test with an arbitrary input callback, but no editor.test.ts regression. The existing editor tests cover an already-explicit CSI-u sequence and run independently of the new environment predicate; they would remain green if the new Editor.handleInput() normalization line were removed. Add a platform/env-injected test that drives the real Editor and asserts Warp/WSL LF inserts a newline without submit, while CR still submits.**
Validation performed
packages/tui/test/terminal.test.ts: 38 passed.packages/tui/test/editor.test.ts: 200 passed.- LSP diagnostics for all changed TUI source/test files: clean.
- Direct predicate probe reproduced the B5 cases above.
Because B2/B4 remain unresolved and the B1 regression coverage is incomplete, this PR is not approvable yet.
|
Round-2 blocker fixes pushed in
Local verification: focused failing-first tests caught the removed TERM_PROGRAM-only positive assumption; focused B5/B7 tests then passed. Full |
code-yeongyu
left a comment
code-yeongyu
left a comment
There was a problem hiding this comment.
Round-3 adversarial review of commit 0d3329c9e24fc0f407675672b81954dbdd7b6eae.
Round-2 blocker resolution
- B5 - VERIFIED.
normalizeWarpWslShiftEnterInput()now requires Linux, a non-empty trimmedWARP_SESSION_IDorWARP_TERMINAL_SESSION_UUID, an exact/run/WSL/<numeric>_interoppath, and a positive existence result;TERM_PROGRAMalone is ignored, and SSH/multiplexer exclusions remain. Direct probes returned raw LF for empty IDs, whitespace-only IDs, TERM_PROGRAM-only, non-numeric interop, wrong prefixes, and a nonexistent socket. - B6 - VERIFIED. The required
Check and testandChangelog gatecontexts are present for head0d3329c9e24fc0f407675672b81954dbdd7b6eae, andgh pr checks 1109reports both passing. All other CI checks also pass. - B7 - VERIFIED. The new tests drive
Editor.handleInput()with injected Linux/environment/socket seams. I copied the test tree, removed only the normalization call, and the Warp-on-WSL LF test failed ('' !== 'hello\n'); with the call present, the focused Editor/terminal run passed 241/241. CR submission and non-Warp LF submission are covered and pass.
Fresh blocker
B8. existsSync does not verify that WSL_INTEROP is a socket.
At packages/tui/src/terminal.ts:89, the production default is fs.existsSync. That API follows symlinks and returns true for regular files and directories as well as Unix sockets. Therefore a syntactically valid /run/WSL/123_interop path that is a symlink to (or is replaced by) a regular file or directory still activates the LF rewrite. This is a remaining false-positive path for the exact predicate this PR is meant to harden. The check should validate the target type (for example fs.statSync(path).isSocket(), which still permits WSL's legitimate symlink aliases) and add a regression for a non-socket target.
Other requested edge cases
- WSL1: the predicate safely leaves LF unchanged when
/run/WSL/WSL_INTEROPis absent. I inspected the WSL source/docs path: the generic interop server is a Unix socket under/run/WSL, and the WSL1 path usesLxInitMessageCreateProcess; absence of that active marker provides no reliable target to normalize against. This is fail-safe rather than a new false-positive. - Hot path and environment changes: non-LF input returns before any filesystem call; the stat is performed only for a standalone LF after the Warp/path gates. There is no cached predicate, and production callers leave the optional seams undefined, so default calls read live
process.env/process.platform; mutating an injected environment between calls changed the result in my probe. - The Editor seam is behavior-neutral by default: it is optional, is not supplied by production call sites, and the mutation test proves it only controls the testable normalizer inputs.
The focused local Editor/terminal tests, TUI build, diff check, and all current CI checks are green, but B8 remains before approval.
|
Round-3 B8 fixed in commit
Gates: TUI build passed; package-wide LSP error scan passed with 0 errors; Pushed to |
code-yeongyu
left a comment
code-yeongyu
left a comment
There was a problem hiding this comment.
Round-4 adversarial review of commit e9b9e4f47b0528eff1e6e7cec7da3cf9be2ef520.
B8 - VERIFIED
The production default now wraps fs.statSync(socketPath).isSocket() in a try/catch, so it follows symlinks for legitimate aliases, accepts only socket targets, and fails safe when stat throws. I probed the production default by redirecting only the canonical WSL path to real temporary filesystem entries: regular file, directory, symlink to regular file, and dangling symlink all preserved raw LF; a real Unix socket and symlink to a real Unix socket both produced ESC[13;2u; an EACCES stat also preserved raw LF. The injected seam receives the canonical path and Editor forwards its optional seam; production callers leave all seams undefined.
The Linux gate makes the Windows named-pipe case unreachable, and there is no meaningful TOCTOU use after the type check for this feature's threat model. Non-LF input returns before any filesystem call. The focused terminal/editor tests passed, the full @earendil-works/pi-tui test command passed, the TUI build passed, source/test diagnostics had no errors, and git diff --check passed.
Fresh blocker
B9. The synchronous socket stat is not gated behind the Warp check.
In packages/tui/src/terminal.ts, isWsl is computed as interopPath !== undefined && validPathRegex.test(interopPath) && socketExists(interopPath), while isWarp is checked only in the final return expression. Consequently, on Linux in an ordinary non-Warp WSL session with a valid WSL_INTEROP path, every standalone LF still invokes the synchronous production fs.statSync() even though the function always returns the original LF. My seam probe with { WSL_INTEROP: "/run/WSL/321_interop" } and no Warp marker recorded one socket-check call; malformed/missing WSL paths and non-LF input did not call it.
Please short-circuit before the filesystem check when no non-empty Warp marker is present (or include isWarp && before socketExists). This keeps the synchronous stat limited to the intended Warp-on-WSL path rather than adding it to every LF handled by Editors in normal WSL sessions.
B8 is otherwise resolved; B9 is the only new blocker found.
|
Round-4 B9 fixed in commit
Verification:
Pushed to |
code-yeongyu
left a comment
code-yeongyu
left a comment
There was a problem hiding this comment.
Round-5 adversarial review of commit f06a0533dd579f836b33ae38c1a84c48cff9ae46.
B9 - VERIFIED RESOLVED
The new isWarp && operand in the isWsl conjunction is a real short-circuit gate before socketExists(interopPath). I verified the required cases with direct probes against normalizeWarpWslShiftEnterInput(): (a) Linux with valid /run/WSL/321_interop and no Warp marker preserved raw LF and made zero socket-check calls; (b) Linux with a valid Warp marker and valid interop path called the seam exactly once and returned ESC[13;2u (both Warp marker variants reached the check); (c) a Warp marker with missing, blank, or malformed interop preserved raw LF and made zero socket-check calls. A Proxy-based probe also confirmed the Warp environment fields are evaluated before the socket seam, so there is no eager socket evaluation.
The Warp-on-WSL happy path remains active: the valid marker/path reaches the socket check and rewrites when it reports a socket. The predicate is not memoized; changing the injected environment between calls changed the result and only the newly qualifying call invoked the socket check. The isWarp probe itself is limited to trimmed environment reads and does not introduce another filesystem operation.
The focused TUI terminal/editor test run passed, the independent B9 probe passed, diagnostics for all changed TUI source/test files reported no errors, git diff --check passed, and the worktree remained clean. I found no new defensible blocker.
VERDICT: APPROVE
2 participants
Summary
Root cause
Warp's terminal path sends Shift+Enter as bare LF (
0x0a). In Senpi's legacy keyboard path, LF must also remain recognizable as Enter because some terminals emit LF for plain Enter.The editor checks submit before newline, so Warp's Shift+Enter LF followed the Enter binding and submitted the composer instead of inserting a line break. WSL also reports
process.platform === "linux", so Senpi's native Windows modifier helper cannot recover the Shift state from the host console.Fix
ProcessTerminalnow rewrites a standalone LF to the existing CSI-u Shift+Enter sequence (ESC [ 13 ; 2 u) only when both Warp and WSL are detected in a direct local session.Evidence
v0.2026.07.22.09.01.stable_01on WSL verification confirmed Shift+Enter inserts a newline and Enter still submits normallyValidation
cd packages/tui && npm testnpm run checknpm run buildnode scripts/check-pr-changelog.mjs --base upstream/mainScope
The change is limited to
ProcessTerminal, shared multiplexer detection, focused terminal regression coverage, the TUI changelog, and the nearest fork-change tracker. No public API, migration, keybinding, or editor-dispatch changes.Related