Native shell completions: drive all shells through in-band generators - #15294
Draft
warp-agent-staging[bot] wants to merge 48 commits into
Draft
Conversation
Replaces the zsh-only, key-triggered OSC 9280 round trip with a single generator-shaped client path across zsh, bash, fish, and PowerShell. - zsh: a new foreground generator (select can't run through the backgrounded warp_run_generator_command) chains the user's zle-line-init/zle-line-finish, arms a guard flag, and reuses the existing compadd shim's OSC 9280 emission. Also fixes CORE-3795 (the shim's -d flag lookup missed _describe's clustered -ld). - bash: complete -p plus bash-completion's lazy loader, synthesizes COMP_WORDS/COMP_CWORD/COMP_LINE/COMP_POINT, calls the -F function directly, emits COMPREPLY (names only). - fish: complete -C "<line>", already used elsewhere in the bootstrap for executable discovery. - PowerShell: [System.Management.Automation.CommandCompletion]::CompleteInput. Removes the client-side trigger state machine (NativeShellCompletionsState::AwaitingPrompt, SendCompletionsPrompt, the write-blocking clause, and the Ctrl-Y write) and reuses the existing OSC 9280 completions wire protocol for all four shells. CHANGELOG-NONE
Contributor
Author
|
This PR was generated with Warp. Comment   |
Two mechanical fixes found by building and running presubmit, neither of which the author's 4GB sandbox could surface: - pty_controller_lifecycle_tests.rs: ShellCompletion doesn't derive (E0369). Assert the results are empty instead. - native_shell_completions.rs: the repo forbids inline #[cfg(test)] test modules (script/check_no_inline_test_modules). Move the tests to a sibling native_shell_completions_tests.rs included via #[path].
…ORE-3795 regression, empty-input handling Critical fixes: - zsh: scope the zle-line-init takeover to the single select iteration it drives, capturing/restoring by widget name (zle -A/-N) rather than functions[...], so a differently-named bound widget (add-zle-hook-widget, used by p10k/zsh-syntax-highlighting/zsh-autosuggestions) survives a completion request instead of being silently replaced forever. - zsh: guarantee the request always terminates independently of the armed flag by restoring the widget unconditionally right after select returns, and make the capture idempotent against repeat firings. - pty_controller.rs: RunNativeShellCompletions was missing from the is_command check in execute_next_queued_write, so the next queued write could be drained straight into a shell mid-select and lost. - zsh: the CORE-3795 fix regressed -- (i) returns one past the array length (not 0) on no match, so the presence guard was always true. Reverted to (I) and restricted the search to the leading flags prefix so a real -d/-ld completion candidate is never mistaken for the flag. Robustness fixes: - zsh/fish/PowerShell: empty decoded line now returns zero matches immediately instead of dumping every top-level command/file. - fish: guard missing/empty hex argument in the decoder (was a stack trace landing in-band); drop "command" from printf for macOS \x decoding, then drop the now-unneeded "--" since fish's builtin printf doesn't treat it as an end-of-options marker. - PowerShell: decode inside try/finally so the OSC terminator is always emitted even if decoding or completion throws; empty/missing hex now decodes to '' instead of crashing on GetString(null). - bash: force IFS for read -ra instead of trusting the session's value; COMP_POINT is now a byte count, not a locale-dependent character count; COMP_WORDS/COMP_CWORD/COMP_LINE/COMP_POINT/COMP_TYPE/COMP_KEY are now local so they never leak into the user's session. - Removed the already-orphaned ^X/list-choices completions path: it had no client-side trigger before this PR, and removing the client's ability to answer the 9280;P read turned it from unreachable into a hang risk if anything ever did trigger it. Test fixes: - Fixed ShellCompletion PartialEq build error (assert is_empty() instead of == Vec::new()). - Rewrote the hex round-trip test to assert the actual per-shell decoder contract (lowercase, unseparated, even-length) instead of just round-tripping through the hex crate. - Replaced a self-referential command comparison in the queueing test with the exact literal, keeping the shell-type-from-session assertion. All shell-side fixes re-verified empirically via PTY-driven harnesses.
…d the input line
In-app verification found that typing with native completions on produced a
new visible block per keystroke, truncated/emptied the real input buffer,
and left the session unable to submit real commands afterward (Enter
produced empty blocks, Up-arrow/Ctrl+U were broken). Root-caused to two
separate issues, both fixed here:
1. `is_in_band_command()` only recognized the literal
"warp_run_generator_command " / "Warp-Run-GeneratorCommand " prefixes
(with a trailing space). All four native-completions generator function
names are longer ("warp_run_generator_command_foreground_completions",
"warp_run_generator_command_native_completions",
"Warp-Run-GeneratorCommand-NativeCompletions") and never matched, so the
client classified every completions request as a normal, visible user
command -- hence one new block per keystroke. Relaxed the check to match
on the shared prefix alone, the same substring convention the shell
scripts themselves already use for this exact purpose
(`_is_warp_generator_command`).
2. Running the generator command as a foreground command necessarily kills
and replaces the shell's real input buffer (see `bytes_to_execute_command`)
to type the invocation and press Enter -- required for zsh's `select`
mechanism, and used uniformly across all four shells. Nothing restored the
user's actual buffer afterward, so on the very next keystroke the shell's
buffer was still empty, corrupting every subsequent request (a two-
character buffer produced a truncated completions request, and the final
Enter submitted whatever fragments had landed on the empty buffer in the
meantime). `PtyController` now tracks the buffer_text a native-completions
request was computed from, and once results come back
(`ModelEvent::CompletionsFinished`), queues it to the front of the write
queue so it is written back to the pty verbatim as soon as the line editor
is active again -- ahead of anything queued in the meantime, including a
newer completions request for what the user has typed since.
Both fixes are shell-agnostic: the prefix relaxation covers all four
generator names, and the buffer restoration is generic to
`RunNativeShellCompletions`, so this should equally resolve the failure for
bash/fish/PowerShell if they hit the same underlying issue.
Added a focused unit test for `is_in_band_command` covering all four
generator name shapes plus a negative case. Could not verify this live (no
computer-use in this environment); requesting re-verification from the
computer-use pass.
…-verification pass - PowerShell: split the kill-buffer chord (Alt+2, an ESC-prefixed two-byte sequence) into its own pty write. PSReadLine sometimes fails to disambiguate it when it arrives concatenated with the command text that follows in a single write/read, leaving the buffer unclouded and typing the command text literally on top of it. Splitting the write (with no explicit delay needed) reliably fixes this, confirmed empirically via a PTY harness. - Fix the phantom block that appeared when PtyController restored the input buffer after a native-completions request: the restore write's echo was being treated as unexpected background output. Added EarlyOutput::push_expected_echo, which registers characters as expected echo regardless of TypeaheadMode (push_user_input only does this for InputMatching mode), and made handle_potential_typeahead consume it first. PtyController now calls this before writing the restored buffer text back. - Defensively exclude in-band/generator command blocks from client-side history: TerminalModel::restored_block_commands() and update_command_history() both now skip is_in_band_command-matching text, so a generator command can never leak into the Up-arrow history overlay (this also plausibly explains the stray "No such widget `zle-line-init'" string reported in phantom blocks, which had identical stale metadata across unrelated shells -- consistent with a restored, pre-fix block from an earlier test run rather than live output). - fish: override fish_title (which by default shows the currently-running command, truncated, in the window/tab title via its own OSC title-setting path, independent of Warp's own hooks) to fall back to its own "just show pwd" behavior for generator commands, matching upstream's format otherwise. Verified with the installed fish for a real command, a generator command, fish's own builtin case, and no argv.
![warp-agent-staging[bot]](https://avatars.githubusercontent.com/in/2196476?s=60&v=4){kind=small}
Contributor
Author
There was a problem hiding this comment.
Overview
Moves native shell completions onto in-band generator commands for zsh, bash, fish and PowerShell, replacing the zsh-only Ctrl-Y/OSC-handshake trigger. Two blockers remain: in-app verification shows the per-keystroke round trip races the user's typing, and PowerShell does not work at all.
Concerns
- Per-keystroke firing is racy by construction. Each request kills the shell's real buffer, runs a foreground command, then writes the buffer back; verification found this intermittently duplicating input (
g→gigi→ screen-filling repetition) and pinning the app at 70-130% CPU for 25+ seconds in bash and fish, requiring a force-kill, with the corrupted text submitted to the real shell on Enter. Whether this should fire on every keystroke at all, or only on explicit invocation, is a design decision to settle before further patches. - PowerShell is non-functional across two fix attempts. Typing produces merged, auto-executed commands (
GeWarp-Run-GeneratorCommand-NativeCompletions 476574) and no completion menu; theAlt+2binding does register, so the failure is in how the chord bytes are interpreted at the live prompt, andBackwardDeleteLineis not a whole-buffer kill in the first place. Consider a PSReadLine key handler that readsGetBufferState()and callsCompleteInputdirectly, which needs no buffer kill, no command text, no Enter and no restore. - fish generator commands land in fish's own history file, which Warp reads, so they appear in the Up-arrow overlay.
bytes_to_execute_commanddocuments that a leading space omits a command from fish history andgenerator_command_foremits none; zsh and bash are unaffected viahist_ignore_spaceandHISTIGNORE. - Exercising the feature requires two non-default settings,
terminal.input.completions_open_while_typing(defaults false) andgeneral.default_session_mode = "terminal". Worth stating in the description so reviewers do not conclude the feature is dead.
Verdict
Checks: build pass, tests pass (this PR's 11 tests; full workspace suite not run), CI skipped (draft), visual proof present
Found: 2 critical, 1 important, 0 suggestions, 0 nits, 1 question
Verified in the running app on Linux across four passes: the shell-side capture is correct in all four shells, including the CORE-3795 description fix. Every remaining failure is in the client-side round trip.
Responding as wilson: Open session · View factory task
…usion - Fixed the regression from e44204c where typing duplicated the buffer (g -> gigi -> gigitgigit, compounding, sometimes CPU-pinning the app). Root cause: push_expected_echo populated the same unmatched_input queue push_user_input uses, so a restore write's echo was surfaced as genuine typeahead via TerminalEvent::Typeahead -> insert_typeahead_text. Real typeahead is meant to be inserted because the editor lost that text; here the editor never lost it (only the real shell's buffer was cleared), so re-inserting it duplicated it on top of what was already there, and the duplication compounded on the next keystroke's restore. Fixed by giving push_expected_echo its own backing queue (EarlyOutput::expected_echo) and a dedicated consume_expected_echo, wired into input()/carriage_return()/linefeed() ahead of the existing typeahead/background-output logic: a match there is dropped entirely now, never surfaced as typeahead and never rendered as background output. handle_potential_typeahead itself is reverted to its original, unmodified behavior. Updated the unit test to assert typeahead() stays empty rather than getting duplicated into it. - Fixed the fish native-completions history leak: fish has no configurable history-exclusion mechanism (unlike bash's HISTIGNORE or zsh's hist_ignore_space) -- a leading space is the only, default, non-configurable way to omit a command from its history file. generator_command_for's fish case never added one, so every request leaked into ~/.local/share/fish/fish_history. Added the leading space, matching the exact convention InBandCommandExecutor already uses for the existing warp_run_generator_command mechanism; the bracketed-paste leading-whitespace preservation this depends on already existed in bytes_to_execute_command. Updated existing tests for the new leading space and added a dedicated test locking in the behavior (and that the other three shells don't gain an unwanted leading space).
…leak The zle-line-init restore's else branch unconditionally ran `zle -D zle-line-init`, which errors if the widget no longer exists by the time the select loop returns (e.g. a chained hook that itself rebinds zle-line-init via add-zle-hook-widget during the chain call). Guard it with the same existence check the other branch already gets for free from zle -A's auto-creation semantics. Also: warp_set_title_active_on_preexec (zsh and bash) is a preexec hook that fires for every command including generator commands, and had no exclusion for them (unlike warp_preexec's own PID-killing logic, which already excludes warp_run_generator_command*). This briefly set the tab title to 'warp_run_generator_comma...' during every native-completions request. Fixed both to skip title-setting for generator commands, matching the existing exclusion convention.
…mand execution Replaces the kill-buffer+type+Enter+restore idiom for PowerShell with a dedicated PSReadLine key handler (Alt+3) that reads the buffer directly via GetBufferState, computes completions via CompleteInput, and reverts the buffer -- never AcceptLine. This eliminates all three PowerShell failure modes found in verification (kill-buffer chord atomicity, buffer concatenation, auto-execution) at the root, since none of them can occur when nothing is ever typed as a command or submitted: - generator_command_for's PowerShell case now returns just the hex-encoded buffer text, with no function-call syntax at all. - send_write_to_event_loop's RunNativeShellCompletions handling branches on shell_type: PowerShell types the hex text (registered via push_expected_echo so it isn't rendered as a phantom block) followed immediately by the new trigger chord, with is_for_command=false and no buffer_text stored for restoration, since nothing here ever touches the real buffer to begin with. - execute_next_queued_write's is_command gating is now shell-type-aware for the same reason: PowerShell's write never transitions the line editor back to active the way a real command's precmd would, so gating queue draining on it would stall forever. - Removed the old Warp-Run-GeneratorCommand-NativeCompletions function and its module export; PowerShell no longer needs the AddToHistoryHandler exclusion either, since nothing is ever submitted. Verified empirically end to end via tmux (needed a real terminal size -- a bare 0x0-sized PTY made RevertLine throw): the Alt+3 binding registers correctly, Get-Ch decodes and completes to Get-ChildItem with its description via the same OSC 9280 wire format the other three shells use, the buffer ends up empty afterward (confirmed via GetBufferState), nothing auto-executes, and the session stays fully functional afterward. Added a unit test for the new dispatch path and updated the existing generator_command_for tests for the format change.
…ces the error
The previous fix (checking \${+widgets[zle-line-init]} before deleting) does not
actually prevent the "No such widget \`zle-line-init'" error in the common case:
zle-line-init still exists at that point (we are the ones who bound it), so the
elif branch is still true and zle -D still runs, still corrupting zsh's internal
state for the next prompt read. Verified empirically with a minimal repro (select
+ a zle-line-init handler that calls accept-line on itself, then zle -D
zle-line-init) that the elif-guarded delete still reproduces the error, and that
never deleting the widget in the "nothing was bound before" case (only restoring
when something WAS bound) does not.
…tions request The previous fix (e62d9c3) stopped restoring/deleting zle-line-init after each select, leaving our capture widget permanently bound to avoid the 'No such widget' error from a chained hook rebinding it during the chain call. But it didn't guard the *next* request's own takeover: on request 2+, zle-line-init is already bound to our own capture widget, so `zle -A zle-line-init _warp_saved_zle_line_init` aliases the saved-widget name to itself. The widget's own chain-to-saved-widget call then recurses into itself indefinitely (measured: 'maximum nested function level reached'), so it never emits the OSC terminator or calls accept-line, leaving the select blocked on a real read forever -- wedging the session (no further menus, Enter stops working) with nothing visible in the GUI, since the select's stderr redirect swallows the error. Fixed by comparing the current zle-line-init binding against our own widget name before capturing: skip the takeover entirely when it's already us, since there's nothing new to save. Verified with the minimum bar requested: at least two (here, three) consecutive completions requests in one session, both with and without a simulated chained hook (add-zle-hook-widget style, matching how p10k/zsh-syntax-highlighting/zsh-autosuggestions register). All three requests return the correct 8 matches with clean stderr in both scenarios; the chained hook's call count increases consistently across requests (confirming the chain keeps working, not just not-crashing); and the session remains fully responsive to ordinary commands afterward in both scenarios.
…and a phantom-block race - last_completed_command_text() (used as the vertical tab's primary label fallback whenever the OSC-set title equals the working directory, which is the common idle-prompt case) never excluded in-band command blocks, so a just-completed native-completions request's full, untruncated command text could surface as the tab label. This explains why the leak showed the complete command rather than warp_title's 25-char-truncated output, and why the earlier zsh/fish preexec-hook guards didn't fully address it: this path never went through warp_title in the first place. Fixed by excluding is_in_band_command_block() blocks, matching restored_block_commands()'s existing filter. - fish_title's leading-space-defeats-the-match issue (found in the last verification round) is fixed by trimming before matching. - warp_preexec's generator-command-detection guard had the same leading-space issue, plus a separate, pre-existing bug: 'test (! string match -q ...)' always evaluates false regardless of the match, since string match -q prints nothing for '!' to negate via command substitution, and bare 'test' with no arguments is false. This meant stale generator PIDs were never killed for *any* command, not just ones affected by the leading space. Fixed by using fish's own 'not', which negates a command's exit status directly, verified empirically for both a real command and a generator command. - Root-caused and fixed the intermittent phantom block containing typed text: ModelEvent::CompletionsFinished's buffer-restore write and LineEditorStatusEvent::Active's input-reporting-sequence write both push_front from the same underlying trigger (the shell returning to a fresh prompt after the generator command completes), in an order that depends on unrelated PTY buffering/event-processing timing. When input reporting fires after the restore, it reports and clears the text just written back, producing PTY output push_expected_echo never registered -- rendering as an unexpected background (phantom) block. Fixed with a one-shot flag that skips re-queueing input reporting immediately after a restore, since it would be redundant: the client already knows the buffer's contents.
…ore than once per redraw Root cause, from PHANTOM_DIAG evidence on a real session: push_expected_echo registered the restored buffer's characters exactly once, but both zsh's ZLE and fish's line editor echo that buffer more than once while redrawing after a native-completions restore -- zsh echoes one character, returns to column 0, then reprints the whole line; fish echoes the whole buffer, returns to column 0 (twice), then echoes it again. The one-shot queue had nothing left to match on the second (or third) copy, so the surplus characters fell through to an unmatched background block -- frozen at one character for zsh, since the surplus overwrites in place at column 0 each cycle, and growing with the buffer for fish, since the whole buffer is re-echoed and re-mismatched every keystroke. Fixed by replacing the one-shot VecDeque-based queue with a Vec plus a match-position index: matched content is never removed, only the position advances, and a carriage return rearms the position back to 0 unconditionally (regardless of how much of the current pass matched), since a carriage return is exactly what precedes each repeat of the echo. push_expected_echo now replaces the registered content outright rather than appending, since each restore is an independent echo to expect. Defensively cleared on precmd too, so a restore whose echo never fully arrives can't bleed into an unrelated later prompt cycle. Added three regression tests reproducing both shells' exact echo shapes from the diagnostic evidence, plus one confirming the precmd boundary.
…t-suite build error Diagnostics on a real session showed CompletionsFinished (and the push_expected_echo call it triggers) fires when 9280;B is parsed, which precedes the shell's own in-band-command precmd DCS -- so precmd normally lands inside the restore window, not after it. The defensive clear added alongside the matcher fix wiped a just-registered echo before its own characters had even arrived in ~5 of 6 restores, which is worse than the original bug: the whole buffer fell through instead of just a surplus copy. Removed the clear; push_expected_echo replacing its content outright on every call already bounds staleness, which was the clear's only intended purpose. Replaced the test that encoded the clear's behavior (and so was passing while asserting the bug) with two tests for the actual invariants: the registration survives a precmd within the same restore window, and staleness is bounded by the next push_expected_echo call rather than by precmd. Separately, pty_controller_lifecycle_tests.rs used bytes.as_ref() on a Cow<'_, [u8]>, which is E0283-ambiguous once typed_path is in the dependency graph (it adds a competing AsRef impl) -- present since the pwsh test landed, unrelated to this commit's own changes. Switched to slice indexing, which is unambiguous.
…e current one Diagnostics showed a third echo shape: fish sometimes returns to column 0 mid-line and continues the same echo from wherever it left off, rather than restarting from the beginning. The unconditional rearm (reset to 0) from the previous fix discarded the in-progress match position in exactly this case, so the continuation was compared against the wrong expected character and fell through. Replaced the single match position with a set of live candidate positions (expected_echo_positions). A carriage return now *adds* a position-0 candidate without discarding whatever was already live, since it isn't knowable in advance whether a given carriage return means a restart or a mid-line continuation. Each subsequent character advances every candidate whose next expected character matches it and drops the rest; a character counts as expected echo if any live candidate predicts it. This is a generalization of the two-shape fix, not a special case: with only one candidate ever live, it behaves exactly as before. Added tests for the exact split-continuation shape from the diagnostics, its minimal two-character form, and the ambiguous case where a carriage return leaves two candidates that both match the same next character.
…ive-shell-completions-generator
warp-agent-staging
Bot
changed the base branch from
master
to
factory/zsh-compadd-describe-flag-fix
August 19, 2026 15:57
`FeatureFlag::NativeShellCompletions` had no corresponding cargo feature, so the only ways to turn it on were editing `DOGFOOD_FLAGS` or setting the `ForceNativeShellCompletions` private pref -- unlike every other flag, which can be enabled per-build with `cargo run --features <name>`. Declare `native_shell_completions` in the app crate and map it to the flag in `enabled_features()`, following the same pattern as the neighboring completions flags. It is not added to any default or channel feature set, so the flag stays off unless asked for explicitly.
…existing fallback The requester hit this: in natural-language mode, native shell completions being enabled anywhere disabled the file-path completions fallback that AI input mode depends on entirely, and also sent the natural-language buffer text to the shell for completion, which the shell has no basis to answer meaningfully. Confirmed via a git diff against master that this file was never touched by this PR: the gap already existed for zsh (the only shell native completions supported before this PR), and was reachable whenever a zsh session had the feature/pref on while in AI input mode. This PR's expansion of native shell completions to all four shells, plus the ForceNativeShellCompletions pref used for testing it, is what made the requester actually hit it, not a change to this file. use_native_shell_completions gated fallback_strategy (via a match keyed only on completions_trigger) and independently gated whether a native shell completions request was dispatched at all (via a check on use_native_shell_completions alone, regardless of trigger). Both need input_type excluded: fallback_strategy's Keybinding/SlashCommandAutoOpen case needs the same file-path fallback AI mode already used before this existed, and the native-completions dispatch must never fire for AI mode at all, on any trigger -- there's no command spec to hand a natural- language buffer to a shell for. Extracted the eligibility and fallback-strategy decisions into small pure functions (should_use_native_shell_completions, completions_fallback_strategy_for_trigger) so they're directly testable without driving the full Input view's GUI test harness, and added eight unit tests pinning every branch, including the exact regression: AI input mode must keep the FilePaths fallback for an explicit Tab press even when native shell completions would otherwise be eligible.
… request would immediately undo Correctness cleanup, NOT a fix for the reported lone-trailing-character ghost block: the requester's own repro (Tab-triggered, a single completions request, on a buffer that was fully typed before the request was made) has no newer request queued behind the restore at all, so this change cannot explain or fix that symptom. Pushing it on its own merits. The defect: CompletionsFinished queues the buffer restore to the front of pending_writes and drains it via execute_next_queued_write. That function is meant to stop draining immediately behind a foreground command -- RunNativeShellCompletions already gets that treatment for the three shells that run it as one -- but the restore write undoes such a command's buffer-clearing effect without being flagged as needing the same protection, since it goes out as a plain PtyWrite::Bytes rather than being recognized as tied to a command's aftermath. If a newer completions request's own write is already sitting in the queue behind the restore when the restore drains, execute_next_queued_write's existing recursion sends that newer request's kill-buffer immediately behind the restore, before the shell can have processed it. I originally described this fix as gating execute_next_queued_write's is_command check on the restore the same way it already does for RunNativeShellCompletions. Writing the actual change surfaced that this would deadlock: the gate's only means of unblocking is the shell's own precmd firing LineEditorStatusEvent::Active again, and nothing about a plain buffer write -- no command runs, no prompt cycle happens -- ever causes that to fire on its own. Gating on it would leave every write queued after a restore stuck until an unrelated real command happened to run, which is worse than the race it would fix. Implemented instead as: don't queue the restore at all when a newer RunNativeShellCompletions request is already waiting behind it, since that request's own kill-buffer is about to clear the line again anyway, making the restore pointless to send. This sidesteps the race without touching execute_next_queued_write's draining logic or its unblocking condition at all -- the newer request proceeds exactly as it would otherwise, and produces its own, current restore once it completes. No test added: the existing PtyController test file has no precedent for driving ModelEvent::CompletionsFinished's dispatch path (via the raw Event channel ModelEventDispatcher forwards from) synchronously inside App::test, and I don't have a confirmed way to verify a test exercising it wouldn't just pass trivially without exercising the code path.
…ssion A future reader of the CompletionsFinished handler will see a restore being deliberately dropped and needs the reasoning at the point where that happens, not only in a review thread.
…eal command starts HELD, NOT PUSHED: waiting on macOS reproduction to confirm the trailing-fragment shape before landing it, per explicit instruction. Verified data from a realistic rc (starship, zsh-autosuggestions, zsh-syntax-highlighting) showed the previously-suspected mechanisms are not it: no BUFFER mutation from compadd (ruling out common-prefix insertion), and the write-ordering skip from 171901e's precondition occurring without leaking (ruling that out too, and it would leak a leading fragment, never trailing). The actual shape: after a full pass already matched the whole registered text, a carriage return is followed not by another full repeat but by only a short trailing fragment -- e.g. just the buffer's last character. Seeding only position 0 on a carriage return can't match a restart that isn't at the beginning, so the fragment falls through and starts a background block. Generalized maybe_rearm_expected_echo to add every position in the registered text on a carriage return, not just 0. This is deliberately permissive, documented as such in the updated doc comment: once a carriage return has been seen, any single incoming character that occurs anywhere in the registered text is absorbed rather than surfaced as background output, for as long as the candidate it matched keeps predicting correctly. Traced this by hand against all six existing tests in early_output_tests.rs plus a new one pinning the exact trailing-fragment shape: every existing case either seeds the same positions as before (no behavioral change) or a superset that still resolves to the same matched/unmatched outcome. Being more permissive raised a real safety question: what ends the window? expected_echo is only replaced by push_expected_echo, and the defensive precmd clear was removed in 838bac9 because it raced the restore. With nothing else bounding it, a stale pattern from the last restore would stay live indefinitely -- and after this change, any later carriage return (including one from a completely unrelated, later real command) rearms every position in it again, risking silently swallowing a character of that command's own echo or output if it happened to match something in the stale pattern. Traced start_active_block (BlockList) and confirmed it only calls reset_user_input, never touching expected_echo/expected_echo_positions -- so nothing was bounding this. Added EarlyOutput::reset_expected_echo and call it from start_active_block, the transition a real user command goes through (start_active_block_for_in_band_command, what a generator/completions command uses instead, is intentionally left untouched, since clearing there would defeat the restore before it ever completes). Added a test pinning this: after a full restore match, starting a real command, then a later carriage return and a character that would have matched the stale pattern's first position -- that character must show up as real background output, not be silently dropped. I don't have a way to compile-check or run this test suite in this sandbox; verified only by hand-tracing the state transitions against the exact logged sequence, against every existing test's inputs, and against this new safety property.
…just on carriage return macOS reproduction (raw PTY bytes, both diagnostic-logged and captured directly) showed the carriage-return-only rearm from the previous commit was addressing the wrong rewind entirely for the reported symptom, and identified a second, wider gap in the same class. The trigger is terminal.input.honor_ps1 = true (the shell draws its own prompt rather than Warp). With it off, zsh's redraw is `s`, CR, then the full reprint -- the carriage-return rearm from the previous commit already handles that. With it on, the same restore's redraw is `s`, a literal backspace, then the full reprint -- zero carriage returns anywhere in the exchange. Since maybe_rearm_expected_echo is only reached from carriage_return(), and backspace() previously delegated straight to the background block with no interaction with expected_echo at all, the reprint's own first character (checked against whatever position the pre-backspace character advanced to) had no live candidate that could match it, and fell through. Measured: 0 backspaces across 16 requests with the prompt honored off, exactly 1 backspace in each of 10 requests with it on. A second, wider gap in the same class: with zsh-syntax-highlighting loaded, a further redraw pass recolours the already-matched command word using CUB (cursor-backward, \x1b[<n>D) rather than a carriage return or backspace. All of the recoloured characters (up to 8 in the measured case) fell through, rendering a visible block. Both are rewinds, the same class of event a carriage return is, but with one difference a carriage return doesn't have: the exact distance is known (1 for backspace, the escape sequence's own parameter for CUB), so each live candidate's new position can be computed directly -- position minus distance -- rather than seeding every position in the pattern the way the carriage-return case has to (a carriage return is an absolute jump to column 0, not a relative move, so its distance back isn't recoverable from the byte alone). Added EarlyOutput::rearm_after_rewind(distance), called from new backspace() and move_backward() implementations in EarlyOutputHandler (previously both blindly delegated to the background block with no interaction with expected_echo at all). Kept maybe_rearm_expected_echo and its all-positions behavior for carriage returns unchanged. Traced both new shapes by hand against the exact measured byte sequences (an 11-character restore with a leading backspace before the full reprint; a full match followed by an 11-column CUB and an 8- character partial recolour reprint) to confirm neither falls through under the new logic, and added a test for each pinning the exact shape. Also re-traced all eight prior tests in early_output_tests.rs (the six from before this fix plus the two added in the previous, carriage-return-only commit) against the new backspace/CUB paths: none of them exercise backspace or move_backward at all, so this change is a pure addition for them -- no behavioral change, and no regression risk from the new code paths being unreachable in those tests. Documented the boundary explicitly in the expected_echo_positions doc comment, per instruction: carriage return, backspace, and CUB are handled; other rewind mechanisms -- an absolute cursor move, or other column-addressing escape sequences -- are not, and would need the same treatment as backspace/CUB if a redraw shape using one of those ever surfaces. Kept reset_expected_echo on the real-command path from the prior commit unchanged; that gap was independent of which rewind mechanism triggers the leak and remains fixed the same way. I don't have a way to compile-check or run this test suite in this sandbox; verified only by hand-tracing the state transitions against the exact measured byte sequences and against every existing test's inputs.
The all-positions widening from an earlier commit was motivated by a trailing-fragment-after-a-carriage-return hypothesis for the reported ghost block. That hypothesis was disproven by macOS reproduction: the actual rewind was an unhandled backspace (or, in a further case, CUB), not an ambiguous carriage return, and both are now handled by rearm_after_rewind with their exact, known distance. Traced both measured shapes against position-0-only carriage-return rearm plus the new distance-based rearm: neither depends on the carriage-return path being widened, since neither shape involves a carriage return at all. The all-positions widening was therefore never actually needed to explain a real, measured symptom -- it was carrying risk (a materially wider blast radius for whatever stale-pattern gap existed) without a corresponding benefit. Reverted to the original, narrower rule. Removed the test written for the disproven hypothesis (test_push_expected_echo_survives_a_carriage_return_followed_by_only_a_trailing_fragment): retraced it against the reverted logic and confirmed it now fails (a bare trailing fragment after a carriage return, with no backspace or CUB involved, has no evidence of occurring in a real session and is exactly the kind of shape this revert is meant to stop guessing at). Re-traced all other existing tests, including the two added for backspace/CUB in the prior commit, against the reverted carriage-return logic: none of them depend on the widening, so no other test changes. Also verified via `git show <sha>:app/src/terminal/model/blocks.rs` that `reset_expected_echo`'s call site in `start_active_block` (added two commits ago) is present in the already-pushed history -- it was reported as missing, but grepping the actual committed blob content at the pushed SHA (not just the local working tree) confirms it is there. No change needed for that; flagging in this message since it was raised as a blocking concern.
…o assert directly rather than via block routing Two changes, both requested after the first machine-checked run (8a5cea4 compiled clean, 19/20 tests passed including both backspace/ CUB shape tests -- the backspace and CUB handling is now verified by a compiler, not just hand-traced). **Forward motion.** macOS reproduction found a further gap in the same class: with honor_ps1 off, a syntax-highlighting recolour pass rewinds with a carriage return, re-echoes the 8-character command word, then skips forward over the unchanged remainder with CUF (\x1b[<n>C) rather than re-echoing it. Nothing consumed that, leaving the sole live candidate stranded at its pre-move position instead of advancing past what the CUF skipped over -- which mismatches, and leaks, whatever arrives next. This is very likely what actually explains the original report (a single trailing character leaking), which neither Linux nor macOS reproduced directly, since the buffer size in the original report (11 characters, "starship pr") is exactly the case where the stranded candidate and the correctly-advanced one land in the same place by coincidence, and it takes a longer buffer or a different split to see the two diverge. Added EarlyOutput::advance_after_forward_move, symmetric with rearm_after_rewind but shifting forward: for a forward move, unlike a rewind, there's no ambiguity about whether a redraw follows, since the terminal moving the cursor over content without redrawing it is itself an assertion that the content already there is correct. Gave move_forward its own EarlyOutputHandler implementation (previously blindly delegated in the generic background-output section, same as backspace/move_backward were before the prior commit). Documented in the expected_echo_positions doc comment which four motions are now handled -- carriage return, backspace, CUB, CUF -- and that absolute column addressing (\x1b[<n>G, \x1b[<n>;<m>H) is the next gap of this kind if it surfaces, along with why honor_ps1 is the actual switch between the absolute (carriage-return) and relative (backspace/CUB) rewind mechanisms, not just a detail of one measured session. Added a test for the measured recolour+CUF shape, using a longer buffer ("starship prompt") than the exactly-measured "starship pr" so the CUF lands mid-pattern rather than exactly at the pattern's end, where the stranded and correctly-advanced positions would otherwise coincide and the test couldn't tell the fix apart from its absence. Traced it by hand: without the fix, the stranded position 8 mismatches the recolour's continuation and leaks; with it, the candidate advances to 11 (position 8 + 3, matching the CUF's parameter) and everything after matches cleanly to the end. **Test fix, not a code fix.** The first machine-checked run flagged test_starting_a_real_command_clears_a_stale_expected_echo_registration as the one failure out of 20. Read the dispatch path before concluding either way: BlockList::is_early_output() returns false once the active block is started() (which start_active_block sets), and BlockList's own dispatch routes input straight to the active block once that's true, bypassing EarlyOutputHandler -- and therefore consume_expected_echo -- entirely regardless of whether reset_expected_echo ran. So the test's assertion (a background block containing the fed-in character) was unobservable by construction: a character fed in after start_active_block can never land in a background block either way, which is why it failed for a reason unrelated to what it claimed to test. reset_expected_echo's call site in start_active_block is unconditional and was never the problem. Rewrote the test to assert directly on what it's meant to pin down: that early_output_mut().expected_echo and .expected_echo_positions are both empty immediately after start_active_block, accessed directly since the test module is a descendant of the early_output module and can see its private fields. Dropped the carriage-return-and-character steps that were trying to observe the reset indirectly through block routing. I don't have a way to compile-check or run this test suite in this sandbox. The two shape-pinning tests from the prior commit are now machine-verified (19/20 passed on 8a5cea4); the forward-motion test and this test's replacement are hand-traced only, pending the next machine-checked run.
Contributor
Author
There was a problem hiding this comment.
Overview
Second review of the accumulated changes since the first one, concentrated on the expected-echo state machine that now decides whether shell bytes are absorbed or rendered. The change is in good shape overall — rewind/advance arithmetic is correct at the boundaries, the lifecycle clear is ordered correctly against command submission, and the old widget/^Y/SendCompletionsPrompt path is removed cleanly — but two items need a decision before this is enabled anywhere.
Concerns
- A live expected-echo registration silently deletes characters from unrelated background output, repeatedly. Measured by extracting the matcher and running it: with pattern
git ch, outputgrep -rn foo\r\nrenders asrep -rn foo\r\n; with patternls -la, a progress stream\rloading 10%\rloading 20%\rloading 50%\rloading 99%\rdone\nloses four characters. A mismatching candidate is never dropped andmaybe_rearm_expected_echore-seeds position 0 on every carriage return, so an inert, fully-matched pattern is re-armed by the next\rand eats another character — and carriage returns are exactly what progress-printing background jobs emit. This corrects the PR body's earlier framing: the gap is not about a registration outliving its request, it only has to still be live when unrelated output arrives, which is its normal state. The correction is to bound the registration in time (clear once the shell settles back at a prompt after the restore), not to narrow the matcher: narrowing does not help since the first character matched in one case, and clearing on full match is wrong because the fish double-echo and the CUB recolour legitimately match again after a full match. - The zsh
zle-line-inittakeover can still recurse when a plugin rebinds after us. The self-check covers aliasing our own widget to itself, but not the reverse order: our widget is installed, a deferred plugin (zinit turbo,zsh-defer,zvm_after_init) then registers a hook viaadd-zle-hook-widgetpreserving ours as the previous binding, and the next request saves that dispatcher and rebinds to us — so we chain to the dispatcher, which invokes us. That is the same nested-function-level failure as before, and it fires on the unarmed path, wedging every ordinary prompt read rather than one request. A reentrancy guard inside the widget closes both this and the case the current check handles. - The expected-echo tests only assert one direction. All eleven assert that nothing leaked; none asserts that anything still renders. Replacing
consume_expected_echo's body with "swallow whenever a pattern is registered" leaves the suite green, which is the same shape as the finding above taken to its limit. Two tests close it: unmatched output must reach the background block verbatim, and thegit ch/grepcase, which fails today.
Verdict
Checks: build pass (cargo check clean, 20/20 early_output tests, verified on a separate machine), tests pass, CI skipped (draft), visual proof present — verified in-app on Linux and macOS, 40 restores across four configurations with zero leaks after the rewind fixes
Found: 1 critical, 2 important, 5 suggestions, 6 nits
The remaining items are recorded with the author: candidate positions are never pruned, consume_expected_echo can steal genuine typeahead in InputMatching mode (bash 3.2, macOS system bash, now a supported shell), in-flight state is assigned before the write can be rejected which contradicts the skip-safety comment, fish_title is overridden unconditionally though fish.sh runs after config.fish, one test cannot observe the property it names, two doc comments still describe the restore echo as "recognized as typeahead" (the wording behind the earlier duplication regression), and bash_body.sh handles only -F compspecs.
Responding as wilson: Open session · View factory task
…nit reentrancy guard; add one-directional test coverage
Second adversarial review found one high-severity and two medium-
severity issues in the accumulated diff. Fixes all three.
**HIGH: a live pattern silently deleted characters from real,
unrelated background output.** Extracting the matcher into a
standalone program and feeding it independent input reproduced it
directly: pattern "git ch", background output "grep -rn foo\r\n"
rendered as "rep -rn foo\r\n" (the leading "g" consumed, since it
happens to match the registered pattern's own first character); a
progress-printing job's own carriage returns re-armed a supposedly
inert, fully-matched pattern and it kept eating characters. The
pattern had no time bound at all -- once registered, it stayed live
until the next `push_expected_echo` call happened to overwrite it,
which could be an arbitrary amount of real, unrelated shell activity
later.
Bounded it to a single restore's own redraw window. `PtyController`
now calls the existing `EarlyOutput::reset_expected_echo` a second
time, from the `LineEditorStatusEvent::Active` handling that already
fires once per restore (the same place `just_restored_native_completions_buffer`
already gets cleared). `Active` is emitted a fixed delay
(`LINE_EDITOR_ACTIVATION_DELAY`) after the shell reaches precmd/end-
prompt, which is well after any redraw -- including a later recolour
pass -- following the restore write has had a chance to play out, so
this is the natural point past which the registration is no longer
describing an in-flight redraw. `reset_expected_echo`'s doc comment
now describes both call sites (the pre-existing one on a real command
starting, and this new one) and the concrete corruption this closes.
Also fixed two comments that still said a matched echo is "recognized
as typeahead" -- the wording that caused an earlier round's
duplication regression, since a typeahead match gets *inserted* into
the input editor, which is wrong here (the editor's copy was never
cleared). A match here is dropped entirely; the comments now say so.
**MEDIUM: the zle-line-init takeover still recurses in the reverse
bind order.** The self-check added for the "we alias the saved name to
ourselves" case only closes that one case. A deferred plugin loader
(zinit turbo mode, zsh-defer, zvm_after_init) that registers its own
zle-line-init hook via add-zle-hook-widget *after* our takeover
preserves our widget as the previous binding rather than the reverse,
so zle-line-init ends up bound to that plugin's own dispatcher, which
itself still calls us. The self-check doesn't recognize that as
"already us", saves the dispatcher, and rebinds to us again -- chaining
to that saved dispatcher then invokes us a second time, the same
"maximum nested function level reached" recursion as the self-check's
case, reached from the opposite bind order and on the *unarmed* path,
wedging the shell on every ordinary prompt read.
Added a reentrancy guard inside _warp_native_completions_zle_line_init
itself: an early return if already running, set/cleared around the
function body via zsh's `{ ... } always { ... }` construct so the flag
clears on every exit path including the early returns. This closes
both the self-check's case and the reverse-order case the same way,
without needing to reason about which binding order caused it. Kept
the existing self-check unchanged -- it still serves a distinct
purpose (avoiding overwriting the saved widget with an alias to
ourselves, which would lose the chain to a genuinely different,
earlier-saved hook) that the reentrancy guard alone doesn't cover.
Verified against the exact reverse-order scenario with an isolated,
logged reproduction: a real interactive zsh session, our takeover, a
simulated deferred plugin registering its dispatcher afterward, a
second takeover (which saves the dispatcher, confirmed via
${widgets[_warp_saved_zle_line_init]}), then firing zle-line-init
through a real `select`-driven ZLE context. The trace shows the
expected shape exactly: our own widget runs, chains to the saved
dispatcher, the dispatcher's own chain call back into us hits
"GUARD-BLOCKED" and returns immediately (no recursion), the dispatcher
continues normally and the plugin's own hook still fires, control
returns to our outer call, and it completes normally (accept-line
runs, the always-block clears the guard). The shell remained fully
responsive afterward.
**MEDIUM: the test suite only asserted one direction.** All eleven
`push_expected_echo` tests asserted that nothing leaked; none asserted
that anything still renders when it should have. Replacing
`consume_expected_echo`'s body with `!self.expected_echo.is_empty()`
-- absorbing everything whenever anything is registered, regardless of
match -- would have left the whole suite green. Added two tests
closing this: one feeding output that shares no prefix at all with a
live pattern (must render verbatim), and one reproducing the measured
"git ch" + "grep -rn foo" corruption directly, using
`reset_expected_echo` to simulate the restore window having closed
(exactly what `PtyController` now does), asserting the previously-lost
output renders in full.
I don't have a way to compile-check the Rust changes or run this test
suite in my sandbox; the zsh reentrancy fix is the one piece here
verified against a real, interactive zsh session in this round rather
than only hand-traced.
…e mismatch, not on LineEditorStatusEvent::Active
`fd2ee71`'s bound was wrong. The verifier's logged ordering from a
real GUI session settles it directly, without needing to reason about
what `Active` should mean from its name:
push_expected_echo("starship pr"): after ... positions={0}
LineEditorStatusEvent::Active fired, ..., pending_writes_len=1
LineEditorStatusEvent::Active: skipping input reporting (just restored)
consume_expected_echo('s'): match=true positions_before={0} positions_after={1}
input('s'): consumed as expected_echo
`Active` fires with the restore write still sitting in
`pending_writes` (`pending_writes_len=1`) -- it's that handler's own
`execute_next_queued_write` call that flushes it. Every echoed
character arrives strictly after. So `Active` is not "well after any
redraw has played out" for this path; it is before the write goes out
at all. Clearing there discarded the pattern before a single echoed
character could arrive, which would have made the whole restored
buffer fall through -- an 11-character phantom block instead of the
1-character one being fixed, and it would have started leaking in the
`honor_ps1 = false` configuration that was previously clean. Reverted
that call and the doc comments that justified it.
The window has no clean opening-and-closing *signal* pair in this
codebase: `Active` fires too early, and nothing else was found that
reliably lands only after a restore's own echo -- including any later
recolour/redraw passes -- has fully arrived. Rather than keep
searching for one, bound the window by what the data itself proves:
`consume_expected_echo` now clears the registration on the first
character that no live candidate predicts. A live pattern's whole
purpose is to explain an in-flight redraw; the first character it
can't explain is either the redraw genuinely being over, or something
having gone wrong, and either way there is no more legitimate echo
left to protect past that point.
**What window this actually bounds, stated plainly**: from
registration until the first unmatched character, not "restore start
to restore end" in wall-clock terms. Traced by hand against every
existing shape (plain match, CR-then-restart, CR-then-continue,
ambiguous double candidates, backspace, CUB, CUF) to confirm none of
them ever produces a spurious mismatch mid-sequence -- at least one
live candidate always predicts correctly throughout a genuine,
measured redraw, so the new clearing branch is unreachable on all of
them and none needed updating.
**The residual exposure, and it is real, not eliminated**: if an
already-exhausted pattern (fully matched, its only remaining position
out of bounds) is rearmed by some *unrelated* command's own carriage
return/backspace/CUB, and that unrelated command's own output happens
to start with a character the rearm coincidentally predicts, that one
character is swallowed before the very next, genuinely non-matching
character closes the window. Added a test
(`test_a_rearm_can_still_leak_exactly_one_coincidentally_matching_character`)
that demonstrates exactly this and pins the bound at one character,
not zero -- register "gi", fully match it, an unrelated carriage
return rearms position 0, then "grep" arrives and renders as "rep".
This is progress over the pre-existing unbounded case (a stale pattern
could otherwise corrupt output across arbitrarily many later,
unrelated commands until the next completions request happened to
overwrite it) but it is a bound, not a fix, and the PR body should say
so plainly.
Also added a test for the complementary, common case where the
exposure is fully eliminated: once a pattern has been reset (by
`start_active_block`, or by this same mismatch-triggered clear having
already fired), completely unrelated output renders with zero
characters lost, including when it happens to share a leading
character with the (now-cleared) stale pattern.
Removed `TerminalModel::reset_expected_echo`, the passthrough added
for the reverted `PtyController` call -- it has no other caller.
`EarlyOutput::reset_expected_echo` itself is unchanged and still used
by `BlockList::start_active_block`.
Same standing caveat: hand-traced, not compiled, in this sandbox. Held
locally rather than pushed per instruction, pending the verifier's
confirmation from the same GUI pass that surfaced the ordering
problem in the first place.
…hell
Root cause of "$_. and $_.Na member completions return nothing" despite
the shell producing every candidate correctly (measured: 300/300 candidates
received across six requests, zero truncated downstream): the client
computes its own replacement span with a whitespace heuristic, then
filters the shell's candidates against that span sliced out of the
buffer. For `Get-Process | Where-Object { $_.`, PowerShell's own
CommandCompletion reports a zero-length replacement span right after the
`.` (ReplacementIndex=32, ReplacementLength=0), but the whitespace
heuristic derives the query "$_." -- no member name contains that
substring, so every one of the 124 correct candidates gets filtered out
and the menu never opens. `$_.Na` (one correct candidate, `Name`) fails
the same way with query "$_.Na" instead of PowerShell's own "Na". Cmdlet
and flag completions ("Get-Proc", "Get-Process -") happen to work only
because the whitespace token coincides with PowerShell's own span.
Chose the protocol-extension option (report the shell's span, use it
verbatim when present) over suppressing client-side filtering: PowerShell
already had `ReplacementIndex`/`ReplacementLength` in hand, so it costs
one new OSC sub-message rather than touching all four shell scripts, and
unlike suppressing the filter, it doesn't throw away the legitimate cases
where filtering is what narrows a large candidate set to what's relevant
(e.g. "Get-Process -").
- New OSC 9280;S sub-message reports `<start>,<length>` byte offsets;
optional, so shells that don't send it are unaffected. Threaded through
`Handler::on_completion_replacement_span_received` ->
`IsReceivingCompletionsOutput::Yes.replacement_span` ->
`Event`/`ModelEvent::CompletionsFinished`'s new second field ->
`PtyController`'s results channel -> `input.rs`, which now prefers it
over the whitespace-derived span when present.
- `pwsh.ps1` emits it from `CommandCompletion.ReplacementIndex`/
`ReplacementLength`. Known gap, stated in the emission comment:
these are .NET UTF-16 code-unit offsets, sent as-is, so this is only
exact for ASCII lines -- true of every reported case, but non-ASCII
PowerShell lines are a real limitation worth documenting rather than
solving here.
- Verified empirically with pwsh 7.6.5, installed in-sandbox: the hex
round-trip decodes correctly for all five test lines, the emitted OSC
byte sequence parses exactly as the new Rust-side constant/split
expects (`ESC ] 9280 ; S ; 32,2 BEL` for `$_.Na`), and the derived
query for each of the five lines (`Get-Process | Where-Object { $_.`,
`$_.Na`, `Get-Proc`, `Get-Process -`, `$PSVersionTable.`) now matches
PowerShell's own span exactly, retaining the correct candidate set
(124, 1, 1, 18, 32 respectively) instead of discarding it.
- Confirmed unverified, not compile-checked: the Rust-side plumbing.
I cannot compile the full `warp` crate in this sandbox; `./script/format`
accepts every touched file, and I traced the new call chain (including
every existing call site of the changed types) by hand rather than by
running rustc.
- Untested, stated as a limitation for the PR body: whether bash, zsh and
fish have an equivalent bare-member-completion gap, and whether
`completions_open_while_typing = true` takes a different filtering path.
Neither was exercised in this change.
…ttern `da66d6b`'s "clear on the first genuine mismatch" rule broke every carriage-return-driven redraw shape (7 of 23 `early_output` tests, verified by the reviewer on the pristine pushed blobs, not local instrumentation). Root cause: `carriage_return()` and `linefeed()` probe `consume_expected_echo` with `'\r'`/`'\n'` before rearming, to decide whether those control bytes are themselves part of the registered pattern. A restored buffer's text never contains either, so that probe is *always* a mismatch -- and with the clear living inside `consume_expected_echo` itself, every single carriage return destroyed the registration before `maybe_rearm_expected_echo` ever got a chance to run, regardless of whether the real echo characters around it matched. This is the third attempt at bounding this window, and all three are worth recording so a fourth person doesn't repeat them: 1. `LineEditorStatusEvent::Active`: wrong because it fires *before* the restore write is even flushed (measured: `pending_writes_len=1` at the moment `Active` fires, and that same handler is what flushes it), so it discarded the whole restored buffer rather than protecting it. 2. Clearing on any mismatch inside `consume_expected_echo` itself: wrong because `carriage_return()`/`linefeed()`'s own control-byte probes are guaranteed mismatches unrelated to whether the echo is actually over, destroying the pattern on every CR-driven redraw. 3. This one: `consume_expected_echo` goes back to having no side effect on a mismatch (pure, as it always should have been for a function also used as a probe); the window instead ends at `EarlyOutputHandler::input()`'s call site specifically, which only ever receives real characters, never the CR/LF control-byte probes. Traced this by hand against every one of the file's 23 tests before pushing (all 7 previously-failing ones, plus the 16 that were already passing, to make sure none regressed) -- but per instruction this is not being treated as sufficient on its own; the verifier's test-suite run is what should actually confirm it, and will also compile-check the PowerShell replacement-span plumbing from the immediately preceding commit, across all twelve files it touched. The residual described in `da66d6b`'s own test (`test_a_rearm_can_still_leak_exactly_one_coincidentally_matching_character`) is unchanged: a rearmed, already-exhausted pattern can still absorb exactly one coincidentally-matching character from unrelated output before the next, genuinely non-matching character closes the window.
…e index Two fixes, both to the PowerShell replacement-span work in 796a600: 1. E0027 build error. `IsReceivingCompletionsOutput::Yes` gained `replacement_span` in 796a600; `TerminalModel::input`'s raw-completions branch destructures the variant but wasn't updated, so it fails to compile. Decided the raw path does not need the span (rather than reflexively adding `..`): the only shell that currently emits the span, PowerShell, always uses `incrementally_typed` format, never `raw`; and this branch only accumulates raw text character by character regardless, so it has nothing to do with a span even if one were ever set here. Bound and ignored it explicitly (`replacement_span: _`) with a comment saying why, rather than a silent `..` that would hide the next field addition the same way this one was missed. Re-checked every other `IsReceivingCompletionsOutput` pattern site in the file by hand this time (not just the one the compiler found) to confirm none of the others has the same gap. 2. A whitespace-only line (measured: `" "`) makes PowerShell's `CompleteInput` report `ReplacementIndex=-1, ReplacementLength=-1` -- there's nothing to anchor a replacement to and no matches either. The client's parser correctly rejects a negative pair and falls back to the whitespace heuristic, so this degraded safely, but it did so with a warning logged on trivially reachable input (type spaces, press Tab). `pwsh.ps1` now skips the OSC entirely when `ReplacementIndex -lt 0`, verified against the exact reproduction with pwsh 7.6.5 installed in-sandbox.
PSReadLine's own redraw rewinds with absolute cursor addressing (`\x1b[1;1H`), not with any of the four motions already handled -- measured live: `push_expected_echo`, one character matched, then that exact CUP, then the reprint's first character reads as a genuine mismatch. `EarlyOutputHandler::input()`'s clearing rule (added to fix the CR/LF probe bug two commits ago) then reads that mismatch as proof the echo is over and clears the registration, so the fully-registered-but- now-cleared pattern's *entire* remaining reprint falls through to background output instead of just the one character a rewind gap would otherwise cost. Measured as a scratch unit test on the same tree: 16 of 16 characters leaked with the clearing branch as pushed, versus 1 with it disabled -- the clearing rule turned an existing one-character leak into a whole-buffer one on this specific, previously undocumented motion. Generalized the carriage-return rearm (previously `maybe_rearm_expected_echo`, hardcoded to position 0) into `rearm_at_column(column)`, on the premise already used for CUB/CUF: a registered pattern's echo always starts at column 0, so an absolute column from the escape sequence is already an absolute position in the pattern, exactly as a relative distance already maps onto a relative position shift for CUB/CUF. `carriage_return()` now calls `rearm_at_column(0)`; `goto`/`goto_col` (CUP/CHA) call it with their own column argument. The row component of `goto` is irrelevant to matching, which only tracks a linear character stream, not screen position. Added a test reproducing the measured shape (one matched character, then `goto(VisibleRow(0), 0)`, then the full reprint) and traced it against both the fixed and unfixed logic to confirm it distinguishes them. Updated the `expected_echo_positions` doc comment, which explicitly named absolute column addressing as an unhandled gap, to describe it as the fifth handled motion instead -- and to explain why PSReadLine's own redraw doesn't follow the `honor_ps1` split (relative vs. carriage return) the other three shells' redraws do: it always addresses absolutely, regardless of who owns the prompt.
`9e5ee3c` generalized the carriage-return rearm to any absolute column from CUP/CHA, on the premise that a registered pattern's echo always starts at column 0. That premise held in the byte stream actually measured (`\x1b[1;1H` immediately followed by the pattern's own first character), but it does not obviously hold in general: a carriage return means "column 0" unconditionally by construction, while an absolute column from CUP/CHA does not -- if a redraw re-renders a nonempty prompt together with the buffer, the column it addresses partway through that redraw could just as easily be the prompt's own width as the buffer's start, and nothing in the byte stream distinguishes the two. Exactly the same reasoning is why zsh's ZLE switches from a carriage return to relative motion (backspace/CUB) when the shell draws its own prompt -- absolute positioning stops lining up with the buffer once a prompt is in the way. This has not been measured either way: no session with a real, nonempty prompt has confirmed whether PSReadLine's CUP column reflects the prompt width in that case. Rather than risk seeding a candidate at a confidently wrong position (which, depending on what follows, risks absorbing genuinely unrelated output rather than just failing to protect the pattern), `goto`/`goto_col` now only rearm when the column is 0 -- column 0 is safe under either reading, since it is either genuinely the buffer's start or so early into a redrawn prompt that nothing of the pattern could plausibly be confused with prompt text there. Any other column is treated as no information rather than acted on. The test added in `9e5ee3c` (`goto(VisibleRow(0), 0)`) still exercises this path, since it uses column 0, which remains trusted. Updated both doc comments (the field-level one and `rearm_at_column` itself) to state this as the conservative choice it is, rather than as a general property that happens to match the one measured case -- and to say explicitly that widening it to arbitrary columns needs a real nonempty-prompt measurement first, not just tracing.
cobra-generated "bash completion V2" scripts (kubectl, gh, and most modern
Go CLIs) bake a padded "name (description)" string directly into a
COMPREPLY entry whenever COMP_TYPE is 9 (plain Tab) and there is more than
one match -- real readline only ever inserts an entry when it's unique, so
the padding is safe there, but _warp_native_bash_completions displays every
entry in a menu and inserts whichever one is picked, so the padded string
was being inserted verbatim.
Verified with gh's actual completion script (gh completion -s bash):
`gh pr che` under COMP_TYPE=9 yields
COMPREPLY=("checkout (Check out a pull request in git)" "checks (...)")
cobra's own case statement (__gh_handle_completion_types) strips
descriptions unconditionally under COMP_TYPE 37 (menu-complete) or 42
(insert-completions), regardless of match count -- see
spf13/cobra#1508. Switched COMP_TYPE from 9 to 37
in _warp_native_bash_completions.
Confirmed this has no effect on non-cobra completions: bash-completion
(which drives the vast majority of scripts, including git's) never reads
$COMP_TYPE at all -- grep -rn "COMP_TYPE" /usr/share/bash-completion/ finds
zero matches, and `_git`'s completion function produces byte-identical
output under COMP_TYPE 9 and 37.
Verification performed:
- End-to-end, via _warp_native_bash_completions itself (not just a
standalone simulation), against gh (cobra) and git (bash-completion),
confirming: gh now emits bare "checkout"/"checks" instead of the padded
strings, and git's output is unchanged.
- Added app/assets/bundled/bootstrap/bash_native_completions_test.sh, a
self-contained regression test (no gh/git dependency) covering both
shapes the review asked for: a synthetic cobra-style completion function
(padded under COMP_TYPE 9, bare under 37/42) and a synthetic ordinary
bash-completion-style function (ignores COMP_TYPE entirely). Confirmed
it fails against the pre-fix COMP_TYPE=9 behavior and passes against the
fix.
- This script is not wired into any existing CI job -- the repo has no
shell-script-level test harness for the bootstrap scripts (only Rust
unit tests, which can't exercise bash's own completion machinery). Noted
as a known limitation; run manually via
`bash app/assets/bundled/bootstrap/bash_native_completions_test.sh`.
- Not run through cargo build/test/clippy (no Rust changes in this commit);
./script/format made no changes (it doesn't format shell scripts).
…po; harden zsh's analogous guard
Three fish defects reported from live testing, plus a proactive zsh fix
based on the same root cause as the first:
1. warp_run_generator_command_native_completions's empty-buffer guard
(`test -n "$line"`) didn't cover whitespace-only lines. Measured: a
single space enumerated $PATH (990+ matches, tens of KB) synchronously
in the user's shell via `complete -C " "`. Fixed by trimming before the
check (`test -n "$(string trim -- "$line")"`).
Verified empirically with a real fish 3.7.0 session: pre-fix, a single
space produced 1007 "9280;C" matches (51KB); post-fix, zero.
2. warp_preexec's generator-kill loop had a typo predating this PR: the
loop variable is `pid` but the kill command used `$pids` (undefined,
expands to nothing), so `kill -9 $pids` never killed anything. This was
previously dormant because of a separate negation bug (`test (! ...)`)
this PR's earlier fix corrected, making the typo's effect live for the
first time. Fixed `$pids` -> `$pid`.
Verified empirically: with the pre-fix script, a background job tracked
in `_warp_generator_pids` survived warp_preexec's kill loop (`jobs`
still showed it running afterward); with the fix, the same job is
killed (SIGKILL) as soon as warp_preexec fires for a real command.
3. Large result sets producing no menu at all is still under investigation
by the orchestrator (bisecting the exact match-count threshold); not
addressed in this commit pending that data.
Additionally, applied the same whitespace-trim guard to zsh's
warp_run_generator_command_foreground_completions, which had the identical
gap (`[[ -z $line ]]` doesn't catch whitespace-only). This is the same
class of bug as (1): completing a blank command-position word triggers a
full command enumeration in every shell's completion engine we've checked
so far (fish's `complete -C`, and this is standard zsh Tab-on-blank-buffer
behavior too). Bash and PowerShell were also checked and already handle
this correctly:
- bash: `_warp_native_bash_completions` already returns early because
a whitespace-only line's derived `$cmd` is empty
(`[[ -z "$cmd" ]] && return`). Verified: 0 bytes of output for " ".
- PowerShell: `CompleteInput` on a whitespace-only line already returns
0 matches and a negative ReplacementIndex/Length (this exact case is
already documented in pwsh.ps1's comment). Verified with pwsh 7.6.5.
The zsh fix could not be empirically confirmed to reproduce end-to-end in
this sandbox: the completion widget's output is emitted mid-`zle` (via the
`compadd`-override shim, itself invoked through `list-choices`), which
writes directly to the controlling terminal rather than through the
calling shell's redirected stdout, and piping/subshelling the call (to
capture output another way) disables ZLE entirely per the code's own
documented `USEZLE`-clearing behavior -- so this fix is applied
defensively, on the strength of the same root-cause reasoning verified for
fish, rather than an empirically reproduced zsh-specific dump. Flagging
this as a known verification gap for the reviewer.
Not run through cargo build/test/clippy (no Rust changes in this commit);
./script/format made no changes (it doesn't format shell scripts).
…ontext
Two independent zsh native-completions fixes:
1. Report the replacement span (OSC 9280;S), extending the mechanism already
built for PowerShell to zsh.
Root cause: without a shell-reported span, input.rs falls back to a
whitespace-derived guess at the typed token, then filters candidates by
requiring them to start with that guess. This silently discards every
completion that only replaces a sub-token of the current word -- any path
after a literal `/` (`cd /et` -> candidate `etc` doesn't start with the
guessed token `/et`), a `$`/`${` parameter sigil (`echo $HOM` -> candidate
`HOME` doesn't start with `$HOM`), or similar.
Fix: in the compadd override shim, report `start = ${#line} - ${#PREFIX}`,
`length = ${#PREFIX}` before each match batch. This holds regardless of
how $PREFIX arose (bare word, following an IPREFIX like `/` or `${`,
inside an open quote, with a literal un-expanded `~`, or with a literal
backslash-escape) because $PREFIX always carries the exact characters
typed for the segment being completed, not an expanded or dequoted form.
Verified end-to-end against a real zsh session (compinit -C, PTY capture
of the actual OSC output) across 8 shapes: `cd /et` (S;3,3), `cd /usr/lo`
(S;3,7), `echo $HOM` (S;6,3), `echo ${HO` (S;7,2), `foo=/tmp/` (S;4,5),
`cd foo\ ba` (S;3,7, literal backslash), `cd "foo ba` (S;4,6, open quote,
excluding the quote char), and `cd ~/Doc` (S;3,5, literal un-expanded
`~`). Every case's span, once applied, reproduces exactly the typed
prefix as a substring of the line, and the client's own filter (verified
separately) accepts the real candidates it was previously discarding.
Checked whether bash and fish share this problem: they don't, and this is
now empirically confirmed rather than assumed. Both `_warp_native_bash_completions`
and fish's `complete -C` return whole-word replacements as their
candidates (e.g. `cd /et` -> bash emits `/etc`, fish emits `/etc/`), which
already start with the client's whitespace-derived guess and so survive
the existing filter with no span needed.
2. Fix zsh's `zstyle` context pattern, which never matched anything.
Root cause: the `list-grouped`/`insert-tab`/`verbose`/`list-separator`
styles were registered against
`':completion:warp_complete_via_compadd_override:*'` -- the name of the
registered *widget*. But this completion runs from inside the
`zle-line-init` hook (see the comment on
`_warp_native_completions_zle_line_init`), so $curcontext's leading
component is always literally `zle-line-init` regardless of which shell
command is being completed (measured, e.g.
`zle-line-init:complete:atuin:argument-1`) -- never the widget name. So
none of these styles were ever actually applied to any real completion
request. In particular, `verbose` being unset meant `_describe`-driven
completions (the mechanism `clap_complete`'s zsh generator, and many
other completion functions, use for subcommand/flag descriptions) never
populated real per-item descriptions -- `compdescribe`'s non-verbose mode
leaves the description array empty, and our own compadd shim's `-d`
detection was already working correctly; there was simply nothing in
the description array for it to find.
Fixed by matching `':completion:zle-line-init:*'` instead.
Verified end-to-end with a real `atuin` binary (downloaded its actual
release binary, generated its real `clap_complete`-produced zsh
completion script, sourced it in a PTY-driven zsh session): before the
fix, `atuin ` returned 32 matches with every description empty; after,
the real per-subcommand descriptions come through (`setup` ->
"Setup Atuin features", `history` -> "Manipulate shell history", etc.).
The three subcommands that still show an empty description (`wrapped`,
`config`, `contributors`) genuinely have no description in atuin's own
`--help` output either, confirming this isn't a remaining gap. Also
verified `atuin --h` (an `_arguments`-driven flag, not `_describe`) now
correctly returns `--help` with description "Print help", confirming the
fix isn't limited to the `_describe` code path.
Both fixes were tested together in the same session against the real
zsh_body.sh (not a scratch copy) to confirm the zstyle change doesn't
regress the span reporting or vice versa.
Not run through cargo build/test/clippy (no Rust changes in this commit).
./script/format made no changes (it doesn't format shell scripts).
`ls --col` under native completions inserted `--color` (plus a space,
offering `always` as a ghost), while a real interactive Tab inserts
`ls --color=` followed by the value list -- accepting `always` then landed
as a separate positional argument instead of the option's value, changing
the command's meaning.
Root cause: compadd's `-S suf` gives the string it adds after every match
and is meant to be inserted (e.g. `_arguments`'s
`'--color=-(never auto always)'` spec passes `-S '='` so the option and its
value join correctly). The `compadd` override shim already parses this into
`$asuf` via `zparseopts` (confirmed correct: `${(v)asuf}` holds `=` for
this exact case), but the match-display loop only ever appended the
directory suffix (`$dsuf`, from `-f`) -- `$asuf` was extracted and then
silently never used. Appended it to each match.
`$hsuf` (from `-s`) is `-S`'s display-only counterpart -- a "hint" shown but
never inserted, the same relationship `-p`/`$hpre` has to `-P`/`$apre` --
so it's deliberately left out of the inserted text.
Verified with a real zsh session (compinit -C, raw PTY capture of the OSC
output): `ls --col` now emits `9280;C;--color=` (was `9280;C;--color`).
Regression-checked `git ch` (8 matches with descriptions, unaffected) and
`cd /et` (`etc`, unaffected) to confirm ordinary completions with no `-S`
suffix are unchanged.
Not run through cargo build/test/clippy (no Rust changes in this commit).
./script/format made no changes (it doesn't format shell scripts).
…und block Confirmed on Linux zsh: once a restored buffer is longer than the terminal width minus the prompt's own width, ZLE's redraw clears the remainder of the first display row with literal space characters (not an erase-to-end-of-line escape) before moving to the next row with CUD. Every byte from the wrap point on leaked into a background block -- exactly `total - (columns - prompt_width)` bytes in each measured case (125/205/55 character buffers at two terminal widths). Root cause: the first space of that fill doesn't match the buffer's own text at that position, so it was treated as the first real mismatch `EarlyOutputHandler::input()` uses to end the expected-echo window (per existing, documented behavior -- this is also what makes a stale registration eventually stop absorbing unrelated output). Once the window closes, everything that follows leaks, even though the rest of the redraw (the second row's real characters) does go on to match the buffer. Fix: a space character no longer ends the window on its own, as long as at least one live candidate still expects more of the registered pattern (`awaiting_more_expected_echo`) -- narrower than "the window is open at all," so a stray space after the pattern is already fully matched still renders normally, only a space received mid-redraw is treated as wrap padding. `move_up`/`move_down` (CUU/CUD) already didn't touch the window either way (pure screen-position detail of a redraw already known to be in progress), so no change was needed there. Added `test_push_expected_echo_survives_wrapped_line_space_fill_at_the_wrap_boundary`, reproducing the measured byte shape (first-row characters, space-fill, CUD, second-row characters) and asserting it doesn't leak; and `test_a_stray_space_after_a_full_match_still_leaks_normally`, confirming the narrower scoping doesn't swallow unrelated output that happens to start with a space once the pattern is already fully matched. Not verified against a live build in this sandbox (cargo check -p warp reliably OOMs here, as in every prior round of this PR) -- reviewed by hand against the exact measured byte sequence and the existing test patterns in this file, but requesting the verifier's usual full build/test pass to confirm both new tests (and the existing 24) pass. Separately: a second, related-looking finding from the same verification round -- committed block headers and tab titles getting corrupted (truncated and fused with command text) immediately after a completions request -- was NOT fixed in this commit. My hypothesis, not yet confirmed: `consume_expected_echo`'s swallow (this fix's new space case, and the pre-existing CR/backspace/CUB/CUF cases) never advances any grid's own cursor-position tracking for the swallowed characters, since they never reach `block.input(c)` at all. If the same physical redraw is split across this swallow and a later grid that does receive characters (e.g. once the real active block starts), that grid's cursor tracking would be behind by exactly the swallowed count, which is consistent with a fused/truncated header. I have not verified this against the actual grid/cursor code, and a real fix would need to advance cursor state without rendering visible content, which is a larger change than I want to make speculatively -- flagging this for the verifier to check whether the corruption's byte offset matches the swallowed count, which would confirm or rule this out.
… byte offsets
P0: typing e.g. "echo <accented/CJK/emoji char> Get-Ch" and pressing Tab
crashed the app (panic: "byte index N is not a char boundary"), twice
reported live -- the first crash auto-recovered into a fresh session
(losing the window/tab), the second took the window down entirely.
Root cause: `CommandCompletion.ReplacementIndex`/`ReplacementLength` are
.NET string offsets, i.e. UTF-16 code units, but pwsh.ps1 sent them as-is
over the OSC 9280;S wire message, and the client uses them as UTF-8 byte
offsets directly into `buffer_text` (`Span::slice`, `&source[start..end]`).
Any multi-byte character before the completed token shifts the two
countings apart and can land the byte index mid-character. Measured:
"echo 中 Get-Ch" is 13 UTF-16 units but 15 UTF-8 bytes; ReplacementIndex 7
(correct in UTF-16 units) falls inside "中"'s 3-byte UTF-8 encoding
(bytes 5..8) when misread as a byte offset. This was a known, named gap
in the original PowerShell span commit ("only exact for ASCII lines"),
not a new regression -- it just hadn't been exercised with non-ASCII input
until now. The pre-span whitespace-derived fallback never had this problem,
since it always derived offsets from the buffer itself.
Two fixes, at two different layers:
1. Convert at the source (pwsh.ps1), where the exact string is known:
compute the UTF-8 byte length of `$line.Substring(0, ReplacementIndex)`
and of the substring through the end of the replacement range via
`[System.Text.Encoding]::UTF8.GetByteCount`, and send the difference as
the byte-based start/length instead of the raw UTF-16 offsets. Verified
with the installed pwsh for both a CJK character (3-byte UTF-8, 1
UTF-16 unit) and an emoji (4-byte UTF-8, a UTF-16 surrogate pair, 2
units) before the token: the converted byte range, sliced by hand
against the actual UTF-8 bytes, yields exactly "Get-Ch" in both cases.
This keeps the wire protocol uniformly byte-offset-based, matching what
zsh/bash/fish already send, with no client-side plumbing changes.
2. Defense in depth: `Span::slice` itself can no longer panic on any input,
by clamping both offsets to the nearest valid char boundary at or below
them (and to the string's bounds). This covers any other span whose
units turn out to be wrong for a reason not yet found -- a wrong menu
is recoverable, a panicked window is not. Added `floor_char_boundary`
(a dependency-free equivalent of the standard library's still-unstable
`str::floor_char_boundary`) and five new tests in `meta_tests.rs`
covering the exact crashing shape, out-of-bounds offsets in both
directions, and an end-before-start span.
Verified: `cargo check -p warp_completer` and `cargo test -p warp_completer
--lib meta` both pass cleanly (6/6, including the 5 new tests) -- this
crate builds within this sandbox's memory limit, unlike the full `warp`
app crate. The `.ps1` change itself can't be exercised through that same
build, so it was verified directly against the installed pwsh (7.6.5) by
reproducing the exact byte-offset arithmetic from the two reported crash
lines and confirming the corrected slice.
The bash matrix verified the cobra fix works (bare names, gh/kubectl correct) but found it regressed `make`: bash-completion's own `make` completion script -- the one script out of 841 in a stock install that actually reads $COMP_TYPE -- branches on it to choose between a full directory-prefixed path (COMP_TYPE 9) and just the next path component (any other value). Under 37, `make sub/dir/` + Tab returned `deploy` instead of `sub/dir/deploy`; since the bare component no longer contains the typed prefix, the client's own filter discarded it and no menu appeared at all -- silently dead, one level into any prefixed target. COMP_TYPE 9 is faithful to real readline for all 841 stock scripts (grep confirms nothing else reads $COMP_TYPE); 37 was only faithful for 840. Reverted to 9. To still avoid cobra's own padded "name (description)" entries (the original problem 9 causes for cobra-generated scripts), the padding is now split apart *after* the completion function returns, rather than avoided by changing COMP_TYPE: a name, two or more spaces, then a parenthesised description running to the very end of the entry, is split into a bare name plus a real description -- something neither COMP_TYPE value alone provided, since bash previously had no description channel at all. Anchored narrowly (both the multi-space run and the trailing, unescaped `)$` must be present) so a legitimate candidate that happens to contain parentheses elsewhere passes through unchanged. Found and fixed while implementing: the first version of the split regex used `(.+)` for the name group, which is greedy and matches `.` against spaces too -- with more than 2 padding spaces (cobra pads shorter names further to align columns), it silently absorbed some of the padding into the "name" instead of leaving all of it for the space-quantifier group. Fixed by requiring the name group to end in a non-space character (`(.*[^[:space:]])`), which cannot happen if trailing spaces are absorbed. Caught this with a real `gh` completion (`checks` padded with more spaces than `checkout`, to align both to the longest entry's width) before it reached committed tests. Verified against real gh, git, and a make-shaped Makefile in this sandbox: - `gh pr che` -> `checkout` (description "Check out a pull request in git") and `checks` (description "Show CI status for a single pull request"), both clean, no trailing whitespace. - `git ch` -> unaffected (`checkout`, `cherry-pick`, `cherry`, no descriptions -- bash-completion never reads $COMP_TYPE). - `make sub/`, `make sub/dir/`, `make build/` against a real Makefile with nested targets -> full relative paths (`sub/dir/`, `sub/dir/deploy`, `build/artifact`), matching real interactive Tab. Updated `bash_native_completions_test.sh`: the cobra fixture now also asserts the recovered description text (not just that the name is bare), and a new `make`-shaped fixture (a synthetic COMP_TYPE-branching compspec returning a directory-prefixed path under 9 and a bare component otherwise) locks in the directory-prefix behavior this revert restores. Also wired the script into `script/presubmit` (`bash bash_native_completions_test.sh`, alongside the existing PSScriptAnalyzer step for the same reason) per the reviewer's point that a bootstrap script with no test harness is exactly how a COMP_TYPE regression ships unnoticed -- this was previously a manually-run script that nothing required anyone to remember. Verified: `bash app/assets/bundled/bootstrap/bash_native_completions_test.sh` passes 4/4 (2 new assertions plus the make case). `bash -n` on bash_body.sh, the test script, and script/presubmit all pass.
Same class of bug as the PowerShell UTF-16 crash, caught before it could
crash anything (Span::slice's new defensive clamp would have absorbed
it), but still wrong: zsh's `${#var}` counts *characters*, while the OSC
9280;S wire format is byte offsets (matching the client's own buffer
indexing and every other shell's span). Measured: any accented or CJK
character before the completed token shifts the reported start left by
exactly the extra UTF-8 byte count --
ls /tmp/plain/xy -> reported 14, correct byte start 14 (delta 0, ASCII)
ls /tmp/café/xy -> reported 13, correct byte start 14 (delta 1)
ls /tmp/日本/ni -> reported 11, correct byte start 15 (delta 4)
echo café/xy -> reported 10, correct byte start 11 (delta 1)
ASCII lines are unaffected, which is why ordinary testing missed it.
Fixed with `local LC_ALL=C` before the span computation, which makes
zsh's `${#...}` count bytes instead of characters (confirmed
empirically). Scoped via `local` to the rest of this `compadd()` call --
deliberately not narrower, since nothing later in the function does
character counting that C locale would change: `$#__hits`/`$#__dscr`/
`${#dirsuf}` are array element counts (locale-independent), and the
description prefix-strip (`##$__hits[$i] #`) is a literal byte-for-byte
match.
Verified against the real reported shapes (real files under `café/` and
`日本/` directories, a real zsh session, raw PTY capture of the OSC
output): `ls /tmp/utf8test/café/xy` now reports `S;3,22`, and slicing the
line's actual UTF-8 bytes at `[3:25]` (3 + 22 = 25, the line's real byte
length) decodes cleanly to `/tmp/utf8test/café/xy` -- not a character
count, and not a boundary violation. Same check for the CJK case
(`S;3,23`, slice `[3:26]` decodes to `/tmp/utf8test/日本/ni`) and the
plain ASCII control case (byte and character counts coincide, so it was
already correct and stays correct).
$PREFIX did not need separate treatment: it's already correctly counted
in bytes by the same `LC_ALL=C` setting, since both `${#_WARP_NATIVE_COMPLETIONS_LINE}`
and `${#PREFIX}` are evaluated under it.
Not run through cargo build/test/clippy (no Rust changes in this commit).
./script/format made no changes (it doesn't format shell scripts).
`terminal.input.honor_ps1 = true` leaked the hex-encoded restore text
(e.g. "4765742d4368696c") into a background block on every completions
request, persisting across Ctrl+U and unrelated commands. Absent
entirely with honor_ps1 = false.
Root cause: PSReadLine's redraw always rewinds with absolute cursor
addressing (CUP), but the previous rule only trusted a CUP to column 0,
deliberately conservative pending a real-prompt measurement (a redraw
that re-renders a nonempty prompt together with the buffer could in
principle address a column reflecting the prompt's own width rather than
the buffer's start). That measurement now exists: with a real 29-column
prompt, every redraw is `\x1b[2;30H` (column 30, i.e. column 29 in this
codebase's 0-based columns) -- exactly the case the conservative rule
declined to trust, so the conservative rule converted a hypothetical
wrong-position risk into a guaranteed leak whenever the prompt is
non-empty.
The same measurement gives the fix, and it's better than threading a
prompt width through: PSReadLine always re-renders the *entire* buffer
from its own start, so a CUP unconditionally means "the buffer's own
position 0" regardless of which screen column it lands on -- column 1
with a zero-width prompt and column 30 with a 29-wide one were both
measured to be the buffer's start. (An "empty" prompt isn't column 0
either -- PowerShell substitutes its own `PS>` fallback, measured at
column 8 -- reinforcing that the column itself carries no information
worth gating on for this line editor.) zsh, the other line editor this
matters for, was separately measured to never emit absolute cursor
addressing at all for this restore, so widening the rule has no effect
on it.
Fixed by having `goto`/`goto_col` (CUP/CHA) always rearm position 0,
rather than only when the reported column is 0.
Also addressed, from the same measurement: the echo is *cumulative*, not
one-shot -- a 12-character buffer was measured to re-echo as an
increasingly long prefix across 12 separate redraws ("1", "12", "123",
... rather than the full buffer once), each preceded by its own CUP. A
single seed wouldn't be enough; this already works correctly since
`goto`/`goto_col` call `rearm_at_column` unconditionally on every CUP
occurrence, not just the first -- confirmed with a new test simulating
exactly this shape.
Added two tests: one reproducing the measured nonzero-column case (CUP
to column 29, matching the real 29-column prompt) and confirming it no
longer leaks; one reproducing the cumulative multi-redraw echo shape
across 7 redraws of increasing length, each preceded by its own CUP.
Not verified against a live build in this sandbox (cargo check -p warp
reliably OOMs here) -- reviewed by hand against the exact measured
byte/column values and the file's existing test patterns, requesting the
verifier's usual full build/test pass (including the app-level repro:
honor_ps1 = true with a real prompt, no leaked hex on repeated requests).
OSC 9280's C (match) and D?description params are semicolon-delimited,
and the client only reads the third param, so a literal `;` inside a
match or description (e.g. a filename like semi;colon.txt, or a .NET
tooltip like "int Count { get; }") silently truncated everything after
it -- corrupting insertion, not just display. A BEL or ESC byte in the
same text would end the whole OSC sequence outright, before the
completions-parsing code even runs.
Fix the whole class at once by hex-encoding both fields in all four
shells, reusing each shell's existing warp_hex_encode_string /
Warp-Encode-HexString helper (already used for JSON hook payloads and
the in-progress buffer text):
- zsh: compadd shim's match/description display loop
- bash: _warp_native_bash_completions's COMPREPLY reply loop
- fish: warp_run_generator_command_native_completions's complete -C loop
- PowerShell: the Alt+3 handler's CompletionMatches loop
Client-side, add decode_hex_completions_payload (app/src/terminal/model/ansi/mod.rs)
to decode and validate the hex payload, degrading gracefully (skip the
match, or treat as no description) on a missing/malformed/non-UTF-8
payload rather than surfacing a wrong string. The S (replacement span)
OSC is untouched -- it's just two decimal numbers, no text content.
Verified:
- New Rust unit tests for decode_hex_completions_payload covering `;`,
BEL, ESC, multibyte text, and missing/malformed hex (both mod_tests.rs
and OSC-dispatch-level integration tests). cargo fmt clean; the
full `warp`/`app` crate can't be compiled in this sandbox (reliably
OOMs), so these are hand-reviewed against the exact `Params` type
(`&[&[u8]]`) rather than compiler-verified.
- Extended bash_native_completions_test.sh with a semicolon-containing
match, and updated its OSC-payload collection helper to hex-decode
before comparing (all 5 cases pass).
- Empirically verified each shell's actual emission code (copied
verbatim from each script) against real zsh/bash/fish/pwsh
interpreters: a match/description containing `;`, BEL, or ESC now
arrives on the wire as pure hex digits and decodes back to the exact
original text.
- Span::slice's doc comment attributed the UTF-16-to-byte-offset conversion to "the client-side conversion at the OSC boundary", but the conversion happens shell-side, in pwsh.ps1. Corrected so a future units bug isn't chased in the wrong layer. - pwsh.ps1's replacement-span computation could throw if a shell ever reported ReplacementIndex + ReplacementLength past the end of the line (Substring would throw, and the surrounding try/catch would turn that into a silent, warning-free empty completions response). Not reproducible with any real CommandCompletion input tried, but clamped defensively anyway -- costs nothing, and a silent empty response is a worse failure mode than a slightly-wrong span. Verified: cargo fmt clean; `cargo test -p warp_completer --lib meta` (6/6, unaffected by the doc-only change); a standalone pwsh script confirmed the clamp no longer throws for an out-of-range ReplacementIndex+ReplacementLength while leaving the normal in-range case's computed span unchanged.
Verifier proved by execution (Tab+Enter) that native completions inserted
nothing for any multi-component path (cd /et, ls /tmp/plain/xy, cat
~/.zsh, ls /workspace/warp/cra), despite real zsh having matches.
Root cause, confirmed empirically by instrumenting the real compadd shim
in a live zsh session: _path_files (the completion function behind path
arguments for cd/ls/cat/etc.) restores $PREFIX to the *entire* remaining
path before calling compadd (e.g. "/tmp/zsh_path_test/et" for
`cd /tmp/zsh_path_test/et`, not just "et"), and reports the directory
portion separately via compadd's `-p` flag (parsed into $hpre) purely for
*display* -- the real match strings are bare basenames ("etc"). The
existing replacement-span computation used the whole $PREFIX, reporting
a span the actual match text ("etc") never starts with. The client's own
filter then discarded every candidate, silently doing nothing on Tab.
Fixed by stripping $hpre (and $apre, from -P, handled the same way but
not directly reproduced) from the front of $PREFIX when it's a genuine
prefix, before computing the span -- matching what _path_files itself
excludes from insertion.
Verified against a real zsh 5.9 session (compinit-initialized, driving
the actual `warp_run_generator_command_foreground_completions` entry
point end to end, not just static reasoning):
- `cd /tmp/zsh_path_test/et` (2-level nested path): span corrected from
(3,21) [covering the whole path, which "etc"/"etcetera" don't start
with] to (22,2) [covering just "et"] -- matches now pass the client's
filter.
- `cd /et` (real /etc): span (4,2), one match "etc" -- matches the
verifier's exact repro.
- `ls /tmp/plain/xy` (two matches xyz/xyzzy): span (14,2), both pass.
- `ls --color=` (value completion after the -S suffix fix): span
(11,0), matches never/always/auto -- already correct before this fix
(PREFIX was already empty there), confirming the reported "no menu"
for this case is not a shell-side bug; flagging back to the
orchestrator as likely client-side.
- Regression-checked `echo $HOM` (sub-token case unaffected by hpre,
since it's empty there): unchanged, span (6,3), still matches "HOME".
`zsh -n` syntax check passes. Not independently re-verified in-app
(no computer-use in this environment); requesting the verifier's zsh
matrix be re-run on this commit.
…rip control chars from accepted completions Fixes four issues found by verification of the hex-encoding commit (f852ae0): 1. BLOCKING: bash's and fish's warp_hex_encode_string piped through `echo`, which treats an argument that looks like one of its own flags (-n, -e, -E) as that flag instead of literal text. Measured: kubectl -, ssh -, and npm - all offer exactly such a candidate as a real completion, so `-n` encoded to nothing and `-e`/`-E` encoded to just echo's own trailing newline. 2. The same `echo` appended a trailing newline to every payload (latent until now). 3. Forking two processes per hex-encode call (echo|od|tr) cost ~4x on a large completions result set (measured: 8.4s -> ~2s for 3656 matches). Fixed bash's encoder with a pure-bash, no-fork loop using `printf -v` byte by byte (LC_ALL=C for byte-safe indexing), which fixes all three at once. Fixed fish's encoder by switching `echo` to `printf '%s'` (fish's echo has the same flag-swallowing bug). 4. The cobra description-splitter regex greedily backtracked to the last multi-space run in an entry, so a description that itself contained a parenthesised, multi-space-padded aside folded part of the description into the name. Separately, the regex alone couldn't distinguish a genuine cobra-padded reply from a real candidate that merely looks padded (e.g. a file named "Backup (copy)"), splitting the latter too. Fixed the regex to structurally stop at the first multi-space run instead of the last, and added a structural guard: cobra only pads a reply with more than one match and pads every entry in such a reply, so the split is only applied when there are 2+ non-empty COMPREPLY entries and all of them match the padded shape -- never for a lone candidate that merely looks padded. Also, client-side: an accepted completion match/prefix can contain a literal control character (e.g. a real but pathological filename with an embedded newline), which previously got inserted verbatim -- a newline in particular puts the input editor into a stuck multi-row state (Ctrl+U only clears the current row) and silently disables native shell completions for the rest of that buffer (should_use_native_shell_completions bails on a multiline buffer). Added strip_control_characters (input.rs), applied to both completion-acceptance insertion paths, with unit tests per control-character class (newline, CR, BEL, ESC, tab, DEL, a C1 control), distinct from a deliberate multi-line paste, which the editor continues to support unmodified. Verified: - bash_native_completions_test.sh: 10/10 passing, including new fixtures for the exact -n case, an exact-byte assertion on the encoder itself, a nested-paren cobra description, and a lone real "Backup (copy)"- shaped candidate that must not be split. - Directly benchmarked the old vs. new bash encoder at the reported 3656-match scale (real timing, not estimated): ~4.47s -> ~0.19s for the encoder alone; ~1.99s for the full completions pipeline at the same scale (down from the originally reported 8.4s). - Fish's fixed encoder verified directly against -n/-e/-E/multibyte cases with a real fish 3.7.0 interpreter. - `bash -n`/`fish -n` syntax checks pass; `cargo fmt` clean on input.rs. input.rs's changes are hand-reviewed, not compiler-verified (the full `app` crate reliably OOMs in this sandbox, as in every prior Rust change in this PR) -- carefully checked against Cow<str> borrow/move semantics inside the FnMut closures it's used in.
zsh's `ls --col` + Tab correctly completes to `ls --color=` (the -S suffix fix from an earlier round), but insert_completion_result_into_editor unconditionally appends a trailing space unless the completion ends with a path separator. `--color=` doesn't end with a slash, so the editor appended a space after it, leaving the cursor one character past where a hand-typed value should go -- typing "always" next produced "ls --color= always" instead of "ls --color=always". Extend the no-trailing-space exemption to also cover a completion ending in '=', matching the shell-side convention this exact case follows (an '=' suffix means a value goes directly after it, with no space). This is shell-agnostic and client-side, so it applies uniformly regardless of which shell produced the '='-suffixed completion. Found during zsh/Linux in-app verification: `ls --col` + Tab (completing to `--color=` via the -S suffix) worked, and typing `=` by hand then Tab (retriggering completions) produced a correct value menu -- but accepting the first completion left the cursor past a trailing space, so hand-typing the value produced the extra space. Confirmed as a client-side cursor/insertion issue, not a shell-side one; the shell's own OSC output (span, suffix) was already correct. Verified: `cargo fmt` clean on input.rs. Not compiler-verified -- the full `app` crate reliably OOMs in this sandbox, as with every prior Rust change in this PR -- hand-reviewed against the existing, identical `ends_with` condition pattern immediately above it.
…uiltin fish's warp_hex_encode_string piped through `od -An -v -tx1 | command tr -d ' \n'` -- two external process forks per call. Measured to cost ~1ms per candidate on a large completions result set (e.g. `git checkout ` with thousands of branches, each match and description encoded separately), 7.2s in the shell for one such request, of which complete -C itself is only 0.137s -- the rest is almost entirely these forks. Unlike bash, fish has no byte-safe equivalent of `LC_ALL=C` string indexing (fish strings are Unicode codepoints internally, not raw bytes), so a true no-fork encoder isn't available here the way it was for bash's rewrite. But `od`'s own output only needs its spaces and line-wrap newlines stripped, which fish's builtin `string replace` can do without forking `tr`. This halves the per-call fork count (1 fork instead of 2), without touching od itself (still needed to get raw UTF-8 bytes out of a fish string). Verified: round-trips correctly for -n/-e/-E (the previously-broken flag-swallowing cases), multibyte (café), semicolon, embedded newline, short and multi-line (>16 byte, wrapping od's line width) inputs, and empty string, all against the real installed fish 3.7.0 interpreter. `fish -n` syntax check passes. This is a partial mitigation, not a full fix -- forking od at all is still real cost at scale, and does not by itself make as-you-type completions viable for large result sets in fish. Investigating the separate as-you-type request-stream issue (reported alongside this) is higher priority and not yet resolved.
open_completion_suggestions gates every as-you-type dispatch on Block::is_command_grid_active() (state == BlockState::BeforeExecution), which exists to avoid firing completions while a long-running user command occupies the foreground -- but a native-shell-completions generator command is itself exactly such a foreground command. The gate goes false the moment that generator command starts and stays false until it finishes, and since the check only ever runs inside a keystroke's own edit-event handler, no *new* keystroke means no recheck. Typing faster than a request's round trip means every keystroke after the first lands while the gate is closed and silently no-ops -- no request ever fires for the buffer the user is left looking at, even once the shell goes idle again moments later. This also starves handle_completion_suggestions_results's staleness guard of a replacement request: a stale result is correctly dropped, but nothing was ever going to retry for the buffer that superseded it. Confirmed by a verifier instrumenting the gate directly: slow typing (2s/character) got a request per keystroke; a fast burst of the same 10 characters got exactly one request (for the third character) and seven consecutive blocked checks, with no request ever issued for the final buffer. The gate also blocks a keystroke landing mid-burst during otherwise-slow typing, not only a fast burst's last character -- so the fix needs to fire on every return to idle, not just once typing appears to have stopped. Fix, trailing-edge: track the editor snapshot the last as-you-type native-completions dispatch was computed from (native_completions_as_you_type_dispatch_snapshot). On every AnsiHandlerEvent::Precmd (i.e. every time the active block returns to idle, for any reason -- a real command finishing or a completions generator command finishing), check whether the current buffer differs from that snapshot, and if so, dispatch once more via open_completion_suggestions for whatever the buffer is now. Precmd is the same hook that flips is_command_grid_active() back to true. The loop-guard invariant -- comparing against the last *dispatched* snapshot, not just checking whether results are pending -- means a successful retry updates the tracked snapshot to the buffer it was just dispatched for, so the very next Precmd sees no difference and does nothing unless the user typed more since. This also naturally coalesces a fast burst into one trailing request for the final state, rather than a queue of stale ones. Extracted the actual decision (does the buffer differ from the last dispatch) into a small generic function, should_retry_as_you_type_ completions, specifically so it can be unit tested without a live EditorSnapshot (which has no public constructor outside its own module) -- including the exact loop-guard case (unchanged buffer must not retry). Verified: cargo fmt clean on input.rs. The core decision logic (should_retry_as_you_type_completions) was additionally verified via a standalone rustc compile outside this sandbox's memory-constrained app crate, confirming all three cases (no prior dispatch, unchanged buffer, changed buffer) behave as intended. The rest of the change (wiring into the existing model_events subscription, the new field, and the dispatch-time tracking in run_completions_async) is hand-reviewed against the exact existing patterns it mirrors (the sibling TerminalModeSwapped subscription arm, and the existing editor.read(ctx, |view, ctx| view.snapshot_model(ctx)) idiom used elsewhere in this same function) rather than compiler-verified -- the full app crate reliably OOMs in this sandbox, as with every prior Rust change in this PR. Not verified live (no computer-use in this environment); requesting the verifier who instrumented the gate re-run both the fast-burst and mid-burst-during-slow-typing experiments against this commit to confirm the blocked keystrokes stop disappearing.
…shot) The trailing-edge retry (previous commit) compared whole EditorSnapshot values to decide whether the buffer had moved on since the last dispatch. Verified live: of 23 retries in one session, only 5 had an actually-changed buffer -- the other 18 fired with identical text and identical selections and dispatched a redundant, duplicate request for the buffer that had just been dispatched. EditorSnapshot's own derived PartialEq compares more than text and selections (its internal buffer_text_runs), and a completions round trip perturbs that without changing anything visible -- so two snapshots that looked the same to the user still compared unequal. Severity was bounded (fires once per dispatch, the following Precmd is correctly guarded) rather than an infinite loop, but it silently doubled the shell work for roughly every as-you-type completion: an extra full foreground generator command each time, cheap on a small result set, not cheap on a large one. Fix: introduced AsYouTypeCompletionsBufferState, holding only the two fields that actually matter (text, selections), built from EditorSnapshot's own accessors. The retry guard now compares that narrower type instead of the whole EditorSnapshot, so a round trip's internal perturbation of buffer_text_runs no longer produces a false "changed" result. should_retry_as_you_type_completions itself is unchanged -- generic and still correct -- the fix is entirely in what gets passed to it. Also fixed the test gap that let this through: every existing test for should_retry_as_you_type_completions instantiated the generic with String, so none of them ever exercised EditorSnapshot's real equality behavior. Added as_you_type_completions_buffer_state_tests, testing the exact AsYouTypeCompletionsBufferState type used in production (constructed directly, with real string_offset::CharOffset/vec1::Vec1 values, matching this file's own existing use of Vec1::new elsewhere) -- including the case that would have caught the regression: identical text and cursor position must not trigger a retry. Verified: cargo fmt clean on input.rs. The new tests were manually traced against AsYouTypeCompletionsBufferState's derived PartialEq (structural equality over `text: String` and `selections: Vec1<Range<CharOffset>>`, both already known to be correctly Eq/PartialEq-comparable from their own crates) rather than compiled and run -- the full app crate reliably OOMs in this sandbox, as with every prior Rust change in this PR. Not verified live; requesting the verifier who found this re-run their session-long instrumentation (the text_differs/sel_differs logging) against this commit to confirm the 18 redundant-request cases are gone while the 5 genuine ones and the loop guard both still hold.
…cation A verifier sweeping macOS as-you-type behavior found the same shell command (git log --, cd /et, echo \$HOM, etc.) intermittently produce no completions menu at all, in a configuration where natural-language detection classifies each buffer's input type (AI vs. Shell) asynchronously and per-buffer. Investigated their hypothesis by reading should_use_native_shell_completions's call site. Confirmed: run_completions_async re-reads self.ai_input_model.input_type() fresh on every call, and both the original as-you-type dispatch and the new trailing-edge retry (previous two commits) go through this exact function. detect_and_set_input_type runs its classification asynchronously (spawned, debounced), so its verdict can genuinely change between when a request is first dispatched and when a later retry re-evaluates eligibility -- if the buffer gets classified as AI input in that window, native shell completions silently stop being used for that buffer, with nothing in the existing code distinguishing this from the open_completion_suggestions gate blocking dispatch entirely (both are currently silent). This isn't a bug introduced by the retry work -- it's a property of the pre-existing is_ai() exclusion in should_use_native_shell_completions, which the retry's re-evaluation-at-a-later-time exposes to more than a single one-shot dispatch would be. Not fixing the classifier interaction here (that's a separate design question -- e.g. whether completions eligibility should be frozen for a request's duration, or whether NLD should defer to an in-flight completions round trip) -- adding observability so it stops being invisible, per the verifier's own question about whether a silent skip is distinguishable in logs. Added a debug! log line (per logging-and-error-reporting: hot per- keystroke path, so debug not info; no buffer/command text logged, consistent with never logging user-generated command content) firing specifically when AI-mode classification is the sole reason native shell completions were skipped -- distinguishable from every other skip reason (feature disabled, shell doesn't support it, multiline buffer) by construction, since those are checked in the same condition. Verified: cargo fmt clean; log::debug!/log::warn! already used extensively elsewhere in this exact file, confirming the macro is already in scope with no new import needed. Not compiled -- the app crate still OOMs in this sandbox. Not verified live; this is purely additive (no behavior change), so the risk is contained to the log call itself compiling and firing under the exact boolean condition described. Requesting the verifier enable this log level and re-run a failing case to confirm whether the hypothesis holds (the skip fires exactly on the buffers that went quiet) or whether the real cause lies elsewhere.
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.
Computer-use video recordings
View video recording - Screen recording of native shell completions working in zsh on the final PR head: typing `git ch` key by key with the completion menu appearing, then `echo hi` producing exactly one block, then `ls --col`.
Computer-use screenshots
Get-ChildItem -— parameter completions with type annotations, from the pass on92e615d3; the PowerShell path is untouched by later commits.git ch— matches with descriptions onf50ff27e, output area clean.git ch— menu open with the output area clean onf50ff27e.ls --col—--color,--color=, no descriptions, matching bash's own compspec output.Plans: none — this change was tracked in Linear rather than a plan document.
Summary
Implements in-band, generator-based native shell completions for all four shells (zsh, bash, fish, PowerShell), replacing the zsh-only, key-triggered Ctrl-Y/OSC 9280 round trip described in CORE-3794. It is stacked on #15313, which fixes CORE-3795 (the
compaddshim's-dflag lookup missing_describe's clustered-ld) and must merge first, since the zsh path here depends on descriptions being resolved correctly.Stack
compaddshim description fix (CORE-3795). Merge first.This PR targets
factory/zsh-compadd-describe-flag-fixrather thanmaster, so its diff shows only the completions work. The CORE-3795 fix and the review history relating to it, retained below, now live in #15313.The feature remains behind
FeatureFlag::NativeShellCompletions(off on every channel) and theForceNativeShellCompletionsprivate pref; neither is promoted by this PR.This PR has been through one round of review. All findings below were fixed and re-verified empirically; see "Response to review" for what changed and why.
Mechanism per shell
All four shells now compute completions for an arbitrary command line in the user's own live session and emit them through the existing OSC 9280 "completions" wire protocol (
\e]9280;A;incrementally_typed\a, then\e]9280;C;<match>\aand optionally\e]9280;D?description;<description>\aper match, then\e]9280;B\a). This protocol was already fully implemented on the Rust side (ansi/mod.rs,terminal_model.rs,completions.rs) and shell-agnostic, so no client-side parsing changes were needed for bash/fish/PowerShell — reused instead of inventing a second wire format.zsh_body.sh):selectis the only builtin that lets an ordinary command reach a real ZLE completion context (entersubsh()nullsshout/clearsUSEZLEfor any subshell,$( ), pipeline segment, or backgrounded job — seeSrc/exec.c:1156,1205). So this cannot go through the existing (backgrounded)warp_run_generator_command; it's a new foreground-only entry point,warp_run_generator_command_foreground_completions <hex-encoded line>. It probes ZLE capability and the decoded line for emptiness up front (zero matches, not an error/hang, in either case), then temporarily takes over thezle-line-initwidget for exactly the oneselectiteration it drives: it saves whatever was bound there before (by widget name, viazle -A/zle -N, notfunctions[...]— see "Response to review" for why that distinction matters), installs its own capture widget, runs{ select _ in 1; do break; done } 2>/dev/null, and restores the prior binding immediately afterward, unconditionally. The capture widget chains to whatever it replaced, setsBUFFER, invokes the existingwarp_complete_via_compadd_override_internalwidget (unchanged — thecompaddshim +warp_main_completer/_generic), then submits a throwaway single-space buffer viaaccept-line. The DCS bracketing already used elsewhere in the bootstrap swallows part of theselectredraw (not all of it — see the known limitation below). This request is synchronous and can't use the async cancel-by-PID machinery — same as the widget it replaces, not a regression.bash_body.sh):warp_run_generator_command_native_completions <hex-encoded line>resolvescomplete -p <cmd>, lazily loads the compspec via whichever of_comp_complete_load/_comp_load/_completion_loaderbash-completion exposes, synthesizesCOMP_WORDS/COMP_CWORD/COMP_LINE/COMP_POINT/COMP_TYPE=9/COMP_KEY=9aslocals (dynamically scoped, so the compspec function sees them exactly as bash's own real completion machinery presents them, and they vanish again on return rather than leaking into the user's session), and calls the-Ffunction directly (notcompgen -F, which warns and returns unfiltered results). Names only — bash has no description channel. Deliberate simplification: word-splitting usesread -ra(withIFSforced to bash's default, independent of the session's actual$IFS) rather thaneval, so a partially-typed, unbalanced quote or embedded$( )in the line under completion can never be executed; the observed tradeoff is that a quoted argument containing a space yields zero matches rather than the differently-split matches bash's own tokenizer would produce.fish.sh):warp_run_generator_command_native_completions <hex-encoded line>callscomplete -C "<line>"(the same entry pointcrates/warp_terminal/src/shell/mod.rsalready uses for executable discovery), which returnsmatch<TAB>descriptionpairs directly.pwsh.ps1): unlike the other three shells, this never executes anything as a command at all (see "Fifth round" below for why that turned out to be necessary, not just cleaner). A dedicated PSReadLine key handler (Alt+3) reads the hex-encoded line directly out of the input buffer viaGetBufferState, decodes it, calls[System.Management.Automation.CommandCompletion]::CompleteInput($line, $line.Length, $null), and reverts the buffer — neverAcceptLine. Multi-line parameter-set tooltips are collapsed to one line for display.All four functions hex-decode their sole argument (
warp_hex_decode_string/Warp-Decode-HexString, mirroring the existingwarp_hex_encode_string) instead of shell-quoting it. The client (native_shell_completions.rs) hex-encodes the buffer text on the way out, so the argument only ever contains[0-9a-f]and needs zero shell-specific quoting.The generator function names for zsh/bash/fish all start with/contain
warp_run_generator_command, so each shell's existing history-exclusion and generator-cancellation checks (_is_warp_generator_command,HISTIGNORE) recognize them without any changes to that logic. PowerShell has no generator function name to recognize at all here, for the same reason it never executes anything as a command.What was removed vs. kept
Removed (the old zsh-only trigger/round-trip, and one dead path it exposed):
NativeShellCompletionsState::AwaitingPromptand the whole enum (pty_controller.rs)SendCompletionsPromptevent end-to-end:Event::SendCompletionsPrompt,ModelEvent::SendCompletionsPrompt,TerminalModel::send_completions_prompt, theansi::Handler::send_completions_prompttrait method, the OSC9280;Pdispatch arm, and theview.rsmatch armPtyController::can_write_to_ptywarp_complete_via_compadd_overridewrapper widget + itsbindkey '^Y'(zsh)^X/list-choices path (warp_complete_via_list_choices,warp_read_completion_buffer, itszle -N/bindkey '^X'registrations, and itszstyles). This was already unreachable before this PR — nothing on the Rust side has ever written^Xto trigger it (confirmed by grep) — but removing the client's ability to answer the9280;Pread (above) turned it from "unreachable" into "would hang the shell onread ... < /dev/ttyif anything ever did trigger it." Removing it was the reviewer's offered alternative to "keep the handler and fix the stale comment"; given the hang risk, removal was the safer choice.Kept: the OSC 9280 A/C/D/B wire protocol end to end, the
compaddshim (whose description fix now lives in #15313) andwarp_main_completer/_generic, and thezle -C warp_complete_via_compadd_override_internal list-choices warp_main_completerwidget registration.New:
PtyController::run_native_shell_completionsnow resolves the active session'sShellType, builds the per-shell command vianative_shell_completions::generator_command_for, and writes it through the same in-band-command path other generator commands already use (bytes_to_execute_command+start_in_band_command_execution), instead of a bespoke keystroke write.ShellType::supports_native_shell_completionsnow returnstruefor all four shells.Decisions made that the design left open
read -rainstead ofeval, trading quote fidelity for never executing arbitrary substrings of a partially-typed line (see the exact observed tradeoff above).Explicitly out of scope
FeatureFlag::NativeShellCompletionsor changing its channel gating..zshrc/compinitsetup would provide -- see validation notes below.Current status: known issues and limitations
This PR has been through eighteen rounds of iterative fixes as of
8c2858c(full history below, kept as the evidence behind each one -- not required reading to review the current state). This section is what a reviewer actually needs.Feature gating, unchanged: still behind
FeatureFlag::NativeShellCompletions(off on every channel) and theForceNativeShellCompletionsprivate pref. Neither is promoted by this PR.Tracked, deferred issues -- each is a real, confirmed bug, not fixed in this PR, and each has its own Linear issue:
terminal.input.honor_ps1 = true(confirmed one-shot, not cumulative, across five request lengths). The tab-title symptom is confirmed to be the same off-by-N accounting gap as the header corruption, not a separate defect, and should be fixed together with it. Root cause and two candidate fixes are written up in the Tenth round below. Confirmed not reproducible on macOS -- Linux-specific.completions.rs'sFrom<ShellData> for Vec<ShellCompletion>does an unconditionaloutput.sort_by(|a, b| a.name.cmp(&b.name)), with its own pre-existing TODO predating this PR ("we need to get metadata from the shell about how the results should be sorted"). A client-side menu/data-model fix, not a wire-protocol issue -- the shell already sends its own order and the duplicate on the wire.total - Wbytes), and what a real fix needs (threading terminal width/restore-column state intoearly_output.rs, currently untracked there) are in the Thirteenth round below.;payload-truncation fix in particular) exposed it more routinely. Same family as CORE-3800/CORE-3801: client-side handling of already-correct results, not a wire-protocol issue. See the Fourteenth round below.A completions menu opens with no entry preselected on the first Tab, by design (
handle_completion_suggestions_resultspicksUnselectedwhen classic completions are enabled,Firstotherwise) -- confirmed independently on bash, PowerShell, and zsh. Pressing Enter immediately after the first Tab submits the raw, pre-Tab line rather than the top entry; press Down (or Tab again) first. This single behavior caused three independent verification passes to misreport a working feature as broken, so it's called out here at the top rather than only in the round history.A cost characteristic worth knowing, not a bug: the native-completions generator is dispatched whenever the feature is enabled, before Warp's own bundled spec results are consulted -- only which result set is used is conditional, not whether the shell pays for a foreground request. So in the flag-only configuration, every keystroke costs a foreground shell round trip even on a command where a bundled spec ultimately wins.
Documented limitations (expected behavior given the mechanism, not bugs):
selectredraw only swallows what precedes the redraw's first ESC byte, not the whole thing (see the original review round) -- whatever follows is a rendering question for the visual-verification pass, not a mechanism bug.COMPREPLYis names only); zsh/fish/PowerShell all resolve descriptions.compdef) doesn't resolve to the aliased command's completions under a minimalcompinit-only test harness -- matches real interactive Tab behavior in the same harness, so this is a harness property, not a divergence from real zsh.__NounNamecompleteness for$_., non-token-end cursor positions producing no menu, a different, still-cosmetic stale as-you-type menu -- see below) are reported but not chased further in this PR -- none looked load-bearing enough to block on, and they need a running app/computer-use to characterize properly, which the verification passes are better positioned to do than static reasoning here.terminal.input.completions_open_while_typing = true) had a request-stream gate bug that silently dropped completions for the buffer a fast typist was left looking at -- this is now fixed and verified live (Sixteenth-Eighteenth rounds below); see those for the mechanism and the fix. One separate, smaller as-you-type cosmetic issue remains open: a superseded request's menu can persist briefly on screen instead of closing immediately (clears on the next keystroke either way, and Enter always runs the correct command) -- inferred cause is in the Fourteenth round, not chased further.Honest gaps in verification: this implementation agent has had no computer-use access for almost this entire PR, so shell-script changes are verified against real installed interpreters (zsh 5.9, bash 5.2.21, fish 3.7.0, PowerShell 7.6.5) driving the actual production entry points end to end, but client-side/UI changes are hand-reviewed rather than run in the actual Warp app unless a separate verification pass (computer-use or a build agent) explicitly confirmed them -- see the screenshots/video linked above for what has been confirmed that way. The full
warp/appRust crate reliably OOMs in this ~4GB sandbox, so almost every Rust change inapp/src/...across this PR is hand-reviewed against the exact types/traits/borrow semantics involved rather than compiler-verified; only the smallwarp_completercrate could be built and tested directly here.Testing traps found during this PR (read before re-testing any of the above)
Four testing techniques used during this PR's own verification produced false negatives that each cost a full round to catch. Recording them here so the next person doesn't repeat them:
'a' * N): a wrong candidate position at the wrap boundary still matches by coincidence when every character in the buffer is identical (buffer[W] == buffer[W-1]). With realistic, non-repeating text, the exact same leak reproduced completely unchanged (Thirteenth round). Use non-repeating fixtures for anything that depends on matching a specific position in a buffer, not just matching some character.9280;Swas emitted with numbers that looked reasonable across 8 shapes (includingcd /et) -- but never by confirming the span actually covered the same text the real candidates started with. The Twelfth round found that for any multi-component path, the span covered the entire$PREFIXwhile the real candidates were bare basenames, so every one of those "verified" shapes had silently been broken the whole time; native completions inserted nothing for any of them. Verify a span by checking it against the actual candidate text returned for the same input, not by checking that the numbers alone look sane.should_retry_as_you_type_completionswas unit tested by instantiating its generic parameter withString, and every test passed -- but the actual production caller comparedEditorSnapshotvalues, whose derivedPartialEqalso compares internal state (buffer_text_runs) that a completions round trip perturbs independently of the visible text and selections the tests exercised. The result: 18 of 23 retries in one live session were redundant duplicates that the passing tests gave no signal about (Eighteenth round). This is the most general of the four traps here: when a generic helper's correctness depends on the equality semantics of whatever type it's called with in production, a test written against a convenient stand-in type proves nothing about that call site -- bind at least one test to the real type, however inconvenient to construct.Response to review
An independent review surfaced two critical issues in the zsh path, a Rust queue-draining bug, a regression in the CORE-3795 fix itself, and several empty-input/leakage/portability issues across the other shells. All are fixed and re-verified:
zle-line-init/zle-line-finishas permanent global functions and captured any prior hook viafunctions[zle-line-init]. That capture is empty for a widget bound to a differently-named function — exactly whatadd-zle-hook-widgetproduces, which is how p10k, zsh-syntax-highlighting, and zsh-autosuggestions all register. So it silently replaced (and never restored) those plugins' line hooks for every zsh user on the channel, completions or not. Fixed by moving the takeover entirely insidewarp_run_generator_command_foreground_completions, scoped to the oneselectit drives: save/restore go through widget names (zle -A,$widgets), notfunctions[...], so a differently-named bound widget survives intact. Verified with a harness simulatingadd-zle-hook-widget: the plugin's widget binding is unchanged before and after a completion request, and its function still runs (call counter increments) during the request.selectwould sit at an invisible prompt eating the user's next keystroke, with no timeout on the client side. Fixed: the widget takeover/restoration described above bounds this independently of the flag, and the flag is now also guarded so a repeat firing of the capture widget (observed to happen underselect) is idempotent rather than looping. The empty-line and non-interactive/no-ZLE cases now also return immediately with a real (empty) terminator instead of arming anything.execute_next_queued_write'sis_commandcheck didn't includeRunNativeShellCompletions, so it would immediately drain the next queued write into a shell that was mid-select— those bytes would be consumed by the read and lost. Fixed by treatingRunNativeShellCompletionslikeCommandthere, matching what the removedAwaitingPromptclause used to prevent.(I)to(i)broke it:(i)returns one past the array length (not0) when nothing matches, so theif (( __d_idx ))guard was true on everycompaddcall. Reverted to(I), and additionally restricted the search to the same leading flags-only prefix the existing-O/-A/-Dcheck uses, so a real completion candidate that happens to look like a flag (a literal-d/-ldmatch, e.g. fromls/find) is never mistaken for the flag itself. Verified with isolated unit cases for-ld(clustered), plain-d, and both false-positive shapes.''on empty/missing input instead of crashing onGetString($null), and the whole native-completions function decodes/completes inside atry/finallyso the OSC terminator is always emitted even if something throws. Fish's decoder also crashed (a missing-operandtestplus a five-line stack trace landing in-band) on a missing/empty argument; both are now guarded explicitly.printfportability. Droppingcommandfromcommand printf(to get\xdecoding right on macOS, where the externalprintf(1)'s%bonly understands octal escapes) surfaced that fish's builtinprintfdoesn't treat a leading--as an end-of-options marker the way externalprintf(1)does — it printed the two literal characters instead of anything from the format. Fixed by dropping the now-unneeded--(the format string is a fixed literal, never user input, so it was never needed for safety here).read -ranow forcesIFSto bash's default rather than trusting the session's value;COMP_POINTis now a real byte count (wc -cunderLC_ALL=C) instead of${#line}'s locale-dependent character count;COMP_WORDS/COMP_CWORD/COMP_LINE/COMP_POINT/COMP_TYPE/COMP_KEYare nowlocal(dynamically scoped, so the compspec function still sees them, but they're gone again the moment the request returns) instead of leaking into the user's global session state on every request.hexcrate's own encode/decode symmetry, which wouldn't catch a change to the wire format; it now asserts the actual contract (lowercase, unseparated, even-length hex) and decodes by hand the way the four shell scripts do. The queueing test's command assertion compared againstgenerator_command_for's own output (trivially true regardless of correctness); it now asserts the exact literal, keeping theshell_type-from-active-session assertion as the one that matters. Fixed a build error the reviewer's build agent found (ShellCompletionhas noPartialEq) by assertingis_empty()instead of== Vec::new().Known limitation, reported but intentionally not addressed in this PR: DCS passthrough ends at the first ESC byte, so the bracketing around zsh's
selectonly swallows what precedes zsh's first escape sequence in the redraw, not the whole thing. Whether any of the remainder is visible in the actual app is exactly what the separate visual-verification pass (computer-use) is checking; if it is, that's a follow-up, not a blocker for this PR's mechanism.Blocking client-integration bug found by in-app verification, and its fix
After the review round above, in-app (computer-use) verification found the feature unusable end to end: typing with native completions on produced a new visible block per keystroke, the real input buffer got truncated/emptied, and the session stopped accepting real commands afterward (Enter submitted empty lines, Up-arrow/Ctrl+U broke). Root-caused to two separate issues, both fixed:
is_in_band_command()didn't recognize the new generator names. It only matched the literal"warp_run_generator_command "/"Warp-Run-GeneratorCommand "prefixes (with a trailing space). All four native-completions generator function names are longer (warp_run_generator_command_foreground_completions,warp_run_generator_command_native_completions,Warp-Run-GeneratorCommand-NativeCompletions) and never matched, so the client classified every completions request as a normal, visible user command — a new block per keystroke. Fixed by relaxing the check to match on the shared prefix alone, the same substring convention the shell scripts themselves already use for this exact purpose (_is_warp_generator_command).bytes_to_execute_command) to type the invocation and press Enter — required for zsh'sselectmechanism, and used uniformly across all four shells for consistency. Nothing wrote the user's actual buffer back afterward, so the next keystroke landed on an empty buffer, corrupting every subsequent request and, on Enter, submitting whatever fragments had accumulated.PtyControllernow tracks thebuffer_texta request was computed from and, once results come back (ModelEvent::CompletionsFinished), queues it to the front of the write queue so it's written back to the pty verbatim as soon as the line editor is active again — ahead of anything else queued in the meantime, including a newer completions request for what the user has typed since.Both fixes are shell-agnostic (the prefix relaxation covers all four generator names; the buffer restoration is generic to
RunNativeShellCompletions), so they should equally resolve the failure for bash/fish/PowerShell if those hit the same underlying issue. Added a unit test foris_in_band_commandcovering all four generator name shapes plus a negative case.I could not verify this fix live (no computer-use in this environment) — requesting re-verification from the computer-use pass specifically for: no new visible blocks per keystroke, the input line correctly accumulating what's typed (e.g.
git checkout --detach), and the session remaining fully functional afterward (Enter, Up-arrow, Ctrl+U). If some residual flicker or latency remains even with these fixes, that would point at the deeper, harder-to-fix architectural tension flagged in CORE-3794's discussion (a foreground command execution cycle inherently happening once per keystroke) rather than at either of these two bugs specifically, and is worth flagging back to the requester as a product-level tradeoff (e.g. debouncing, or reserving native completions for explicit invocation) rather than something to keep patching silently.Validation
Build:
cargo check -p warp(the full app crate, all its GUI/wgpu/winit dependencies) is reliably OOM-killed in this sandbox (~4GB RAM, no swap), even with--no-default-features --features local_fs,local_tty,-j 1,CARGO_INCREMENTAL=0, and-C debuginfo=0— tried again after the review fixes with the same result. All dependency crates compile first; only the finalwarpcrate itself hits the ceiling.cargo check -p warp_terminal(the much smaller crate covering theShellTypechange) passes. The rest of the Rust changes are reviewed carefully by hand but not compiler-verified in this environment../script/formatran clean. Could not runcargo clippyorcargo nextest runfor the same memory reason. An independent build agent on a 32GB runner reportedcargo check -p warpclean as originally authored, with one--all-targetstest-only error (fixed here, see above).Shell scripts: verified empirically, both originally and again after the review fixes, by extracting each function into a standalone harness and driving a real interactive shell under a PTY (zsh 5.9, bash 5.2.21, fish 3.7.0, PowerShell 7.6.5 — all installed in this sandbox), comparing OSC-captured matches against genuine interactive Tab completion for the same line in the same shell process:
git ch→ 8 matches with descriptions from_describe(the CORE-3795 case), unchanged before/after the hook-ownership rewrite. A live-onlycompdefresolves correctly. A simulatedadd-zle-hook-widget-style plugin hook (a differently-named bound function) is now provably preserved across a completion request: its widget binding and callable function are identical before and after, and it still fires once during the request. Empty line → zero matches immediately, no dump. A live-only alias (as opposed to compdef) didn't resolve to the aliased command's completions in my minimalcompinit-only harness — but neither did real interactive Tab for the same alias in the same shell, so this is a property of the harness (no real.zshrc), not a divergence between old and new behavior; flagged rather than claimed as reproduced.git ch→checkout/cherry-pick/cherry, matching interactive Tab. ConfirmedCOMP_WORDSet al. are unset immediately after a request (declare -pfails to find them) — no leakage. Confirmed a custom sessionIFS(:) produces identical output to the default. Confirmed a quoted argument with an embedded space now yields zero matches (not a crash, not a mis-split). Empty line → zero matches immediately.git ch→ matches with descriptions, matchingcomplete -C's own output. Empty hex, missing argument, and any input at all now decode/complete without the stack trace the reviewer measured; confirmed the builtin-printf\xdecode produces the exact original bytes.Get-Ch→ single matchGet-ChildItem, confirmed identical to a real interactive-Tab PTY comparison inpwsh(a correction from my first pass, which incorrectly assumed PowerShell was unverifiable here —pwshruns fine on Linux).cd /tm→/tmp, matchingCompletionTextexactly (interactive Tab additionally appends/for a directory result, which is PSReadLine's own insertion behavior on top ofCompletionText, not part of the completion data). Empty string, missing argument, and malformed (odd-length) hex all now cleanly emit only the start/end OSC markers instead of throwing.I did not attempt to reproduce or verify visually in the actual Warp app (no computer-use in this environment); that pass is being run separately.
For the visual verification pass
~/.config/warp-terminal/user_preferences.json:{"prefs": {"ForceNativeShellCompletions": "true"}}(create the file/dirs if absent). Restart Warp after writing it. (The real flag isFeatureFlag::NativeShellCompletionsincrates/warp_features/src/lib.rs:165, off on every channel — the pref bypasses that.)./script/bootstraponce, then./script/run(orcargo run) to build and launch the desktop app../script/presubmitruns fmt/clippy/tests if there's enough memory on the runner.git ch(descriptions from_describe, the CORE-3795 case). For live fidelity, define a throwawaycompdefon a made-up command name in the same session, then complete it.git ch,ls --col(flag completion, no descriptions — bash has none).git ch(with descriptions),cd /et(path).Get-Ch(single unambiguous match + tooltip),Get-ChildItem -(multiple flags + tooltips),cd /tm(path).selectredraw (beyond what the DCS bracketing swallows) is visible on screen for a moment during a completion request.TERM != emacs— the generator deliberately reports zero matches rather than hanging otherwise.zle-line-init(p10k, autosuggestions, syntax-highlighting, vi-mode) should keep working normally after a completion request — that's the specific thing the critical fix above addresses.-Fcompspec;compgen/-W-only compspecs aren't attempted.Second verification round: PowerShell fix, phantom blocks, history/title leaks
The first client-integration fix (above) resolved zsh, bash, and fish end to end, confirmed by the computer-use pass: exact input accumulation, correct menus with descriptions, and normal session behavior (Enter/Ctrl+U/history) afterward. That pass found four more issues, three of them now fixed here, one investigated and explained:
Alt+2, sent as the two-byte sequenceESC '2') requires PSReadLine to disambiguate an escape sequence, and when those two bytes arrive in the same write/read as the command text that follows, PSReadLine sometimes fails to recognize the chord at all — leaving the existing buffer untouched while the (undecoded) command text types on top of it. Confirmed empirically with a PTY harness: sending the chord and the command text as two separate pty writes (even with zero explicit delay between them) reliably fixes it, while a single combined write reliably reproduces the bug. Fixed by splittingPtyController::send_write_to_event_loop's PowerShell writes into twoMessage::Inputcalls at the exact byte boundarybytes_to_execute_commandalready establishes (the kill-buffer bytes, then everything else), via a newsplit_kill_buffer_writehelper with unit tests. The other three shells are unaffected — their kill-buffer byte is a single, unambiguous control character with no escape-sequence parsing involved, sosplit_kill_buffer_writeis a no-op for them.EarlyOutput's typeahead-vs-background-output classification only recognizes explicitly-registered input (push_user_input) when the shell usesTypeaheadMode::InputMatching(legacy bash only) — forTypeaheadMode::ShellReported(zsh, fish, PowerShell, and most bash), any raw character echo received while no block is running becomes background output, since normal typing for those shells never touches the pty until Enter and this scenario had never come up before. Fixed by addingEarlyOutput::push_expected_echo(and aTerminalModelwrapper), which registers input as expected echo regardless ofTypeaheadMode, and changinghandle_potential_typeaheadto always try consuming it first.PtyControllernow calls this immediately before writing the restored buffer text back, so the echo is recognized as typeahead (and correctly fed back into the input editor, which is what typeahead is for) instead of falling through to background-output handling. This is additive and touches nothing else:push_user_inputand its existingInputMatching-only behavior are unchanged, and nothing else populates the new registration path, soShellReported-mode sessions behave exactly as before unless something explicitly calls the new method. Added a unit test exercising this forTypeaheadMode::ShellReportedspecifically (the mode the existing tests show not auto-matching without it).No such widget `zle-line-init'string, with byte-for-byte identical stale metadata, rode along in the phantom blocks — across zsh, bash, and fish, including runs where zsh wasn't involved at all. The verifier could not reproduce this by hand in the same live session (0 bytes on stderr, balanced widget bookkeeping). Identical stale content appearing across otherwise-unrelated shells is not something a real shell could produce live; it's much more consistent with a restored block from an earlier test pass — before theis_in_band_commandfix landed, when generator commands really were visible, ordinary blocks — surfacing again via Warp's session/tab restoration. That block would have satisfiedTerminalModel::restored_block_commands()'s filter (which didn't check for in-band commands) and fed straight into the Up-arrow history overlay, which is exactly finding 4 below. Given both findings point at the same restored-block path and the phantom-block mechanism above is now fixed independently, I did not chase this further as a separate live bug; the defensive fix in item 4 should prevent it from recurring regardless of the exact history of any given block.TerminalModel::restored_block_commands()filtered restored blocks onis_restored() && !is_background() && state() != DoneWithNoExecution, but never checkedis_in_band_command_block()— so a restored, pre-fix generator-command block (see above) would have been included. Added that check. Also added a defensiveis_in_band_commandcheck at the top ofupdate_command_history(theExecuteCommandEvent-triggered path), even though generator commands are never expected to reach it (they're written directly viaPtyController, bypassingExecuteCommandEvententirely) — cheap insurance against any future code path accidentally routing one through there.fish_titlefunction sets the window title to the currently-running command (truncated to 20 chars) via its own, independent OSC 0/2 title-setting mechanism — entirely separate from Warp'swarp_preexecJSON hook, which does already know about in-band commands. Since nothing overrodefish_title, it dutifully showedwarp_run_generator_command_nativ…while a completions request was running. Fixed by overridingfish_titleinfish.shto fall back to its own existing "just show pwd" behavior (the same thing it already does for its ownfishbuiltin case) when the command matches the generator-command prefix, otherwise reproducing upstream's exact format (including theINSIDE_EMACS/SSH-hostname handling) unchanged. Verified against the installed fish for a real command, a generator command, fish's builtin case, and no argv at all.Two settings needed to exercise the feature at all, worth calling out so nobody re-derives them:
terminal.input.completions_open_while_typingdefaults to false (nothing fires as you type until it's turned on), and a restored tab keeps its original shell regardless ofWARP_SHELL_PATH— open a fresh tab and confirm the shell before trusting a per-shell result.Verification for this round: items 1, 2, and 5 were verified empirically — item 1 via a PTY harness comparing the combined-write (broken) and split-write (fixed) cases for both the
Alt+2chord and aCtrl+2/NUL alternative I ruled out along the way (also broken, so the fix is specifically about write-splitting, not chord choice); item 2 via a new unit test inearly_output_tests.rsexercisingTypeaheadMode::ShellReported; item 5 via the installedfishdirectly. Items 3 and 4 are explained and defensively fixed but not independently reproduced live, since the theory is that they were already stale/historical by the time this round started. I could not run the actual computer-use verification myself in this environment; requesting a third pass to confirm PowerShell now works end to end and that the phantom block and tab-title issues are gone in zsh/bash/fish/PowerShell as applicable.Fourth round: fixed a regression from the second round, and the fish history leak
The third verification pass (on
e44204c) found that the phantom-block fix from the second round introduced a new regression: typing intermittently duplicated the buffer (g→gigi→gigitgigit, compounding, occasionally CPU-pinning the app for 25+ seconds). It also confirmed PowerShell was still broken in the same way, and pinpointed the fish history leak's exact cause.Root cause of the regression, confirmed (not speculative):
push_expected_echo(added in the second round to stop the restore write's echo from rendering as a phantom block) fed the restored text into the same queuepush_user_inputuses for real typeahead. A match there is surfaced viaTerminalEvent::Typeahead, which the input editor consumes withinsert_typeahead_text— correct for real typeahead, where the editor lost that text and needs it back, but wrong here: the input editor's own buffer was never cleared in the first place (only the real shell's buffer was, by the kill-buffer+type+Enter cycle). So re-inserting the restored text via the typeahead path duplicated it on top of what the editor already had, and the duplication compounded on the next keystroke's own restore.Fix: gave
push_expected_echoits own backing queue (EarlyOutput::expected_echo, separate fromunmatched_input) and a dedicatedconsume_expected_echo, checked ininput()/carriage_return()/linefeed()before the existing typeahead/background-output logic. A match there is now dropped entirely — never surfaced as typeahead, never rendered as background output — which still satisfies the original phantom-block fix's goal without the side effect that caused the duplication.handle_potential_typeaheaditself is reverted to its original, pre-second-round behavior. Updated the existing unit test to asserttypeahead()stays empty (previously asserted it got populated, which was the bug).fish history leak, root cause and fix: fish has no configurable history-exclusion mechanism (unlike bash's
HISTIGNOREor zsh'shist_ignore_space) — a leading space is fish's only, default, non-configurable way to omit a command from its history file.generator_command_for's fish case never added one. Fixed by adding it, matching the exact conventionInBandCommandExecutor::execute_command_internalalready uses for the pre-existingwarp_run_generator_commandmechanism;bytes_to_execute_command's bracketed-paste leading-whitespace preservation (which this depends on) already existed for this exact reason. Added a dedicated test locking in the leading space for fish and confirming the other three shells don't gain one.PowerShell — not re-attempted this round, per explicit instruction. The orchestrator is taking the underlying design question (whether native completions should fire per keystroke at all, versus only on explicit invocation) to the requester, and asked me to hold off on further chord-level fixes until that comes back, and specifically asked my opinion on an alternative approach: binding a PSReadLine key handler that calls
[Microsoft.PowerShell.PSConsoleReadLine]::GetBufferState()directly, runningCommandCompletion::CompleteInputon that text, and emitting the OSC from inside the handler — no kill-buffer, no command text typed, no Enter, no buffer restore.I agree this is the right redesign for PowerShell specifically, independent of how the per-keystroke question is resolved. It's structurally the same idea as zsh's
selecttrick (reach the completion engine directly rather than faking a command execution) and it's a more natural fit for PSReadLine's architecture, where a key handler can call .NET APIs directly without any command-execution semantics at all. It would eliminate all three PowerShell failure modes observed so far (the atomicity issue, the no-menu case, and the corrupted-history case) simply by never touching the real buffer, kill-buffer, or Enter — and it already has a working precedent in this same bootstrap script: the existingAlt+1input-reporting handler does exactly this shape (GetBufferState+Warp-Send-JsonMessage, no command execution) for a different purpose. I have not implemented this, since it was explicitly out of scope for this round pending the design decision.Fifth round: PowerShell redesign, zsh restore fix, tab title leak, async-path research
PowerShell: redesigned around
GetBufferState, no command execution at allThe kill-buffer+type+Enter+restore idiom (shared with the other three shells) turned out to be fundamentally unsafe for PowerShell specifically: PSReadLine doesn't reliably disambiguate the kill-buffer chord when concatenated with what follows it, even split across two pty writes. Rather than continue chasing chord-level fixes, PowerShell's native completions are redesigned around a dedicated PSReadLine key handler (
Alt+3) that reads the buffer directly viaGetBufferState, computes completions viaCommandCompletion::CompleteInput, and reverts the buffer — neverAcceptLine. This is structurally the same trick zsh'sselectuses (reach a real completion context without faking a command), and it eliminates every PowerShell failure mode found in verification at the root, since none of them can occur when nothing is ever typed as a command or submitted:AddToHistoryHandlercheck is now unused for this path) and no way for it to auto-execute.generator_command_for's PowerShell case now returns just the hex-encoded buffer text (no function-call syntax).send_write_to_event_loop's handling ofRunNativeShellCompletionsbranches onshell_type: for PowerShell it types the hex text (registered viapush_expected_echoso it isn't rendered as a phantom block) immediately followed by the trigger chord, withis_for_command=falseand nobuffer_textstored for restoration.execute_next_queued_write'sis_commandgating is now shell-type-aware for the same reason — PowerShell's write never transitions the line editor back to active the way a real command's precmd would, so gating queue draining on it would stall forever.Verified empirically end-to-end via
tmux(a bare, unsized PTY madeRevertLinethrow — needed a real terminal size): theAlt+3binding registers correctly (not shadowed by the defaultDigitArgumentbinding),Get-Chdecodes and completes toGet-ChildItemwith its full description via the same OSC 9280 wire format the other three shells use, the buffer is confirmed empty afterward viaGetBufferState, nothing auto-executes, and the session stays fully functional (Write-Hostright after runs normally).zsh: fixed a live "No such widget `zle-line-init'" error
Root-caused with a minimal, isolated repro (a
zle-line-inithandler that callsaccept-lineon itself inside aselectloop, nothing else involved): deleting thezle-line-initwidget viazle -Dafter that specific pattern corrupts zsh's own internal state for the next interactive prompt read, which then fails outright with the widget error — regardless of whether the widget being deleted is one we bound ourselves or something else. An earlier version of this fix checked${+widgets[zle-line-init]}before deleting, but that doesn't help: the widget still exists at that point (we're the ones who bound it), so the check is true and the delete still runs, still corrupting the next prompt — confirmed this doesn't actually resolve the error in the common case. Fixed by never deletingzle-line-initwhen nothing was bound to it before our takeover: the armed-flag-guarded capture widget is left in place instead, which is a transparent no-op for every future firing until the next request re-arms it.zsh and bash: fixed the tab-title leak
warp_set_title_active_on_preexec(both shells) is apreexechook that fires for every command, registered before the user's RC files are sourced — so it runs for native-completions requests too, briefly setting the tab title towarp_run_generator_comma.... Neither shell's title hook had the same generator-command exclusionwarp_preexec's own PID-killing logic already has. Fixed both to skip title-setting for generator commands, matching the existing convention (zsh reuses_is_warp_generator_command; bash mirrors its ownwarp_preexec's prefix check).Async-path research: can bash/fish/PowerShell drop the foreground round trip?
Tested empirically (not reasoned about) whether the three non-zsh shells could compute completions through the existing backgrounded
warp_run_generator_commandmechanism instead of a foreground one, which would drop the kill-buffer/Enter/restore/block-classification machinery entirely for whichever shells can do it:( ... & wait ), the exact existing generator pattern) produced identical results to foreground, including a completion registered live in that session viacomplete -F— confirmed visible in the subshell since it's a true fork of the interactive process.complete -Cthere sees persisted/config-file completions fine, but a completion registered live in the current session was invisible to the child process — a real loss of live-session fidelity, which is the entire point of this feature.[powershell]::Create()/runspace-pool shape from this bootstrap script, not justStart-Job. It's a separate execution context that doesn't inherit live-session state either (it has to explicitly load common functions). This confirms theGetBufferStateredesign above is the right call for PowerShell independent of this question.This is left as a design/scope question for review, not implemented in this PR: bash could move to the async path (dropping the foreground machinery entirely for that shell), but fish and PowerShell cannot without giving up live fidelity, and zsh already can't for the structural ZLE reason explained above.
Fish: confirmed a non-command-execution route exists, not implemented
Per a follow-up question: does fish have an equivalent of zsh's
zlewidget / PowerShell's PSReadLine handler — something that reads the live buffer and emits completions without ever executing a command? Confirmed empirically that it does:bindcan bind a key directly to a fish function (running in the live interactive process, not a child process) that readscommandline(fish's equivalent of$BUFFER/GetBufferState), callscomplete -Con it, and returns without ever callingcommandline -f execute. Tested viatmux: bound to\ex, it correctly returnedgit ch's real completions with descriptions, matching standalonecomplete -Coutput. (An initial test underfish --no-configreturned filename completions instead of git's — a test-harness artifact from skipping fish's own completion autoloading, not a limitation of the mechanism; a normal fish session with completions loaded works correctly.) This would let fish drop the foreground command-execution path entirely, the same way the PowerShell redesign above does — not implemented in this PR, reported as a viable follow-up alongside the bash async-path result.Sixth round: a correctness cleanup, and two known gaps written down honestly
A fifth verification pass found the requester's own machine reproducing a phantom block containing a single trailing character (e.g. typing
starship prin zsh left a block containing justr), distinct from the earlier phantom-block shapes already fixed above. Investigating it surfaced a real, independent defect in the write queue, which is fixed here — but the requester's own repro (Tab-triggered, a single completions request, on a buffer already fully typed before the request was made) has nothing queued behind that request's restore at all, so this fix does not explain or resolve that specific symptom. It's included on its own merits as a correctness cleanup; the phantom-block investigation itself is ongoing and not part of this update.The defect:
ModelEvent::CompletionsFinishedqueues the buffer restore to the front ofpending_writesand drains it viaexecute_next_queued_write. That function is meant to stop draining immediately behind a foreground command —RunNativeShellCompletionsalready gets that treatment for the three shells that run it as one — but the restore write undoes such a command's buffer-clearing effect without being recognized as needing the same protection, since it goes out as a plainPtyWrite::Bytesrather than being tied to the command whose aftermath it's cleaning up. If a newer completions request's own write is already queued behind the restore when the restore drains, the existing recursion sends that newer request's kill-buffer immediately behind the restore, with no gap for the shell to have processed it.The fix, and a correction along the way: the first shape of this fix I considered was gating
execute_next_queued_write'sis_commandcheck on the restore write the same way it already gatesRunNativeShellCompletions. Writing the actual change surfaced that this would deadlock: that gate's only way of unblocking is the shell's own precmd firingLineEditorStatusEvent::Activeagain, and nothing about a plain buffer write — no command runs, no prompt cycle happens — ever causes that on its own; every later write would stay queued until an unrelated real command happened to run. Implemented instead as: skip queueing the restore at all when a newerRunNativeShellCompletionsrequest is already waiting behind it, since that request's own kill-buffer is about to clear the line again anyway, making the restore pointless and racy to send. This doesn't touchexecute_next_queued_write's draining logic or unblocking condition at all.Traced every path a superseding request can take to confirm this is safe rather than a new way to lose the buffer (full argument is in the code comment at the skip site, in
pty_controller.rs): either the newer request's write never reaches the pty at all (rejected bybefore_write_fn, or retain-filtered away by a still-newer request while still queued), in which case the real buffer was never touched and there's nothing to restore; or its kill-buffer does go out, at which point it can no longer be retain-filtered, so it will run to completion and fire its ownCompletionsFinished, where the same check repeats. That recursion is bounded by real keystrokes, so the first request in the chain that finishes with nothing newer queued behind it has its restore sent — and submitting a command requires the user to stop typing regardless, which is what lets the chain resolve before Enter is reachable.Two known gaps, written down rather than left to only survive in review discussion:
CompletionsFinishednever fires for it and the buffer is never restored. This was already true before this fix and remains true after it, since the original code also only ever restored on that event firing. Not fixed in this PR.pty_controller_lifecycle_tests.rshas no precedent for drivingModelEvent::CompletionsFinishedthrough the realEventchannelModelEventDispatcherforwards from, synchronously insideApp::test— every existing test in that file callsPtyControllermethods directly instead. I don't have a confirmed way to verify a test sending through that channel would be exercised before an assertion runs, and would rather leave this gap explicit than write a test that passes without actually exercising the code path.Seventh round: root cause and fix for the trailing-character phantom block
The requester hit a phantom block containing a single trailing character (e.g. a block reading just
rafter typingstarship prin zsh) on his own machine, distinct from the phantom-block shapes already fixed in earlier rounds. Root-caused and fixed via cross-platform reproduction (Linux and macOS) with byte-level diagnostics.Root cause.
EarlyOutput's expected-echo matcher (added in an earlier round to stop the buffer-restore write's own echo from rendering as a phantom block) only rearmed on a carriage return. A line editor's redraw doesn't always rewind with a carriage return, though:terminal.input.honor_ps1 = false), the restored line starts at column 0, so ZLE's rewind is a plain carriage return -- already handled.terminal.input.honor_ps1 = true(the shell draws its own prompt), the line no longer starts at column 0, so ZLE switches its rewind onto relative motion instead: a backspace for a one-character rewind, or CUB (\x1b[<n>D) for a full-line rewind. Neither was handled at all --backspace()andmove_backward()blindly delegated to the background-output path with zero interaction with the matcher.\x1b[<n>C) rather than re-echoing it. This was also unhandled, leaving the matcher's live candidate stranded at its pre-move position instead of advancing past what the CUF skipped over -- which mismatches, and leaks, whatever arrives next. This is very likely the actual mechanism behind the reported single trailing character.Fix. Backspace and CUB now shift every live candidate back by their exact, known distance (
EarlyOutput::rearm_after_rewind); CUF shifts every live candidate forward by its own known distance (EarlyOutput::advance_after_forward_move) -- both additive, never discarding existing candidates, the same principle the original carriage-return handling already used. The carriage-return case itself was narrowed back to seeding only position 0 (an earlier attempt widened it to seed every position, motivated by a since-disproven hypothesis that the trailing-character shape was carriage-return-based rather than a rewind with a known distance) -- traced both measured shapes against the narrower rule and confirmed neither needs the wider one.Also fixed, found while investigating:
expected_echo/expected_echo_positionswere never cleared when a real command starts, so a pattern left over from the last restore could in principle persist indefinitely across later commands' own carriage returns/backspaces/CUB/CUF, risking a character of unrelated output being silently absorbed if it happened to match something in the stale text. AddedEarlyOutput::reset_expected_echo, called fromBlockList::start_active_block(never fromstart_active_block_for_in_band_command, which a generator/completions request's own command uses, since clearing there would defeat the restore before it can complete).Validation. Cross-platform reproduction with byte-level diagnostics on a scratch branch (not merged into this PR): Linux confirmed the backspace/CUB shapes and the compiled, machine-run
early_outputtest suite (20/20 passing, including a deliberately discriminating test for the CUF fix -- verified failing against the pre-CUF-fix code and passing against the final code); macOS confirmed all three motions firing and being absorbed across 40 restores over four configurations, withreset_expected_echofiring once per command start and real command output remaining intact.Two honest limitations, not defects:
reset_expected_echois real by construction, but was never observed actually biting: on the pre-fix build, a stale pattern was consulted 350+ times after its own request had finished, and in every case it only ever matched characters that genuinely belonged to its own restore echo (i.e. it got lucky, not that the gap doesn't exist). Describing this as closing a real, if unobserved, gap rather than fixing something caught in the act.Separate, pre-existing, cosmetic issue found during this verification (not fixed here): with
terminal.input.honor_ps1 = true, the command block's header intermittently drops part of the prompt and splices it into the command text, sourced from the shell's own erase-and-rewind bytes; command execution and output remain correct. Worth a follow-up issue; not addressed in this PR.Eighth round: cobra description padding, three fish defects, and two zsh gaps in the CORE-3794 mechanism itself
This round's findings are split between defensive hardening for paths the client doesn't currently reach, and two real, previously-undetected gaps in the zsh completion mechanism this PR adds.
bash: cobra-generated completions baked descriptions into the inserted text
cobra's "bash completion V2" scripts (
gh,kubectl, and most modern Go CLIs) branch on$COMP_TYPE: under9(plain Tab) with more than one match, they bake a padded"name (description)"string directly into theCOMPREPLYentry -- safe for real readline, which only ever inserts an entry when it's unique, but not for us, since we display every entry in a menu and insert whichever one is picked. Measured withgh's real completion script:gh pr cheunderCOMP_TYPE=9returned"checkout (Check out a pull request in git)"verbatim, which would have been inserted as that whole string. cobra's own case statement strips descriptions unconditionally underCOMP_TYPE37(menu-complete) or42, regardless of match count (cobra#1508). Fixed by switchingCOMP_TYPEfrom9to37in_warp_native_bash_completions. Confirmed empirically this has no effect on non-cobra completions: bash-completion (which drives the vast majority of scripts, including git's) never reads$COMP_TYPEat all, and_git's completion function produces byte-identical output underCOMP_TYPE9and37. Addedbash_native_completions_test.sh, a self-contained regression test (synthetic cobra-style and ordinary bash-completion-style functions, nogh/gitdependency) covering both shapes; confirmed it fails against the pre-fixCOMP_TYPE=9behavior and passes against the fix. It isn't wired into any CI job -- this repo has no shell-script-level test harness for the bootstrap scripts -- so it's a manually-run regression test for now, not an automated one.Whitespace-prefix buffer guards: reachable via the cursor, not just a whitespace-only line
fish's native-completions guard (
test -n "$line") didn't cover a whitespace-only line, and neither did zsh's ([[ -z $line ]]) -- both would fall through to "complete everything," the same class of bug already fixed for the empty-string case in an earlier round. Measured for fish by callingwarp_run_generator_command_native_completionsdirectly against a real fish 3.7.0 session: a single space produced 1007 matches (51KB). Both are now fixed by trimming before the emptiness check. bash and PowerShell were checked and already handle this correctly (bash's derived$cmdcomes out empty and returns early; PowerShell'sCompleteInputon whitespace already returns zero matches and a negativeReplacementIndex, already documented inpwsh.ps1).Why this is reachable, not just a theoretical guard: the client sends
buffer_text[0..cursor_position](input.rs:12435), so what has to be whitespace-only for this guard to matter is the prefix up to the cursor, not the whole line. The client genuinely does send an empty-prefix request in normal use: with a non-empty line already typed and the cursor moved to column 0, the app issuesRunNativeShellCompletions("")against that non-empty line -- observed directly in the app's own logs, not inferred. This is exactly the case the empty-string guard (from an earlier round) protects against, so that guard is load-bearing in normal use, not paranoid. The whitespace variant (leading spaces, cursor placed after them, then Tab) is the same manoeuvre one column over, and is expected to be reachable the same way, though the exact repro is still being measured in-app as of this writing (an initial attempt was confounded by Warp's Home binding being a smart-home that jumps to the first non-whitespace character rather than column 0). What's already confirmed directly in the app, independent of that measurement: a fully whitespace-only line with the cursor at the end of it does not currently trigger a request at all (1/2/3 leading spaces + Tab with nothing else typed produced norun_native_shell_completionscall in the logs, identical before and after this fix, while a real query likestar+ Tab worked normally in both). Two edges of the whitespace-prefix case remain untested and are named here rather than left implicit: a buffer containing a literal tab character (untestable through the Tab-triggered harness, since Tab is the trigger itself), and whetherterminal.input.completions_open_while_typing = true(the as-you-type path, off by default) changes any of the above.Also fixed in fish:
warp_preexec's generator-kill loop had a typo predating this PR -- the loop variable ispidbut the kill command used$pids(undefined, expands to nothing), sokill -9 $pidsnever killed anything. This was previously dormant because of a separate negation bug (test (! ...), fixed in an earlier round) that kept this branch from ever running; fixing that made the typo's effect live for the first time. This one is a real, currently-reachable bug (a stale generator job now survives and keeps running after a real command starts), not defensive hardening. Verified empirically: pre-fix, a background job tracked in_warp_generator_pidssurvived the kill loop (jobsstill showed it running afterward); post-fix, the same job is killed as soon aswarp_preexecfires for a real command.zsh: native completions were silently discarded for any sub-token replacement
The most impactful finding this round:
cd /etandecho $HOMshowed no menu at all in the app, despite the shell genuinely returning matches (etc;HOMEplus severalHOMEBREW_*). Root cause: onlypwsh.ps1reports the OSC9280;Sreplacement span added in an earlier round, so for zsh the client falls back to a whitespace-derived token guess and filters candidates by requiring them to start with it --etcdoesn't start with the guessed token/et,HOMEdoesn't start with$HOM, so both were silently dropped. This affects any candidate that only replaces a sub-token of the current word: a path segment after a literal/, a$/${parameter sigil, or similar.Fixed by extending the
9280;Sspan protocol to zsh'scompaddoverride shim:start = ${#line} - ${#PREFIX},length = ${#PREFIX}, reported once before each match batch. This holds regardless of how$PREFIXarose, because it always carries the literal characters typed for the segment being completed, never an expanded or dequoted form -- verified against 8 shapes with a real zsh session and raw PTY capture of the actual OSC output:cd /et,cd /usr/lo(nested path),echo $HOM,echo ${HO(brace param),foo=/tmp/(assignment value),cd foo\ ba(literal backslash-escape),cd "foo ba(open quote, correctly excluding the quote character), andcd ~/Doc(literal un-expanded~). Checked whether bash and fish share this problem instead of assuming: they don't -- both report whole-word candidates (bash's/etc, fish's/etc/) that already start with the client's whitespace-derived guess.zsh: a
zstylecontext pattern that never matched anything, silently dropping every_describe-driven descriptionA second, independent zsh finding:
uvx --pandatuin(a real subcommand list) returned matches with every description empty, while the same shell's own interactive listing shows real descriptions. Root cause: thelist-grouped/insert-tab/verbose/list-separatorstyles were registered against':completion:warp_complete_via_compadd_override:*'-- the name of the widget. But this completion runs from inside thezle-line-inithook, so$curcontext's leading component is always literallyzle-line-init(measured, e.g.zle-line-init:complete:atuin:argument-1), never the widget name -- so none of these styles were ever actually applied to any real completion request. Withverbosenever turned on,_describe-driven completions (the mechanismclap_complete's zsh generator, and many other completion functions, use for subcommand/flag descriptions) never populated a real description array to begin with; thecompaddshim's own-ddetection was already correct, there was simply nothing for it to find. Fixed by matching':completion:zle-line-init:*'instead. Verified end-to-end with a real downloadedatuinbinary and its actualclap_complete-generated zsh completion script: before the fix,atuinreturned 32 matches with every description empty; after, real per-subcommand descriptions come through (confirmed the three that still show empty --wrapped,config,contributors-- genuinely have no description in atuin's own--helpoutput either). Also confirmedatuin --h, an_arguments-driven flag rather than a_describeone, now correctly returns--helpwith description "Print help", so the fix isn't limited to one code path.Not yet addressed, pending further data
git checkoutwith thousands of matches vs. smaller sets that render fine) is under active investigation by the requester, bisecting the exact match-count threshold; not addressed in this round pending that number.\x1b[A/\x1b[1Bcursor motions (moving between rows) that the linear-character-stream expected-echo matcher from the seventh round doesn't handle -- being verified visually before any fix is attempted, since this is a different axis (row motion vs. in-row motion) from everything fixed so far and deserves to be reasoned about on its own rather than pattern-matched onto the existing rearm helpers.COMP_TYPEchange not independently re-verified in a working bash-completion environment as of this writing -- my own verification's sandbox initially lacked thebash-completionpackage; I installed it and re-verified against realgh/gitcompletion scripts (see above), but a separate agent with dedicated bash-completion/cobra/clap/node-provider coverage is checking the blast radius of this change across a wider set of real-world completion scripts, since it now applies to every bash completion request, not just cobra's.Ninth round: zsh
-Ssuffix dropped, and a wrapped-line redraw leakTwo more zsh-specific findings from continued verification, plus a set of passes worth recording since they cover exactly what earlier rounds were most worried about.
zsh:
ls --colinserted a semantically different commandzsh's own listing for
ls --colis--color=followed by the value list (always auto never); native completions inserted--colorplus a space and offeredalwaysas a ghost, so accepting it producedls --color always--alwaysbecomes a path operand rather than the option's value, changing what the command does. Root cause: compadd's-S sufgives the string it adds after every match, meant to be inserted (_arguments's'--color=-(never auto always)'spec passes-S '='so the option and value join correctly). Thecompaddoverride shim already parsed this correctly into$asufviazparseopts(confirmed: holds=for this exact case) but the match-display loop only ever appended the directory suffix ($dsuf, from-f) --$asufwas extracted and then silently never used. Fixed by appending it to each match.$hsuf(from-s) is-S's display-only counterpart and is deliberately left out of the inserted text. Verified with a real zsh session (raw PTY capture of the OSC output):ls --colnow emits--color=(was--color); regression-checkedgit chandcd /etto confirm completions with no-Ssuffix are unchanged.A wrapped-line redraw leaked its tail into a background block
Confirmed on Linux zsh, with an exact rule: once a restored buffer is longer than the terminal width minus the prompt's own width, every byte from the wrap point on leaked into a background block -- exactly
total - (columns - prompt_width)bytes in each measured case (125/205/55-character buffers at two terminal widths; clean at shorter lengths and at 20/50/74/80-character buffers that didn't reach the wrap point). This is very likely the true explanation for the trailing-character phantom block from an earlier round, which nobody had reproduced directly, and reaches down to buffers as short as 55 characters at a narrow window width.Root cause: ZLE's redraw for a wrapped line clears the remainder of the first display row with literal space characters (not an erase-to-end-of-line escape) before moving to the next row with CUD (
\x1b[1B). The first such space doesn't match the buffer's own text at that position, so it was treated as the first real mismatch that ends the expected-echo window (existing, documented behavior -- also what makes a stale registration eventually stop absorbing unrelated output). Once the window closes, everything that follows leaks, even though the rest of the redraw (the second row's real characters) does go on to match.move_up/move_down(CUU/CUD) themselves were already harmless no-ops with respect to the window either way -- they're a pure screen-position detail of a redraw already known to be in progress, so leaving them untouched was correct, not an oversight.Fixed by no longer letting a space end the window on its own, as long as at least one live candidate still expects more of the registered pattern -- deliberately narrower than "the window is open at all," so a stray space received after the pattern is already fully matched still renders normally; only a space received mid-redraw is treated as wrap padding. Added a test reproducing the exact measured byte shape (first-row characters, space-fill, CUD, second-row characters) and confirming it doesn't leak, plus a companion test confirming the narrower scoping doesn't swallow an unrelated space once the pattern is already fully matched.
Not verified against a live build:
cargo check -p warpreliably OOMs in this sandbox, as in every prior round of this PR -- reviewed by hand against the exact measured byte sequence and the file's existing test patterns, with the two new tests requesting the verifier's usual full build/test pass.A second, related-looking finding from the same round, not fixed here: committed block headers and tab titles get corrupted (truncated and fused with command text, e.g.
MINRC /workecho HEALTH2-OK) immediately after a completions request, one-shot per request. My hypothesis, explicitly not yet confirmed: the expected-echo matcher's swallow (both the space case above and the pre-existing carriage-return/backspace/CUB/CUF cases) never advances any grid's own cursor-position tracking for the swallowed characters, since they never reach a grid'sinput()at all. If the same physical redraw is split across this swallow and a later grid that does receive characters, that grid's cursor tracking would end up behind by exactly the swallowed count -- consistent with a fused/truncated header. I have not verified this against the actual grid/cursor code, and a real fix would need to advance cursor state without rendering visible content, which is a larger change than I want to make speculatively without confirmation. Flagging for the verifier to check whether the corruption's byte offset matches the swallowed count, which would confirm or rule this out.Passes worth recording
From continued verification of earlier-round fixes, since they cover exactly what those rounds were most worried about at the time:
zle-line-initinstalled by a plugin after our own takeover both survives and keeps firing -- 48 firings logged across a session, including alongside a completions request's own buffer and the real prompt read that follows it, with no nesting error and no "No such widget" error at any point.compdefand a live-only alias (registered only in the running session, not from any file) both resolved correctly through native completions, confirming results are genuinely coming from the live session rather than a static/cached completion database.Not a finding:
ls --colon macOS returning nothing is correct, since BSDlshas no--coloroption at all.Tenth round: a crash fix, three more real bugs, and one deferred to a follow-up
The PowerShell matrix found a crash and two more real bugs; the bash matrix found a live regression in the previous round's own fix. All are fixed except one, which is deliberately deferred.
Crash: PowerShell's UTF-16 replacement-span offsets used as UTF-8 byte offsets
Typing e.g.
echo 中 Get-Ch(any multi-byte character before the completed token) and pressing Tab crashed the app --panic: byte index N is not a char boundary-- reported live twice, once auto-recovering into a fresh session (losing the window/tab) and once taking the window down entirely. Root cause:CommandCompletion.ReplacementIndex/ReplacementLengthare .NET UTF-16 code-unit offsets;pwsh.ps1sent them as-is over9280;S, and the client slices its UTF-8 buffer directly at those offsets (Span::slice,&source[start..end]), landing mid-character whenever any multi-byte text precedes the token. This was a known, explicitly-named gap in the original PowerShell span commit ("only exact for ASCII lines"), not a new regression -- it just hadn't been exercised with non-ASCII input until this pass.Fixed at two layers: (1)
pwsh.ps1now converts to UTF-8 byte offsets at the source, using[System.Text.Encoding]::UTF8.GetByteCount()on the relevant substrings of$line-- verified against the installed pwsh for both a CJK character and an emoji (a UTF-16 surrogate pair) before the token, confirming the converted byte range slices cleanly to the token. (2)Span::sliceitself can no longer panic on any input, clamping both offsets to the nearest valid char boundary and to the string's bounds -- a wrong menu is recoverable, a panicked window is not. This is the first Rust change in this PR verified by an actual compiler and test run rather than by hand:crates/warp_completeris small enough to build in this sandbox (cargo check/cargo test -p warp_completer --lib meta, 6/6 passing, 5 new tests).zsh's own replacement span had the same class of bug
${#var}in zsh counts characters, not bytes, while the wire format is byte offsets. Any accented or CJK character before the completed token shifted the reported start left by exactly the extra UTF-8 byte count (ls /tmp/café/xyoff by 1,ls /tmp/日本/nioff by 4) -- caught before it could panic anything, sinceSpan::slice's new clamp absorbs it, but still a wrong query and a wrong insertion. Fixed withlocal LC_ALL=Cbefore the span computation, which makes${#...}count bytes (confirmed empirically); scoped to the rest of thatcompadd()call, which is fine since nothing later in it does character counting. Verified against the exact reported shapes with realcafé/日本directories in a real zsh session, raw PTY capture of the OSC output, and byte-level slicing of the real line to confirm the corrected offsets land exactly on the real path. Checked bash and fish for the same units question: neither currently emits a9280;Sspan at all (their own candidates are already whole-word), so this bug class doesn't apply to them, and bash's separateCOMP_POINTcomputation was already byte-safe from an earlier round.bash:
COMP_TYPE=37(from the ninth round) regressedmake; reverted, cobra padding split insteadThe bash matrix verified the cobra fix worked but found
make-- the one script out of 841 in a stock bash-completion install that actually reads$COMP_TYPE-- regressed: it branches on$COMP_TYPEto choose a full directory-prefixed path (9) vs. just the next path component (anything else), somake sub/dir/+ Tab returneddeployinstead ofsub/dir/deployunder 37; since the bare component doesn't contain the typed prefix, the client's own filter discarded it and no menu appeared at all -- silently dead, one level into any prefixed target.Reverted
COMP_TYPEto 9 (faithful to real readline for all 841 stock scripts). To still avoid cobra's own padding, the padded"name (description)"shape is now split apart after the completion function returns -- a name, two or more spaces, then a parenthesised description to the very end of the entry -- into a bare name plus a real description, rather than avoided by changingCOMP_TYPE. This gets bash a real description channel for the first time, something neitherCOMP_TYPEvalue alone provided. Caught my own bug while implementing: a first version's regex used a greedy name group that absorbed some of cobra's own column-alignment padding (which varies per entry, e.g.checksvscheckout) into the "name" when there were more than 2 spaces; fixed by requiring the name group to end in a non-space character. Verified against realgh,git, and a realMakefilewith nested targets. Added amake-shaped fixture and a description-recovery assertion tobash_native_completions_test.sh, and wired it intoscript/presubmitso this can't regress silently again.PowerShell:
honor_ps1 = trueleaked the hex-encoded restore text on every requestRoot cause: PSReadLine's redraw always rewinds with absolute cursor addressing (CUP), but the matcher only trusted a CUP to column 0, deliberately conservative pending a real-prompt measurement. That measurement now exists: with a real 29-column prompt, every redraw addresses column 30 (0-based column 29) -- exactly the case the conservative rule declined to trust, converting a hypothetical wrong-position risk into a guaranteed leak whenever the prompt is non-empty. The same measurement gives the fix: PSReadLine always re-renders the entire buffer from its own start, so any CUP means "the buffer's own position 0" regardless of screen column -- column 1 with a zero-width prompt and column 30 with a 29-wide one were both measured to be the buffer's start, and even an "empty" prompt isn't column 0 (PowerShell substitutes its own
PS>fallback, measured at column 8). zsh, the only other line editor that matters here, was separately measured to never emit absolute cursor addressing at all for this restore, so widening the rule has no effect on it. Fixed by havinggoto/goto_colalways rearm position 0. Also handled: the echo is cumulative, not one-shot (a 12-character buffer re-echoes as an increasingly long prefix across 12 separate redraws, each with its own CUP) -- already correct, since every CUP occurrence rearms independently, confirmed with a new test.PowerShell: not yet fixed in this PR
Descriptions/matches containingFixed in the eleventh round below.;, BEL, or ESC corrupt the wire payload.$PSVersionTable.PS+ Tab showsPSCompatibleVersionsbeforePSVersion(the shell's own first/best match), and a raw wire order ofzeta, alpha, murenders asalpha, mu, zeta. This changes what accepting the top entry means, cross-shell, and needs a client-side fix (or an explicit product decision to keep it) rather than silence -- flagging for a follow-up.git checkout masputsmasteron the wire twice (bash) and the menu shows two identical rows. Also a follow-up, not fixed here.Deferred to a follow-up: committed-block-header corruption under
honor_ps1 = trueConfirmed, not just hypothesized: the header-corruption finding from the ninth round's PR body is the same mechanism as the wrapped-line leak, confirmed 3 for 3 at three different buffer lengths -- the missing character count from the header equals the preceding completions request's buffer length exactly (13, 14, and 11 characters missing for 13-, 14-, and 11-character requests respectively), corroborated from the unrelated wrapped-line measurement (
swallowed = columns − prompt_width, i.e. the window spans exactly the columns the echo occupies on the first row).Confirmed one-shot per request, not cumulative: re-measured with a fixed 24-character prompt across five request lengths (6, 11, 14, 18, 22 characters) -- each one's own missing-character count exactly matched its own request length (6, 11, 14, 18, 22 respectively), and two no-Tab controls in between returned the full, uncorrupted 24-character header. An earlier-looking
18→13→10→6→2progression across ascending request lengths was a testing-order artifact, not evidence of a shared, leaking counter. This matters for whichever of the two candidate fixes below gets picked: the fix needs a per-request cursor advance (each request's own swallowed-byte count applied once, to the one grid/header it affects), not a mechanism aimed at resetting some cumulative or leaked state between requests.Mechanism: characters swallowed by the expected-echo matcher (
consume_expected_echo's match, and the space-fill case added this round) never reach any grid'sinput(), so no grid's cursor tracking advances for them, even though the real terminal's cursor did move by that many columns.honor_ps1 = trueis the only mode affected, because that's the only mode where the shell's own prompt is streamed as literal characters with relative-cursor-based redraws (rather than delivered via a hook/DCS) -- a later prompt's own rewind, computed relative to what the real terminal now has, ends up misaligned against what this client's model has, since the model is missing the swallowed columns. Not fixed by the wrapped-line fix in this round, since that fix stops the leak but doesn't add the missing cursor accounting.Two candidate fixes, deliberately not implemented here (this PR is already well past its original scope, and
early_output.rshas been through seven rounds of fixes and multiple wrong attempts at one bound -- a structural change here belongs in its own reviewable change):EarlyOutputitself and apply it wherever a later prompt's own relative cursor math could be affected.pending_background_blockmechanism this file already has.Leaning toward (2): it likely also subsumes a separately-found defect (a wrapped buffer plus Tab can leave residue that Ctrl+U doesn't clear, merging two separately-typed commands into one block -- input corruption, not just rendering, on the same wrap path). The question that decides it -- does
pending_background_blockdefer rendering until the window closes, or render immediately and remove later, i.e. would (2) trade the scrollback artifact for a visible flash -- resolves in (2)'s favor: a background block is inserted into the real block list and starts processing real characters immediately on the first one (start_background), but stays at zero height (invisible) untilrender_delay_completeflips, ~100ms later (BACKGROUND_OUTPUT_RENDER_DELAY_MS) -- a delay that already exists for exactly this reason (to avoid a flicker when typeahead is briefly captured into a background block and then cleared before it would have rendered). A completions round trip finishing within that window, as it normally does, would mean the discarded block never had a chance to become visible.Also from this round's verification, not yet fully chased: the wrapped-line leak is not
honor_ps1-specific after all (it reproduces athonor_ps1 = falsetoo, clearing once a later command runs, vs. persisting permanently athonor_ps1 = true-- the fix should apply to both, but this distinction is worth the verifier re-confirming explicitly in both modes).Eleventh round: hex-encoding the match/description payloads (the
;/BEL/ESC class from the tenth round)This closes the one item left "in progress" at the end of the tenth round: OSC 9280's
C(match) andD?descriptionparams are;-delimited and the client reads only the third param, so a literal;inside a match or description truncated everything after it -- corrupting insertion, not just the menu (e.g. a file namedsemi;colon.txtwould insert as the unterminatedsemi). A BEL or ESC byte in the same text would end the whole OSC sequence outright, before the completions-parsing code even runs at all. Per guidance, this is fixed as a whole class (not just the semicolon case that motivated it), since BEL and ESC are equally reachable through the same filenames/tooltips.Fix: hex-encode both fields in all four shells, reusing each shell's own existing
warp_hex_encode_string/Warp-Encode-HexStringhelper (already used for JSON hook payloads and the in-progress buffer text, so this is the established idiom for exactly this problem, not a new one):compaddshim's match/description display loop._warp_native_bash_completions'sCOMPREPLYreply loop (after the cobra-padding split from the tenth round, so the split happens on plain text first, then both resulting pieces are encoded).warp_run_generator_command_native_completions'scomplete -Cloop.Alt+3handler'sCompletionMatchesloop.Client-side, added
decode_hex_completions_payload(ansi/mod.rs) to decode and UTF-8-validate the payload, degrading gracefully -- skip the match, or treat as no description -- on a missing, malformed, or non-UTF-8 payload rather than surfacing a wrong string. TheS(replacement span) OSC is untouched; it's just two decimal numbers, no text content, so it was never exposed to this bug class.Verification:
decode_hex_completions_payloadcovering;, BEL, ESC, multibyte text, and missing/malformed hex, plus OSC-dispatch-level integration tests inmod_tests.rsexercising the same cases end-to-end throughosc_dispatch../script/formatclean. As in every prior round,cargo check -p warp/appreliably OOMs in this sandbox, so this Rust change is hand-reviewed against the exactParamstype (&[&[u8]], confirmed by readingosc_dispatch's own signature and existing sibling code in the same file using the identicalparams.get(2).map(|osc_data| String::from_utf8_lossy(osc_data))pattern) rather than compiler-verified.bash_native_completions_test.shwith a semicolon-containing match (semi;colon.txt), and updated its OSC-payload collection helper to hex-decode before comparing against plain-text expectations (all 5 cases, including the pre-existing cobra/make ones, still pass).;, BEL, or ESC now arrives on the wire as pure hex digits ([0-9a-fA-F]only) and decodes back to the exact original text, confirmed by capturing and hex-decoding the actual emitted OSC bytes for each shell.Two nits from the crash-fix's in-app confirmation, folded in here:
Span::slice's doc comment attributed the UTF-16-to-byte-offset conversion to "the client-side conversion at the OSC boundary" when it's actually shell-side, inpwsh.ps1-- corrected. The replacement-span computation inpwsh.ps1could throw if a shell ever reportedReplacementIndex + ReplacementLengthpast the end of the line (Substringthrows, and the surroundingtry/catchwould turn that into a silent, warning-free empty completions response); not reproducible with any realCommandCompletioninput tried, but clamped to$line.Lengthdefensively anyway -- costs nothing, and a silent empty response is a worse failure mode than a slightly-wrong span. Verified:cargo fmt/cargo test -p warp_completer --lib meta(6/6, unaffected by the doc-only change), and a standalone pwsh script confirming the clamp no longer throws for an out-of-range input while leaving the normal case unchanged.Correction carried over from the crash-fix's verification: mid-line Tab completion is not a blanket no-op -- it works when the cursor sits at a token end. The "does nothing" cases found so far are all non-token-end positions (mid-token inside a path, before a closing quote, at position 0); this will be stated precisely (not as a general "mid-line completion doesn't work") whenever that investigation is written up.
Twelfth round: zsh path/value sub-token completions were silently inserting nothing (regression)
The zsh matrix proved by execution (Tab, then Enter, then checking what actually ran) that native completions inserted nothing for any multi-component path --
cd /et,ls /tmp/plain/xy,cat ~/.zsh,ls /workspace/warp/craall ran verbatim with no menu, despite the real shell genuinely having matches (cd /ethad previously worked on an older commit, confirming this was a regression from the span work rather than a pre-existing gap). Path completion is the single most common completion there is, so this outranked everything else outstanding.Root cause, confirmed by instrumenting the real
compaddshim in a live,compinit-initialized zsh session (not static reasoning):_path_files-- the completion function behind path arguments forcd/ls/cat/etc. -- restores$PREFIXto the entire remaining path before callingcompadd(e.g./tmp/somedir/etforcd /tmp/somedir/et, not justet), and reports the directory portion separately via compadd's-pflag (parsed by the shim into$hpreback in the ninth round, but never used) purely for display. The real match strings are bare basenames (etc). The ninth round's span computation used the whole$PREFIX, so it reported a span that no real candidate ever starts with (/tmp/somedir/etvs. a candidate ofetc) -- the client's own filter then discarded every candidate, which is indistinguishable from Tab doing nothing. The eight shapes the ninth round claimed to verify includedcd /etandcd /usr/lo, both of which go through this exact path -- that verification evidently didn't confirm actual insertion correctness against the real match text, only that some plausible-looking span was emitted.Fix: strip
$hpre(and$apre, from-P, handled the same way for symmetry though not directly reproduced) from the front of$PREFIXwhen it's a genuine prefix, before computing the span -- matching exactly what_path_filesitself excludes from insertion.Verification, against a real zsh 5.9 session,
compinit-initialized, driving the actualwarp_run_generator_command_foreground_completionsentry point end to end and capturing the real emitted OSC bytes:cd /tmp/somedir/et(2-level nested path): span corrected from(3,21)(covering the whole path) to(22,2)(covering justet) --etc/etceteranow pass the filter.cd /etagainst a real/etc: span(4,2), one matchetc-- the verifier's exact repro, now correct.ls /tmp/plain/xyagainst two real matching directories: span(14,2), bothxyz/xyzzypass.echo $HOM(a sub-token case that doesn't involve-p/-P, so$hpre/$apreare empty there): unchanged, span(6,3), still matchesHOME.ls --color=value completion (always/auto/never) is not a shell-side bug. The verifier also reported no menu ever appearing for the value afterls --color=+ Tab, even though the-Ssuffix itself (from the ninth round) landed correctly. Tested this exact case against the fix above: the shell-side OSC output is already correct on its own -- span(11,0)(a zero-length span right after the=, matching real zsh) and matchesnever/always/autowith empty descriptions, exactly mirroring real interactive Tab's own value listing. Since$PREFIXis already empty in this case,$hprewas never the problem here. If the app still shows no menu for this case, the bug is client-side (Rust), not in the shell script -- flagging back rather than claiming a fix for something not actually broken here.zsh -nsyntax check passes. Not verified in-app (no computer-use in this environment); requesting the verifier's zsh matrix be re-run on this commit, specifically re-confirmingcd /et,ls /tmp/plain/xy,cat ~/.zsh, andls /workspace/warp/cranow insert correctly end to end, and separately confirming whetherls --color=+ Tab still shows no menu (which would point at a client-side bug worth a dedicated follow-up).Thirteenth round: the wrapped-line leak is still present, and this round's fix is deferred rather than forced
Cross-platform re-verification found the ninth round's wrapped-line fix insufficient, and root-caused why in a way that points at a real gap in the matcher's design rather than a missed case it can patch around.
The ninth round's own test technique hid this: both that round's and this round's initial local re-check used repeating text (
'a' * N), under which the leak looked essentially fixed. With realistic, non-repeating text, the leak reproduces exactly as before the ninth round's fix: a stray block holding the tail, lengthtotal − W(the same rule as originally measured), reproduced on a real pty with a 97-character line at 80 columns. Noting this so the technique isn't reused uncorrected: any future local verification of this matcher needs non-repeating fixtures, since a repeating one can make an incorrect match look correct by coincidence (buffer[W] == buffer[W-1]for'a' * N, so a wrong position still "matches").Root cause, traced character-by-character against the matcher's own rules (confirmed independently on Linux and macOS): a wrapped redraw doesn't do one clean pass per row. It prints the continuation row's first character, then issues another carriage return and reprints the entire row again from its own start. On the exact measured byte sequence (
… mi+ space +\r+\x1b[K+k+\r+ke november oscar…): the space is absorbed by the existing fix; the first\rseeds candidate0(giving{0, 79}); the lonekmatches candidate79, advancing it to80(now expectingbuffer[80], which ise); the second\rseeds0again ({0, 80}); the reprinted row's first character iskagain -- butbuffer[80]ise, notk, because the reprint restarts from the row's own start (position 79), not from 80. Neither live candidate (0or80) expectsk, so this is a genuine mismatch, and it's exactly this reprint's first character that closes the window and leaks the rest -- one character earlier than where the visible leak actually starts, but the same underlying reason in every measured case:total − W.Why this isn't a small patch: the existing rearm rules (
rearm_at_column(0)on\r,rearm_after_rewind/advance_after_forward_movefor backspace/CUB/CUF with a known distance) all work because the distance or target position is either always 0 (full restart) or given directly by the escape sequence's own parameter. A mid-redraw\rthat restarts the current row doesn't carry its own row-start position in the byte stream at all -- that position (row_index * Wfor a fixed-width wrap, orW₀ + (row_index - 1) * Waccounting for the first row's narrower width under the prompt) has to be computed externally, from the terminal's column count and the column where the restore began, neither of whichEarlyOutputcurrently tracks or receives. Getting this wrong in either direction reintroduces exactly the failure modes the seven prior rounds on this file already fought through (either under-seeding, which leaks, or over-seeding, which risks matching wrong positions in genuinely different output). CUU/CUD were independently re-confirmed as no-ops for this specific mechanism, so this isn't a vertical-motion-tracking problem -- it's specifically about knowing where the current row starts in the pattern.What a real fix needs: thread the terminal's column count and the column where the restore write begins into
push_expected_echo(or a sibling call), then have\rseed a candidate at every row-boundary position implied by those two numbers (0,W₀,W₀ + W,W₀ + 2W, …) up to the pattern's length, rather than only0. This is new state and a new rearm rule, not an adjustment to an existing one -- exactly the kind of structural change the tenth round's deferred header-corruption fix already flagged as needing its own reviewable change rather than a same-round patch. Deferring this alongside that issue for the same reason, and because they may end up sharing a fix: both are consequences ofEarlyOutput's linear character-stream model having no notion of screen column/row at all, which the header-corruption issue's own two candidate fixes already grapple with.This must be fixed before
FeatureFlag::NativeShellCompletionsis promoted to any channel. A visible stray block containing a fragment of the user's own text is acceptable to carry behind an off-by-default flag during this PR's iteration, but is not acceptable to ship: it's a genuine, silent data-integrity issue in the user's terminal output, not a cosmetic rough edge.New, unreported this round: stray-block content appears to accumulate across successive requests rather than being replaced -- a later over-width restore produced a block containing the previous request's leftover leaked fragment concatenated with the new one. This is very likely a direct symptom of the leak above rather than an independent defect:
background_block_mut()reuses any existing, unfinished background block, and nothing currently finishes or clears it between two completions requests that both leak into it, so a second leak's fragment naturally appends to the first's. Recorded here rather than chased as its own fix, since it should resolve as a side effect once/if the row-start fix above lands -- worth the verifier explicitly re-checking once it does, rather than assuming.Confirmed working correctly on this round's re-verification, for the PR record: descriptions (
atuin29/32 non-empty,uvx --p8/10,starship pr3/3,--prerelease=keeping its=); multibyte replacement-span byte offsets (ls /tmp/café/xy→S;3,13, confirmed correct against a hand-encoded UTF-8 byte count); and the$hprefix above, independently re-derived and confirmed by a second measurement pass (cd /usr/lo→PREFIX=[/usr/lo]/hpre=[/usr/]/candidatelocal;cd ~/Doc→PREFIX=[~/Doc]/hpre=[~/]/candidateDocuments/;echo $HOM→PREFIX=[HOM]/IPREFIX=[$], nohpreat all, which is exactly why the$sigil case never needed this fix).Platform-specific note for CORE-3799: the committed-block-header mangling (tenth round's deferred finding) does not reproduce on macOS at all, at either short or long command lengths, which narrows that defect to Linux specifically -- worth carrying into whatever follow-up eventually picks up both deferred issues.
Fourteenth round: hex-encoder correctness/performance bugs, cobra splitter false positives, and control characters in accepted completions
Verification of the eleventh round's hex-encoding fix (bash matrix) found real bugs in the fix itself, plus one new, independent issue in accepted completion text.
BLOCKING: bash's and fish's hex encoder destroyed
-n/-e/-Ecandidates, and appended a trailing newline to every payloadwarp_hex_encode_stringpiped its argument throughechoin both bash and fish.echotreats an argument that looks like one of its own flags as that flag rather than literal text: measured,echo -nprints nothing,echo -e/echo -Eprint onlyecho's own trailing newline. This is not synthetic --kubectl -offers-n,ssh -offers-n/-e/-E, andnpm -offers-n/-E, all as real completions, so those exact flags either vanished from the menu entirely or encoded to a blank row. The sameechoalso appended a trailing newline to every payload, latent until now since nothing downstream happened to be sensitive to it.Fixed bash's encoder by rewriting it as a pure-bash, no-fork loop (
printf -vbyte by byte underLC_ALL=Cfor byte-safe indexing), which sidestepsechoentirely. Fixed fish's encoder by switchingechotoprintf '%s'(verified fish's builtinechohas the identical flag-swallowing bug;printfdoes not). Verified both against-n/-e/-E/multibyte text with the real installed interpreters, and added an exact-byte assertion on bash's encoder (warp_hex_encode_string "checkout"must equal636865636b6f7574, not that plus a trailing0a) so a regression here fails a test directly rather than several layers removed from the bug.bash's fork-per-payload encoder cost ~4x on a large completions result set
Measured directly:
echo | od | trforks two processes per hex-encode call, and a large result set (e.g.git checkoutwith thousands of branches, each match and description encoded separately) made that cost add up to something perceptible in the app. The same no-fork bash rewrite above eliminates this: benchmarked the encoder alone at 3656 calls (matching the reported scale) at ~4.47s before vs. ~0.19s after, and the full completions pipeline at the same scale at ~1.99s (down from an originally reported ~8.4s).The cobra description-splitter had two false-positive classes
Two independent bugs in the tenth round's cobra-padding splitter (
_warp_native_bash_completions):cmd (outer (inner) tail)) backtracked to the last multi-space run in the entry instead of the first, folding part of the description into the name.Backup (copy)-- so a lone such candidate got split too, silently discarding everything after the first(.Fixed the regex to structurally stop at the first multi-space run (by requiring the name capture group to consist only of single-space-separated tokens, never containing a 2+-space run itself), which needs no backtracking and can't prefer a later split point. Added a structural guard on top: cobra only pads a reply once it has more than one match, and pads every entry in such a reply -- so the split is now only applied when a reply has 2+ non-empty entries and all of them match the padded shape, never for a lone candidate that merely looks padded. Verified with new fixtures for both shapes (a two-entry reply where one description contains nested parens; a single real
Backup (copy)-shaped candidate) alongside the existing cobra/ordinary/makefixtures, all 10 passing.New: a literal control character in accepted completion text could get the input editor stuck
Independent of the wire encoding (which is correct now): once a payload contains a real newline (e.g. from a pathological but legal filename), accepting that completion inserted the literal newline into the buffer. This put the editor into a stuck multi-row state --
Ctrl+Uonly cleared the current logical row, repeated presses left stale content,Escapereduced it to one row, andCtrl+Cwas needed to fully clear -- and, sinceshould_use_native_shell_completionsbails on a multiline buffer, silently disabled native shell completions for the rest of that line. A BEL or ESC byte lands in the buffer the same way but is far less disruptive, rendering as a harmless box glyph rather than restructuring the editor.Treated as one decision about control characters in accepted text, not a newline special case: added
strip_control_characters(input.rs), applied to both completion-acceptance insertion paths (insert_completion_result_into_editorandinsert_completion_prefix_into_editor). Chose strip over escape: the input editor is shell-agnostic and doesn't know which shell-specific quoting would even make an escaped control character round-trip correctly if the command were actually run, whereas a completion candidate is sourced from the shell's own completion machinery (never typed or pasted by the user), so a raw control character in it was never something the user intended to insert -- unlike a deliberate multi-line paste, which the editor continues to support unmodified (this is scoped to the completion-acceptance path only). Added unit tests per control-character class: newline, carriage return, BEL, ESC, tab, DEL, and a C1 control, plus a check that surrounding multibyte text is left untouched.Verification:
bash_native_completions_test.sh10/10 passing (including the new-n, exact-byte, nested-paren, and lone-padded-filename fixtures); the bash encoder and cobra-splitter fixes verified directly against the real bash 5.2.21 interpreter and against realgh/Makefilefixtures as in prior rounds; fish's fixed encoder verified against a real fish 3.7.0 interpreter.bash -n/fish -nsyntax checks pass;cargo fmtclean oninput.rs.input.rs's change is hand-reviewed, not compiler-verified -- the fullappcrate reliably OOMs in this sandbox, as in every prior Rust change in this PR -- with particular attention paid toCow<str>borrow/move semantics inside theFnMutclosures it's used in, to avoid a move-out-of-captured-variable compile error.PowerShell in-app verification of this round's commit confirmed the fix at the byte level, not just visually: the stuck multi-row state is gone (a newline fixture now accepts to a single row, one
Ctrl+Uclears it, and completions keep firing on the same line immediately afterward), and the shell's own history file contains no C0, C1, or DEL byte anywhere across five submitted fixtures (newline, BEL, ESC, tab, DEL -- the last two weren't in the original report but behave identically, sincechar::is_controlcovers them uniformly). No regression on any fixture without a control character (semicolon,%20, unicode, space, apostrophe, including PowerShell's own doubled-quote escaping). Thehonor_ps1leak (tenth round) also remains fixed after the bash/fish encoder rewrites.One user-visible consequence of the strip decision, worth stating plainly, confirmed on both PowerShell and bash: stripping a control character out of a path changes what the accepted text names. A file that was genuinely selected from the menu (e.g.
nlline-x.txtwith an embedded newline on PowerShell,bel<BEL>ring.txton bash) no longer matches the real filename once the control character is stripped for insertion, so running the completed line fails -- PowerShell: a real but confusingCannot find path '...' because it does not exist; bash:cat belring.txt-> no such file, where real bash's own Tab inserts the literal byte and the command works. The strip decision stands (the failure this way is loud and local, versus escaping's alternative of the input editor guessing shell-specific quoting it has no way to get right), but do not describe control-character filenames as fully supported -- accepting one now fails loudly rather than corrupting the terminal, which is the tradeoff, not a fix for the underlying case. The better long-term answer is at display time (not offering a candidate the editor can't faithfully insert in the first place), not implemented here.Two more PowerShell behaviors characterized this round, both by design/explained rather than bugs:
handle_completion_suggestions_resultspicksUnselectedwhen classic completions are enabled andFirstotherwise, so the first Tab opens the menu with nothing highlighted and a second Tab selects the top entry. Worth writing down because "accept the top entry" being a two-Tab operation made several earlier reports read as though a single Tab sometimes did nothing.terminal.input.completions_open_while_typing = true) is reproducible and purely cosmetic -- it clears on the next keystroke and Enter runs the correct command regardless. Leading hypothesis (inference, not measurement): the function that applies completion results early-returns when the buffer or selection has changed since the request was issued, and that early return leaves the suggestions mode as-is rather than closing it, so a superseded request's menu is abandoned on screen rather than dismissed.bash in-app verification of this round's commit passed all six checks (with PowerShell's pass above, both shells whose paths changed under this round are now confirmed):
-n/-e/-Eare back everywhere:kubectl -has-n,ssh -runs-m, -n, -oconsecutively and includes-e/-E,npm -has both, and a dedicated-n/-e/-Efixture renders with no blank row anywhere.checkout->636865636b6f7574, no trailing0a); also confirmed the rewrite's multibyte handling is correct underLC_ALL=C--日本語.txtencodes per byte, byte-identical tood(e697a5e69cace8aa9e2e747874), not per code point, which would have been silently dropped client-side as invalid UTF-8 had the locale not taken effect.cmd (outer (inner) tail)alongside a second padded entry) splits at the leftmost run as intended, andcat Backagainst the realBackup (copy)file now completes to the full filename with both spaces preserved.make sub/dir/and a realgh pr cboth regress clean.New issue filed from this round's bash verification, CORE-3802: accepted completions are inserted without any shell escaping --
cat Backinsertscat Backup (copy)and hits a syntax error,cat semiinserts an unescaped;and splits into two commands, and a filename containing a space becomes two arguments, all against real bash ground truth that escapes each case correctly. This predates this PR (insertion has never escaped), but was partly masked for the;case specifically because the payload used to be truncated on the wire before reaching the line (fixed in the Eleventh round) -- fixing the wire truncation exposed the insertion-side gap more routinely. Same family as CORE-3800/CORE-3801 (client-side handling of results), and the issue notes that PowerShell's own candidates already arrive pre-quoted from the shell, so any future client-side escaping fix must not double-apply to them.Fifteenth round: zsh/Linux final verification, a trailing-space-after-
=fix, and a design cost noteThe zsh/Linux verification pass concluded, judging results by what actually executes rather than what the menu displays, across six rounds on five different heads.
The Twelfth round's
$hprepath fix is confirmed correct by executioncd /etrancd /etc(pwdconfirmed);ls /workspace/warp/cralisted the real crates;ls ~/.zfunresolved the tilde; multi-candidate cases now show correct menus where they previously showed nothing. Spans read directly from the production entry point all check out, including the multibyte case that exercises the Tenth round'sLC_ALL=Cfix:ls /tmp/utf8test/café/xy->S;23,2, where 23 is the correct byte offset on a 24-character line.Correction to an earlier report: "inserts nothing" was actually "menu opens unselected"
The zsh verifier's own earlier "path completions insert nothing" finding was wrong in its own diagnosis, not just imprecise: the real behavior is that the menu opens with no entry preselected, so pressing Enter immediately submits the raw, pre-Tab line, which looks identical to nothing having happened unless the menu itself is watched. Pressing Down first, then Enter, accepts normally. This is the same root cause PowerShell's verifier independently found and traced to source:
handle_completion_suggestions_resultspicksUnselectedwhen classic completions are enabled andFirstotherwise -- by design, not a bug, but it makes "accept the top entry" a two-Tab (or Tab-then-Down) operation, and it has now caused two independent verifiers to misreport a working feature as broken. Recorded prominently since a third misreport is exactly the failure mode this is trying to prevent.Fixed: a trailing space after a completion ending in
=broke hand-typing the valuels --col+ Tab correctly completes tols --color=(the-Ssuffix fix from the Ninth round), and typing=by hand and pressing Tab again correctly opens a workingnever/always/autovalue menu. But accepting the first completion (--color=) left the cursor one character past a trailing space thatinsert_completion_result_into_editorunconditionally appends unless the completion ends with a path separator -- so typing the value by hand afterward producedls --color= alwaysinstead ofls --color=always. Confirmed client-side, not shell-side: the shell's own OSC output (span,-Ssuffix) was already correct.Fix: extended the no-trailing-space exemption to also cover a completion ending in
=, matching the shell-side convention this exact shape follows (a trailing=means a value goes directly after it, with no space). Shell-agnostic and client-side, so it applies uniformly regardless of which shell produced the=-suffixed completion. Pushed asb80bfb2.cargo fmtclean; not compiler-verified for the same reason as every otherapp/src/...change in this PR.A design cost characteristic worth stating rather than leaving for a reviewer to discover
From reading
input.rs: the native-completions generator is dispatched whenever the feature is enabled, before Warp's own bundled spec results are consulted -- only which result set gets used is conditional, not whether the shell pays for a foreground completions round trip. So in the flag-only configuration, every keystroke pays for a foreground shell request even on a command where a bundled spec ultimately wins and the shell's own result is discarded. This is a real cost characteristic of the current design, not a bug, and is worth a reviewer knowing about rather than discovering independently.Downgraded and reconfirmed findings
zle-line-inithook installed by a plugin after Warp's own takeover fired 48 times in one round, including during a completions request itself, with no nesting error, and eight interleaved real commands all returned working prompts afterward.compdefand live-only alias both resolve, reconfirming results come from the live session.CORE-3799 and CORE-3801 updated directly on Linear with sharper detail from this pass
xABCDEfrom a 205-character line) -- 6 characters leaked on this head vs. 1 on an earlier head, consistent with an off-by-a-small-constant rather than a different mechanism.One remaining gap in zsh coverage, for whoever picks this up next
terminal.input.completions_open_while_typing = true(the as-you-type path) was never exercised on zsh in this pass -- every zsh result to date is Tab-triggered. This matters specifically for zsh because as-you-type would fire a foregroundselectin the user's live shell on every keystroke while typing, not just on an explicit Tab; whether that's viable at all is an open question this PR doesn't answer for zsh. macOS verification is covering this as of this writing.Sixteenth round: fish's fork-per-payload cost, and the root cause of the as-you-type request stream dying
The fish large-result-set investigation ruled out a match-count threshold entirely (
git checkoutrendered cleanly at up to 50,000 candidates pushed synthetically, withvte'sosc_rawconfirmed as an unboundedVec<u8>on native builds) and found something more serious instead: withcompletions_open_while_typing = true, no case renders a menu at all, independent of match count (git log --at 149 candidates andfunctionsat 321 both failed the same way).Fixed: fish's encoder still forked twice per payload, unlike bash's rewrite
fish's
warp_hex_encode_stringstill piped throughod -An -v -tx1 | command tr -d ' \n'(two forks), the same class of cost the Fourteenth round's bash rewrite eliminated there. Measured directly: forgit checkout, the shell side alone took 7.2s, of whichcomplete -Citself is only 0.137s -- almost the entire cost is these forks, at roughly 1ms per candidate (each match and description hex-encoded separately). At that latency, every as-you-type request is guaranteed to be stale before it returns, regardless of any other fix.fish has no byte-safe equivalent of bash's
LC_ALL=Cstring-indexing trick (fish strings are Unicode codepoints internally, not raw bytes), so a true no-fork rewrite isn't available the way it was for bash. Fixed by strippingod's spaces and line-wrap newlines with fish's own builtinstring replaceinstead of forkingtr, halving the fork count (one fork instead of two). Pushed as1a58623. Verified against-n/-e/-E, multibyte, semicolon, embedded-newline, and >16-byte (od-line-wrapping) inputs with the real installed fish 3.7.0 interpreter;fish -nsyntax check passes. This is a partial mitigation, not a fix on its own -- it roughly halves the cost, not eliminates it, and by itself doesn't make fish's as-you-type viable at scale. The remaining cost is structural, not a missing optimization: forkingodat all is what's expensive, and fish has no builtin path around that for raw bytes the way bash's locale trick provides -- the ceiling here is fish's own string model, not an implementation gap this PR left behind.A measurement-mode caveat: nearly every completions-behavior claim throughout this PR's rounds (the zsh/bash/fish/PowerShell matrices, span/description/suffix verification, etc.) was measured with explicit Tab triggering, not as-you-type -- the one exception called out in the Fourteenth round (the stale as-you-type menu) was already flagged as as-you-type-specific. This as-you-type dispatch gap didn't invalidate any of those Tab-triggered results, which are the bulk of this PR's verification, but it did mean none of them should have been read as claims about as-you-type behavior specifically. The gap itself is now fixed and confirmed live (see the Seventeenth and Eighteenth rounds below), so this caveat applies to reading the history up to this point, not to the current state of as-you-type dispatch.
Root cause found for the as-you-type request stream dying after 2-3 characters
Traced to
open_completion_suggestions(input.rs), which gates every as-you-type dispatch onis_command_grid_active() || is_cli_agent_shell_mode.Block::is_command_grid_active()is defined asself.state == BlockState::BeforeExecution-- true only in the window after a fresh prompt but before any command has started executing. The moment a native-completions request's own generator command begins executing as a foreground command (start_in_band_command_executiontransitions the active block out ofBeforeExecution), this gate goes false and stays false for as long as that generator command is running on the real shell.Since this check runs synchronously inside each keystroke's own edit-event handler, not on a timer or retry, every keystroke typed while a previous native-completions request is still executing silently no-ops at this gate -- no request is dispatched, and critically, nothing re-attempts the check later when the gate reopens, since only a new keystroke re-runs it. If the user's last keystroke in a typing burst lands while a request is still in flight, no request is ever issued for that final buffer state, even after the shell returns to an idle prompt moments later. This is a case of the completions mechanism gating on its own side effect: the check exists to avoid firing completions while a genuinely long-running user command occupies the foreground, but a native-completions generator command is itself exactly such a foreground command, so back-to-back requests during fast typing trip the same gate meant for unrelated long-running commands.
This also directly explains the staleness-guard symptom reported alongside it:
handle_completion_suggestions_results's exact-buffer-equality check (if buffer_text != editor_snapshot_when_completer_was_ran.text() ... return;) is a reasonable guard on its own -- a result computed for an older buffer genuinely shouldn't apply to a newer one -- but with no later keystroke able to fire a fresh request once the buffer moves ahead while the gate above is closed, a dropped-as-stale result is never followed by a replacement request for the current buffer either. Fixing the gate above should resolve both symptoms together, since a fresh request would then have a path to fire once the shell returns to idle, even without a further keystroke.Hypothesis confirmed before implementing, by a verifier instrumenting the gate directly (same 10 characters,
git log --, two runs each): slow typing (2s/character, 8 keystrokes) got the gate open and a request dispatched for 7 of 8 keystrokes; a fast burst of the same 10 characters got the gate open for only 1-2 of 8, with the rest blocked and no request ever issued for the final buffer. The predicted asymmetry held exactly. A further finding changed the fix's required scope: the gate also blocked a keystroke landing mid-burst during the slow runs, recovering only because the next keystroke happened to land after the gate reopened -- so a fix that only re-dispatches once at the end of a burst would still leave intermediate holes; it has to re-check on every return to idle.Seventeenth round: fixed the as-you-type request-stream gate, trailing-edge
Implemented direction 2 from the Sixteenth round, with the mid-burst finding folded in: on every return to idle (
AnsiHandlerEvent::Precmd-- the same hook that flipsis_command_grid_active()back totrue), check whether the buffer differs from the one the most recently dispatched as-you-type native-completions request was computed from, and if so, dispatch once more for the current buffer. Direction 1 (loosening the gate while a request is in flight) was deliberately not taken: it would mean writing to the pty during a foreground command, which is exactly the class of thing that produced the second-round phantom block, the fourth-round duplication regression, and the sixth-round write-queue race earlier in this PR. Direction 2 never fires during a command -- it only notices the world changed while it was busy -- which also matches the constraint that can't be removed: the request is synchronous and can take seconds.Added a new field,
native_completions_as_you_type_dispatch_snapshot, tracking the editor snapshot the last such dispatch was computed from; set inrun_completions_asyncright where a native-completions request is dispatched, and checked on everyPrecmdvia a newretry_as_you_type_completions_if_buffer_changed. The exact invariant specified for this -- compare against the last dispatched snapshot, not merely whether results are pending -- is what keeps it from looping: a successful retry updates the tracked snapshot to the buffer it was just dispatched for, so the very nextPrecmdsees no difference and does nothing unless the user typed more in the meantime. This also naturally coalesces a fast burst into one trailing request for wherever the buffer ended up, and gives the staleness guard (Sixteenth round) a replacement request instead of a dead end.Extracted the actual yes/no decision into a small generic function,
should_retry_as_you_type_completions, specifically so the loop-guard invariant could be unit tested without a liveEditorSnapshot(which has no public constructor outside its own module) -- three cases: no prior dispatch, an unchanged buffer (must not retry), and a changed buffer (must retry).Verification:
cargo fmtclean oninput.rs. The extracted decision function was verified via a standalonerustccompile outside this sandbox's memory-constrainedappcrate (all three cases pass) rather than as an in-treecargo test, since the app crate can't be built here; the rest of the change (the new field, themodel_eventssubscription wiring, and the dispatch-time tracking) is hand-reviewed against the exact existing patterns it mirrors -- the siblingTerminalModeSwappedsubscription arm and theeditor.read(ctx, |view, ctx| view.snapshot_model(ctx))idiom already used elsewhere in the same function -- rather than compiler-verified. Not verified live; requesting the verifier who instrumented the gate re-run both the fast-burst and the mid-burst-during-slow-typing experiments against this commit to confirm the blocked keystrokes stop disappearing.Eighteenth round: the retry guard was comparing the wrong thing
Live verification of the Seventeenth round's fix confirmed the core mechanism -- the gate itself was never loosened, the retry rescued both the fast-burst and the mid-burst-during-slow-typing cases exactly as designed, and 90 seconds hands-off with the buffer unchanged produced zero visible activity (47 retries correctly suppressed by the loop guard) -- but found a real defect in what the guard compared.
The defect: of 23 retries logged in one session, only 5 had an actually-different buffer. The other 18 fired with identical text and identical selections and dispatched a redundant, duplicate request for the buffer that had just been dispatched a moment earlier. Root cause: the guard compared whole
EditorSnapshotvalues, andEditorSnapshot's own derivedPartialEqcompares more than text and selections -- it also comparesbuffer_text_runs, an internal representation that a completions round trip perturbs (most plausibly because the PTY controller writes the buffer text back to undo the foreground command's own buffer-clearing) without changing anything visible to the user. Two snapshots that looked identical still compared unequal.Severity was bounded rather than a loop -- it fires exactly once per dispatch, and the very next
Precmdis correctly guarded, since by then the snapshot really hasn't changed again -- but it added one redundant full foreground generator command for whichever buffer happened to be in flight when a round trip perturbed the snapshot (not specifically the largest one in a burst), 18 of them across a 124-request session -- about 15% of all as-you-type shell work in that session going to duplicate requests that never should have fired. Cheap at 277ms for a small result set, not cheap at 6.3s for a large one.Fix: introduced
AsYouTypeCompletionsBufferState, holding only the two fields that actually matter (text,selections), built fromEditorSnapshot's own public accessors. The retry guard now compares that narrower type instead of the wholeEditorSnapshot, so the round trip's perturbation ofbuffer_text_runsno longer produces a false "changed" result.should_retry_as_you_type_completionsitself is unchanged and was already correct -- the defect was entirely in what got passed to it.The test gap that let this through, and how it's closed: every existing test for
should_retry_as_you_type_completionsinstantiated the generic function withString, so none of them ever exercisedEditorSnapshot's real equality behavior -- exactly the verifier's point that an assertion has to bind to the type actually used in production. Addedas_you_type_completions_buffer_state_tests, constructingAsYouTypeCompletionsBufferStatedirectly with realstring_offset::CharOffset/vec1::Vec1values (matching this file's own existingVec1::newusage elsewhere), including the exact case that would have caught the regression: identical text and cursor position must not trigger a retry.Pushed as
8c2858c. Verification:cargo fmtclean. The new tests are hand-traced againstAsYouTypeCompletionsBufferState's derivedPartialEq(structural equality over types already known to be correctly comparable) rather than compiled and run -- the app crate still OOMs here.Confirmed live on
8c2858c, closing out the as-you-type work: redundant retries went from 18 to 0 in the same session shape (23 retries total -> 4, all 4 genuine), the fast-burst and mid-burst-during-slow-typing rescues still work identically to the Seventeenth round (a fast burst still blocks most gate calls and still opens the menu; the forced mid-burst case at 8,112 candidates is still rescued a second later with no further keystroke), and the loop guard still holds over 40 seconds hands-off with exactly one guard skip per retry. No burst ended without a menu. This closes the as-you-type request-stream investigation; the as-you-type mode can now be treated as verified for the mechanism this PR is responsible for (dispatch reliability), separate from the smaller cosmetic stale-menu issue noted above, which remains open.Nineteenth round: macOS as-you-type sweep -- confirmed the path fix, found a new, orthogonal intermittency, added observability
A macOS sweep of
8c2858cacross bothhonor_ps1settings confirmed the Twelfth round's$hprepath fix (cd /et->cd /etcwith the leading/preserved, not thecd etcmis-insertion originally feared) and descriptions (atuin,starship pr). Also confirmed: no plugin-hook wedge, Ctrl-C/resize mid-request harmless, no generator commands in history, multibyte intact, and block headers are well-formed on macOS -- the CORE-3799 mangling is Linux-specific, narrowing that issue further. The wrap leak (CORE-3801) reproduces exactly as expected and needs no new action.New finding: the as-you-type mid-burst rescue is intermittent under
honor_ps1=false, and it's a different mechanism than the Sixteenth-Eighteenth rounds' fixSame case, same head, both outcomes observed: after a fast
git log --burst, the rescue fired once, then didn't fire across four consecutive retries at 15s/30s. The failing stretch clustered specifically in thehonor_ps1=false(Universal Developer Input) configuration and specifically on shell-only cases (git log --,cd /et,echo $HOM,foo=/tmp/,cd ~/Doc,zqfoo), whilestarship pr/atuinkept working throughout -- i.e. the same command intermittently produces no completions menu at all, with the variable being when it's typed, not what is typed.Investigated the verifier's hypothesis (offered as unproven) by reading the code rather than guessing:
should_use_native_shell_completionsexcludes AI-mode input (input.rs:1542-ish,!is_ai_input), and in the Universal input box,honor_ps1=falseuses per-buffer natural-language detection (NLD) to decide AI-vs-Shell mode -- confirmed from the block-header text they observed (~ (nld overridden)).Confirmed by code reading:
BlocklistAIInputModel::detect_and_set_input_typeruns its classification asynchronously (ctx.spawn, debounced) and can flipinput_typeat any time relative to a completions round trip. Both the original as-you-type dispatch (open_completion_suggestions, on a keystroke) and the Seventeenth round's trailing-edge retry (retry_as_you_type_completions_if_buffer_changed, onPrecmd) route through the samerun_completions_async, which re-readsself.ai_input_model.input_type()fresh at call time rather than caching a value from when the buffer was first typed. So yes, the retry is gated behind the identical decision, and it's more exposed to the classifier's async timing than a single one-shot dispatch would be, since it deliberately re-evaluates state at a later point in time specifically to catch up on what changed. If NLD's async verdict flips a shell buffer to AI mode in the window between an original dispatch and a later retry, native shell completions silently stop being used for that exact buffer -- which is consistent with the observed "same command, different outcome" pattern.Confirmed by code reading, answering the verifier's second question: neither a gate-blocked dispatch (
is_command_grid_activefalse) nor an AI-mode-classification skip (use_native_shell_completionsfalse viais_ai()) had any logging at either site before this round -- both were equally silent, which is exactly what made this hard to see from the outside.Not confirmed, and flagged rather than guessed at: why
starship pr/atuinwere unaffected while the other cases went quiet in the same session. This depends on the actual runtime verdict of the NLD classifier (heuristic/ML-based), which isn't something traceable from the call-site code alone -- I don't have a theory that explains that specific asymmetry, and didn't want to invent one.Fixed: added observability, not a classifier-interaction fix. This intermittency is a property of the pre-existing
is_ai()exclusion interacting with async classification, not a bug introduced by the retry work -- resolving it would mean a design decision (e.g. freezing completions eligibility for the duration of a round trip, or having NLD defer to an in-flight completions request) that's out of scope here. Added alog::debug!line (pushed as4fc7e35) firing specifically when AI-mode classification is the sole reasonuse_native_shell_completionsis false, distinguishable by construction from every other skip reason (feature disabled, shell unsupported, multiline buffer) checked in the same condition. No buffer/command text is logged, per this repo's logging guidance for a hot per-keystroke path and for user-generated command content. Verification:cargo fmtclean;log::debug!/log::warn!already used extensively elsewhere in this same file, confirming no new import is needed. Not compiled (app crate OOMs) and not verified live. Requesting the verifier enable this log level and re-run a failing case to directly confirm (or rule out) the hypothesis, and separately look atlast_ai_autodetection_source()/the classifier's own decision for thestarship/atuinasymmetry, since that's outside what I can determine from static code reading.Also reported:
uvx --pshows no menu despite the shell returning 10 matches (8 with descriptions)Reproduces on both
honor_ps1settings, unlike the intermittent case above. Not yet root-caused -- may be the same AI-mode classification issue (worth checking with the new log line first) or a separate cause. Flagging for the next verification pass rather than guessing further without being able to run it.