Support bindkeys #537#9847
Conversation
Captures desired behavior for honoring user-defined zsh, bash, and fish keybindings in Warp's input editor: live-shell discovery, best-effort widget coverage, precedence (user-customized Warp > shell bindkeys > Warp defaults), per-tab independence, and graceful fallback when widgets are unsupported. Open questions are surfaced inline for the tech spec to resolve. https://claude.ai/code/session_01AbtuqGqnwn4X9yo2jdQAs5
Plans the implementation grounded in current Warp subsystems: bootstrap-side binding query via a new DProtoHook::ShellBindings DCS variant (zsh/bash/fish), per-tab ShellBindings storage on the Shell struct, fine-grained widget-to-InputAction expansion, a third shell_bindings tier in Keymap with precedence user-Warp > shell > defaults, plus settings toggle, feature flag for staged rollout, and telemetry. Resolves the open questions carried from product.md. https://claude.ai/code/session_01AbtuqGqnwn4X9yo2jdQAs5
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @claude, @th1nkful on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
github-actions
Bot
added
the
external-contributor
|
I'm starting a first review of this spec-only pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
|
@cla-bot check |
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @claude on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
The cla-bot has been summoned, and re-checked this pull request! |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR adds product and technical specs for honoring zsh/bash/fish shell keybindings in Warp's command input editor. The specs cover the broad user-facing behavior, rollout, settings, debug UI, parser work, and validation strategy.
Concerns
- String bindings with embedded newlines are specified in a way that does not match shell input-queue behavior.
- The bootstrap/DCS design leaves a new keymap-control trust boundary undefined.
- The dynamic re-query mechanism is under-specified and relies on an env-var trigger the app cannot set inside an already-running shell.
- Fish support does not account for Warp's existing fish bootstrap code that rewrites key bindings during precmd.
- Vi-mode tracking via prompt hooks only will miss mode transitions caused by keystrokes handled entirely inside Warp's editor.
Security
- The new ShellBindings DCS payload can change local key handling, so the spec needs source/session validation and rejection limits before implementation.
- Telemetry for unsupported widget names must account for user-defined widget names, which can be arbitrary/private rather than well-known shell vocabulary.
Verdict
Found: 0 critical, 6 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
Six review comments, all important: 1. product.md warpdotdev#9: bindkey -s injects into the shell input queue, so a newline in the bound string accepts the line — clarified to match shell semantics rather than describing literal text insertion. 2. product.md warpdotdev#11 + tech.md telemetry: user-defined widget names can be arbitrary or private. Telemetry now only sends widget names from a documented shell-vocabulary allowlist; user-defined names redact to the bucket "user-defined". Allowlist co-located with the parser as single source of truth. 3. tech.md: ShellBindings DCS is a privileged terminal-control payload. Added a trust boundary — per-tab WARP_BOOTSTRAP_NONCE embedded in every payload, size caps (256 KiB total, 4 KiB per entry), strict schema validation, no partial application. Prevents arbitrary process output from spoofing bindings. 4. tech.md re-query: original flow was not implementable (the app can't flip an env var inside a running shell). Reworked so the bootstrap caches its own __warp_bindings_hash and decides shell-side when to re-emit; the app is consume-only. Preserves PRODUCT warpdotdev#26/warpdotdev#27. 5. tech.md fish: current bootstrap resets fish_key_bindings and installs four Warp binds on top, which would clobber user vi mode. New plan stops the reset, lets the user's scheme run, then reinstalls only Warp's four reserved binds explicitly per mode — resolves the original input-reporting conflict without losing user bindings. 6. tech.md vi mode: precmd-only mode tracking misses transitions that fire inside the editor (Esc, gg, etc). Added an in-app vi mode state machine driven by dispatched widgets, with shell-reported mode treated as initial state and resync. https://claude.ai/code/session_01AbtuqGqnwn4X9yo2jdQAs5
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @claude on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR adds product and technical specs for honoring zsh, bash/readline, and fish shell key bindings in Warp's command input editor. The specs cover the intended lifecycle, precedence model, parser/matcher architecture, settings, telemetry, and validation plan.
Concerns
- The DCS nonce security model relies on a value placed in the shell environment, which child processes can read, so hostile terminal output could still forge privileged binding updates.
- The tech spec does not preserve the product requirement for string macro bindings to re-enter the key-resolution stream; it describes direct literal buffer insertion instead.
- The setting toggle behavior is incomplete: the product requires toggling back on to re-query each tab, but the tech plan only disables the matcher tier and explicitly avoids app-triggered re-query.
- The zsh query plan omits user-defined keymaps even though the product says they are in scope.
- The fish bootstrap plan intentionally overwrites user bindings on Warp-reserved keys without reflecting that exception in the product precedence model.
Security
- The privileged
ShellBindingsDCS channel needs a trust-boundary design that cannot be spoofed by process output from the PTY. An exported environment nonce is not sufficient.
Verdict
Found: 0 critical, 5 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
Five round-2 review comments, all important: 7. tech.md DCS nonce: env-var-only is insufficient because same-uid processes can read /proc/<pid>/environ. Bootstrap now copies the nonce into a non-exported shell-local variable as its first action, then unsets the env var so descendants cannot read it. Threat model documented explicitly: defends against innocent DCS-in-output and later-spawned descendants, not against same-uid attackers who can already inject keystrokes via TIOCSTI / modify rc files. 8. tech.md string macro contradicts PRODUCT warpdotdev#9: literal buffer writes skip key resolution. Reworked widget enum to distinguish SelfInsert (single-char), Macro(String) (re-injected through the input pipeline so newlines submit and control chars trigger actions), Action, and Unsupported. Added a per-macro length bound to prevent bind-cycle loops. 9. tech.md toggle re-query: original PRODUCT warpdotdev#24 said toggling on re-queries each tab; tech said re-queries are shell-side only. The technical reality is shell-side; PRODUCT warpdotdev#24 is updated to match — toggle on resumes from cached table, drift picked up on next precmd. Pressing Enter on an empty line is the user-visible escape hatch. 10. tech.md zsh keymaps: PRODUCT warpdotdev#2 includes user-defined zsh keymaps but tech only enumerated a fixed list. Bootstrap now uses `bindkey -l` to discover all keymaps including any user-defined ones, then dumps each. 11. tech.md fish reserved keys: \cP, \ep, \ew, \ei conflict with PRODUCT warpdotdev#14's clean precedence rules. Documented as an explicit product-level exception in PRODUCT warpdotdev#14 (fish-only, structural because fish integration uses bind rather than precmd hooks); user bindings on these keys are tagged reserved-by-warp in the debug view. Lifting the exception is a tracked follow-up. https://claude.ai/code/session_01AbtuqGqnwn4X9yo2jdQAs5
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @claude on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec PR defines product and technical plans for honoring zsh, bash/readline, and fish shell bindkeys in Warp's input editor. The scope is generally aligned with issue #537 and includes lifecycle, precedence, rollout, telemetry, testing, and security considerations.
Concerns
- Dynamic rebind timing is internally inconsistent: the product spec promises the next keystroke after declaration completion, while the tech design and performance invariant allow a late async re-query.
- The reserved-key model omits existing zsh/bash bootstrap bindings that are currently installed and reset by Warp, so the proposed fish-only exception does not match today's architecture.
- The DCS spoofing mitigation overstates nonce secrecy for fish because fish user config can run before the
--init-commandbootstrap can unset an exported nonce. - The keymap integration sketch puts terminal-specific binding state into
warpui_corewithout specifying how crate boundaries and action ownership are preserved. - The setting-toggle validation contradicts the proposed no-active-requery behavior.
Verdict
Found: 0 critical, 5 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
Five round-3 review comments, all important: 1. PRODUCT warpdotdev#4 (async timing): the "next keystroke after declaration" guarantee conflicted with warpdotdev#26's non-blocking guarantee. Reworded so the invariant is "the first keystroke after Warp has parsed the next ShellBindings payload from that prompt", with the small async window made explicit. Declarations never block typing. 2. PRODUCT warpdotdev#14 (zsh/bash reserved keys): claim that only fish has reserved keys was wrong — current zsh and bash bootstraps install binds for ^P/\C-p, \ei, \ep, \ew. Listed the reserved set per shell explicitly (zsh: 2, bash: 4, fish: 4); user bindings on those keys import as "reserved-by-warp" and don't fire. 3. tech.md fish nonce: fish runs config.fish before --init-command, so an env-var nonce is readable to user code. Added a fish-specific path: nonce written to a 0600 tempfile under the runtime dir, path passed via --init-command, bootstrap reads then rms the file. WARP_BOOTSTRAP_NONCE env var not used for fish at all. 4. tech.md warpui_core layering: ShellTabBinding/ShellBinding can't live in the UI core. Reworked: warpui_core gets a generic set_contextual(scope_key, bindings) API only; the terminal layer (new keymap_bridge.rs) translates ShellBinding -> Binding with a ContextPredicate::TabIs predicate and BindingOrigin::Shell tag. Resolution order is enforced by predicate evaluation + origin tag, not by a new tier-typed Vec. 5. tech.md toggle test: now asserts cached-table reuse on toggle-on plus next-precmd refresh, matching the relaxed PRODUCT warpdotdev#24 from round 2. https://claude.ai/code/session_01AbtuqGqnwn4X9yo2jdQAs5
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @claude on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
/oz-review |
There was a problem hiding this comment.
Overview
This spec proposes importing zsh/bash/fish keybinding tables into Warp's shell input editor, with product invariants for discovery, precedence, modes, settings, and telemetry plus a technical plan using bootstrap-emitted DCS payloads and a new shell-binding keymap tier.
Concerns
- The matcher integration relies on a dynamic per-tab context predicate that the current keymap context model cannot represent as written, so multi-tab isolation and shell-tier precedence are underspecified.
- The multi-key fallback behavior promised by the product spec is not implementable by simply reusing the current matcher prefix logic because the matcher drops pending buffered keys on mismatch instead of replaying them.
- The shell-side requery plan does not require preserving last command status or other shell state around prompt-hook binding dumps, which conflicts with the product invariant that the query has no visible shell-state side effects.
Security
- Diagnostics for unsupported bindings are allowed to log raw widget names and key sequences even though the spec recognizes user-defined widget names can be private; diagnostics need the same redaction boundary as telemetry, or an explicit local-only/non-uploaded guarantee.
Verdict
Found: 0 critical, 4 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
Four round-4 review comments, all important: 1. PRODUCT warpdotdev#11 diagnostics: applied the same allowlist/bucket redaction to diagnostics as telemetry, since "local-only" is not guaranteed forever. User-defined widget names log as "user-defined"; bound key sequences are not included in diagnostics at all. 2. tech.md precmd state preservation: spelled out the discipline needed to keep PRODUCT warpdotdev#27. Hash function saves $?/$status first thing, never touches set -o / setopt / shopt / KEYMAP / fish_key_bindings, uses local/__warp_-prefixed temporaries, avoids subshells where it can capture $? before/after where it can't, uses \bindkey / builtin bind to bypass user aliases, and does not modify any DEBUG trap. An integration test asserts $?/options/KEYMAP/non-__warp_ vars all unchanged across precmd. 3. tech.md ContextPredicate: TabIs(u64) wasn't expressible because the existing predicate is &'static str only (warpui_core/src/keymap/context.rs:10-17). Reworked: tab scoping moves to a storage tier, not into ContextPredicate. New API: ScopeKey { category, id } + set_contextual / clear_contextual / set_active_scopes. Matcher iterates only over active scopes; bindings still use the existing predicate for in-scope conditions. 4. tech.md matcher prefix replay: the current matcher returns None and clears pending state on mismatch (matcher.rs:258+), so buffered prefix keys are dropped — that contradicts PRODUCT warpdotdev#8 (replay on abandoned prefix). Spec'd an explicit API change: MatchOutcome::AbandonedPrefix(replay, current); legacy callers wrap via a match_or_replay() helper that flattens to today's behavior. https://claude.ai/code/session_01AbtuqGqnwn4X9yo2jdQAs5
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @claude on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
@oss-maintainers — bringing this in for a maintainer pass per CONTRIBUTING.md. Have used the three /oz-review rounds and would appreciate a human review on the spec. Recap of the rounds: The spec is now grounded in real file refs (crates/warpui_core/src/keymap/context.rs:10-17, matcher.rs:258+, app/src/terminal/local_tty/shell.rs:632, app/assets/bundled/bootstrap/{zsh_body,bash_body,fish}.sh). Open questions (rollout default-on vs flag, AI prompt opt-in, user-defined widget forwarding) are explicit and tagged for maintainer input. |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec proposes honoring zsh/bash/fish shell bindkeys in Warp's input editor through bootstrap-emitted binding tables, per-tab state, keymap integration, settings, telemetry, and staged rollout.
Concerns
- The product spec contradicts itself on binding precedence, which could lead implementers to check shell bindings before explicit Warp user overrides.
- Multi-key sequence behavior is underspecified for prefix timeouts and ambiguous bindings, creating a risk of swallowed or delayed input.
- The tech spec's vi-mode model omits states that the product spec and transition table require.
- The proposed keymap origin layering is internally inconsistent about whether
warpui_coreowns shell-origin metadata.
Security
- The privileged DCS payload validation policy conflicts between dropping oversized entries and rejecting malformed payloads, and it does not pin down where size limits are enforced before allocation/parsing.
Verdict
Found: 0 critical, 5 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
Splits the bash handler's `else` branch from "append to READLINE_LINE" to "insert at READLINE_POINT and advance the cursor past the inserted content", matching default bash bracketed-paste behavior for user-driven pastes. The Warp-sync branch (sentinel match) is unchanged since the mirror is the complete buffer state and the cursor lands at end anyway. Adds a bootstrap-time assertion that `__warp_paste_sentinel_len > 0` so an accidentally-unset sentinel length doesn't silently match the empty prefix on every paste and leave sentinel bytes in the buffer. On assertion failure the tab falls back to the same no-Cat-C-dispatch path as bash `enable-bracketed-paste off`.
§6 architectural premise's batched-mode paragraph no longer claims "inject the mirror byte-for-byte" (which §6.2.5 explicitly retired) or lists `fish_postexec` / `fish_prompt` as fish's primary emit path (the wrapped `__fish_paste` is the paste-time entry point; the postexec/prompt hooks emit only coarser fallbacks). §6.2.5 sentinel-format paragraph drops the "21 bytes" magic number and explicitly stores both `__warp_paste_sentinel` and `__warp_paste_sentinel_len` at bootstrap, mirroring what the handler bodies already do.
Adds four explicit tests for the §6.2.5 paste-sync mechanism so the indirect coverage via the atuin pass-through test isn't the only assertion of the path: - "Literal mirror sync — round-trip" exercises the newline-in-buffer case that motivated the literal-paste detour over byte-by-byte injection. - "Literal mirror sync — sentinel collision" pins the nonced-sentinel invariant against a regression that would drop the nonce. - "bash bracketed-paste-disabled fallback" covers detection, toast, debug-view label, fallthrough, and mid-session re-enable. - "fish __fish_paste composition" covers the composability claim with a runtime-installed user wrapper. Also fixes a stale §7.1 reference in the latency-budget test to point at §7.3 where the budget actually lives.
Resolves the TBD on the lock-mode default keystroke. Default is now `Ctrl-Alt-L` (user-rebindable), chosen because `Ctrl-Shift-L` clashes with terminal-selection shortcuts on macOS/Linux. Both TECH §22.5(d) and PRODUCT warpdotdev#22.5(d) name the value, and PRODUCT "Open questions" list the resolution explicitly.
PRODUCT carried four "Open question" markers (warpdotdev#11, warpdotdev#13, warpdotdev#22, warpdotdev#23) that TECH has answered concretely. Each is now annotated "Resolved" with a one-line summary of the resolution pointing at the TECH section that closed it. The bottom-of-document Open Questions list mirrors the same updates. warpdotdev#22 stays scoped: v1 has no AI-prompt opt-in. The opt-in plus its dependent ClassifierGate (warpdotdev#22.5) remain a tracked follow-up that must ship together.
All items in the section are now Resolved; the intro reads oddly with "tech spec must resolve" in active tense. Reframed as "the spec iteration resolved" with a note that the section indexes v1 design decisions for reviewers.
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec defines a broad product and technical design for honoring zsh, bash/readline, and fish bindkeys in Warp's shell input editor, including external widget pass-through, per-tab binding tables, inline plugin rendering, settings, telemetry, rollout, and validation.
Concerns
batchedpass-through explicitly moves the shell cursor to end-of-buffer, which contradicts the product invariant that Category C widgets start with the user's current cursor position and can break cursor-sensitive widgets.- Fish abbreviation expansion on space is claimed as supported, but the
batchedflow only syncs to fish on Category C dispatch or Enter, so ordinary space does not reach fish in time to expand. - The bash/fish wrapper construction still splices user-controlled binding bodies into generated shell source, leaving a wrapper-escape/execution-timing gap the spec should close before implementation.
- The v1 validation section includes AI prompt classifier tests even though the same spec marks that opt-in as a follow-up.
Security
- The wrapper construction for bash/fish bind bodies needs a safer design than sourcing generated functions containing raw user-controlled command bodies; otherwise malformed or hostile bind bodies can escape the wrapper or execute during bootstrap rather than only on the bound key.
Verdict
Found: 0 critical, 3 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
PRODUCT warpdotdev#11.5 narrows the cursor invariant for v1: cursor lands at the pre-dispatch position on `full`-mode shells (zsh, bash with blesh) where every keystroke has already reached the shell; on `batched`-mode shells (vanilla bash, fish v1) the cursor lands at end-of-buffer because the literal-paste sync has no keymap-independent positioning primitive. The v1 motivating widgets (atuin, fzf, edit-command-line) operate on content not cursor; cursor-sensitive widgets that depend on the gap are tracked as a follow-up. Resolves Oz's PRODUCT/TECH cursor contradiction. §6.1 batched mode gets a fish-specific space-trigger sync step (step 5): on fish tabs, space (`U+0020`) is treated as a sync boundary so fish's `abbr` engine can fire natively on space. Cost is one round-trip per space keystroke on fish tabs only. Restores the PRODUCT warpdotdev#11.6 fish-abbr-on-space invariant which the previous batched flow (sync only at Cat C dispatch or Enter) silently violated. §6.3 bash and fish wrapper installation now have an explicit pre-wrap validation step: `bash -n -c "fn() { $body }"` (or `fish --no-execute -c "function fn; $body; end"`) parses the body inside the function-body context and rejects anything that escapes — extra `}`, dangling heredoc terminator, dangling `end` in fish, etc. Rejected bodies leave the original `bind -x` / `bind` intact and skip the Warp wrapper. Combined with a 64 KiB length cap and a `__warp_*` reserved-namespace check, validated bodies are *structurally* guaranteed to execute inside the wrapper's function and nowhere else. Closes Oz's [SECURITY] concern about wrapper-escape. Testing section: the four warpdotdev#22.5 classifier tests move to a new "Follow-up validation (with warpdotdev#22 opt-in)" subsection, since warpdotdev#22/warpdotdev#22.5 are tracked as follow-ups (TECH §"warpdotdev#22") and not in v1 scope. Keeps the v1 test plan honest about what's required to land this PR vs what's queued for the AI-prompt follow-up.
The forbidden-token check claimed to use "the same `bash -n` parse tree" — but `bash -n` doesn't expose a parse tree to the caller, so the check was unimplementable as described. v1 drops the forbidden-token check entirely; `bash -n` (and fish's `--no-execute` equivalent) is the entire structural-escape defense, which is what actually matters for wrapper safety. The section now explains why semantic concerns (`return`, `exec`, `__warp_*` writes) are documented rather than enforced: none of them are wrapper-escape vectors at the shell level. Validation command construction now uses `printf -v` (bash) and `printf ... ` (fish) so `$body` isn't spliced into a double- quoted string where embedded `"` (any body with quoted strings) would break the outer quoting and yield a malformed validation command. §6.1 step 5 clarifies the mirror-at-sync state: the just-typed space isn't in the mirror at sync time (it's intercepted before the local SelfInsert that would append it); Warp syncs the pre-space mirror, injects space, then resyncs the post-space buffer (which includes the space plus any abbreviation expansion fish performed).
Fish validation now pipes the constructed script to `fish --no-execute` on stdin. The previous form, `set -l __validate_src (printf '...')` followed by `fish --no-execute -c $__validate_src`, was broken by fish's command-substitution semantics — `(...)` splits stdout on newlines into separate elements, and `-c` only reads the first. A stray `end` in the body would have passed validation and escaped the wrapper at install time. Piping to stdin keeps the script as a single contiguous source. Bash validation paragraph now states the outer loop scaffold (iterate over parsed `bind -X` output) so the `continue` in the example has visible context. Adds a "Validation cost" note covering the per-binding fork+exec overhead and the long-lived `coproc` validator as the amortization path if a real shell stack pushes wall-clock validation cost over a perceptible threshold.
Reorders bash validation steps so the O(1) length cap runs
before the fork+exec `bash -n` spawn — a pathological 1 MiB body
gets rejected immediately rather than paying for parser
instantiation. Same reorder applied symmetrically to fish.
Drops the half-correct `coproc __warp_bash_validator { bash
--noprofile --norc; }` example body since raw `bash --noprofile
--norc` reads commands from stdin, not bodies-to-validate. The
amortization story stays in prose ("a long-lived validator
subshell that reads bodies on stdin and emits per-body verdicts
on its stdout") without asserting an implementation-specific
protocol the spec doesn't need to pin.
Fish gets the same amortization paragraph pointing back at §6.3
bash, noting fish lacks `coproc` but the FIFO-backed equivalent
is straightforward.
§6.2.5 intro listed only Cat C dispatch (§6.1 step 3) and Enter (§6.1 step 4) as callers of the literal-paste path, but step 5 (fish space-trigger sync, added in the Oz round-7 fixes) is also a caller. Reader of §6.2.5 alone now sees the complete set.
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec PR defines product behavior for honoring zsh/bash/fish shell bindkeys in Warp's input editor. The product.md diff is detailed, but the attached annotated diff only contains product.md; tech.md is reported as patch-unavailable, so implementation-plan details in that file could not be validated or commented inline.
Concerns
- The zsh reserved-key list omits prompt-switch bindings that the current zsh bootstrap installs, which would let user shell bindings break Warp's prompt-mode communication.
- The v1 capability matrix conflicts with the product invariant that fish abbreviations expand natively.
- The shell-emitted DCS overlay path introduces a process-controlled input surface whose spoofing, bounds, validation, and failure behavior need to be explicit in the spec.
Security
- Shell-emitted DCS overlays are new PTY-delivered structured input; the spec should require per-tab authentication or equivalent spoofing defenses, strict payload bounds, schema validation, and discard-on-failure semantics before implementation.
Verdict
Found: 0 critical, 3 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
…curity PRODUCT warpdotdev#14 zsh reserved-key list adds `\ep` and `\ew` (the prompt-switch bindings the existing zsh bootstrap installs). Previously only `^P` and `\ei` were listed, leaving a gap where a user shell binding on the prompt-switch keys could break Warp ↔ shell prompt communication. TECH §6.1 Step 0 mirrors the expanded set. PRODUCT warpdotdev#11.6 capability matrix rewritten to reflect that fish v1 *does* deliver `abbr` expansion on space/enter — via the fish-specific space-trigger sync in TECH §6.1 step 5 plus the Enter sync. The previous matrix wording implied vanilla fish only honored Category C, which contradicted the invariant above that abbreviations expand natively. The matrix now lists three distinct rows: zsh/bash-with-blesh (full inline-render), fish v1 (Cat C + abbr), vanilla bash (Cat C only). PRODUCT warpdotdev#11.6 adds a "Security of the overlay channel" paragraph naming the per-tab nonce, size caps, schema validation, and discard-on-failure semantics for shell-emitted DCS overlays. User-facing note that spoofed payloads are dropped silently. TECH §6.4 rewritten with explicit validation phases for `WarpBufferState` mirroring §1's pattern for `ShellBindings`: pre-decode 64 KiB cap, nonce gate, schema decode, post-decode bounds (buffer 32 KiB, autosuggest 8 KiB, highlight count ≤ 256, cursor and highlight-range bounds-checked). Each phase discards the entire payload on failure. The "Why these caps" sub-paragraph grounds the numbers against observed shell-tool data.
§6.4 header said "three strictly ordered phases" but listed four — pre-decode byte cap, nonce gate, schema decode, post- decode bounds. One-character fix. PRODUCT warpdotdev#11.6 fish-v1 matrix row now mentions vi-mode indicators alongside Cat C and abbr expansion. The in-app state machine from TECH §"warpdotdev#13" delivers cursor-shape rendering regardless of injection mode, so a fish-vi-mode user shouldn't read the matrix and think their cursor shape won't work in v1. Syntax highlighting and autosuggestion overlays remain the actual fish-v1 gap.
§6.4 collapsed phase 2 (nonce gate) into phase 3 (schema decode) since the nonce lives inside the JSON payload per §1's `ShellBindings` shape — there's no way to gate on it before JSON deserialization without hand-rolling a partial parser the spec explicitly avoids. Three phases now: pre-decode cap → schema decode (including nonce check) → post-decode bounds. Keeps §1 and §6.4 saying the same thing about ordering. The "no partial-application path" header rule now explicitly carves out the `last_dispatched_widget` label as the one strip-on-unknown case, since the rule statement was contradicted by the bullet that stripped that label while keeping the rest of the payload. Structural failures still discard the entire payload; only the telemetry label gets the strip-and-continue treatment, which is reasonable because the label feeds no structural decision.
§6.1 step 4 showed `WarpBufferState { buffer, cursor, vi_mode?,
autosuggest?, highlights? }` but §6.4 step 2 validated
`schema_version` and `nonce` fields the shape didn't include.
Shape now lists `schema_version` and `nonce` so the §6.4 checks
match what §6.1 says exists.
§6.4's nonce-location parenthetical now cites
`WarpBufferState`'s own shape instead of pointing at
`ShellBindings`'s shape from §1 (still mentioned for context as
the parallel design). A reader of §6.4 no longer wonders if the
two payload types are accidentally being treated as the same.
§1 step 2 now matches §6.4 step 2's "Schema decode (with nonce check)" wording and explicitly names the nonce comparison. The two DCS payloads (ShellBindings, WarpBufferState) describe their nonce-check location identically. §1's `schema_version` policy paragraph adds a closing sentence saying the same policy applies to `WarpBufferState`. Both payloads now share the same schema-evolution contract on paper.
The original wording read as if the binding-hash on `Precmd` were signed and checked — but the spec's nonce design gates the whole payload, not the hash field. New wording describes the actual mechanism: a `Precmd` payload that fails the nonce check has its contents ignored, including the re-query hash.
§1 used `ShellBindings` for the wire payload (referencing `KeymapTable`, which was never defined elsewhere); §2 uses `ShellBindings` for the in-app storage struct with different fields and a `HashMap<KeymapMode, Vec<ShellBinding>>` for `keymaps`. §1 now explicitly notes the wire-vs-in-app split, defines `KeymapTable` inline as the wire-format helper, and points at §2's struct as the in-app destination.
|
/oz-review |
|
This is your last |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR adds product and tech specs for honoring shell bindkeys in Warp's input editor. I reviewed the attached description, spec context, and annotated diff; inline review is limited to specs/GH537/product.md because the attached diff marks specs/GH537/tech.md as patch unavailable.
Concerns
- The product spec says v1 ships with no AI-prompt opt-in, then defines v1 AI-prompt classifier rules and a lock shortcut, making the implementation scope unclear.
- The overlay-channel spoofing mitigation depends on nonce secrecy but the spec does not state where the nonce is stored or why spawned processes cannot read/reuse it.
- Pass-through failure diagnostics should explicitly inherit the widget-name redaction policy used elsewhere.
Security
- DCS overlay spoofing mitigation depends on nonce secrecy; the spec should document how the nonce is kept out of environment variables, logs, history, and child-process-readable state.
- Widget failure diagnostics can expose user-defined names unless they use the allowlist-or-bucket redaction policy.
Verdict
Found: 0 critical, 2 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
…daction PRODUCT warpdotdev#22.5 now explicitly scopes itself as follow-up (ships with warpdotdev#22's opt-in, not v1). Previously the section described v1-must-avoid failure modes and v1 classifier rules even though warpdotdev#22 resolves "v1 ships with no AI-prompt opt-in" — the contradiction made implementation scope unclear. The opening paragraph names warpdotdev#22.5 as follow-up scope and points at the Open Questions resolution; the rules subsection is reframed as "engaged when warpdotdev#22 opts in; not in v1 scope." PRODUCT warpdotdev#11.6 "Security of the overlay channel" now documents the nonce-secrecy property the spoofing defense rests on: the nonce is delivered out-of-band (env-then-unset for zsh/bash, `0600`-mode tempfile for fish), lives only in the shell's own memory after bootstrap, is not in env/history/output/tempfile, and is rotated by closing and reopening the tab. Same-uid read-the-shell's-memory attackers are out of scope, consistent with TECH §1's documented threat model. A real rc-file-misconfig leak is recoverable via tab restart. PRODUCT warpdotdev#11.5 pass-through failure diagnostic now explicitly uses the same allowlist-or-bucket widget-name redaction policy as warpdotdev#11/telemetry: verbatim names only for well-known ZLE/readline/fish vocabulary, user-defined or plugin-private names redacted to `user-defined`. Closes the leak path Oz flagged.
3 participants
Description
Spec PR for #537 — honor user-defined shell bindkeys (zsh bindkey, bash readline bind / ~/.inputrc, fish bind) in Warp’s input editor, instead of silently ignoring them.
Adds two short documents under specs/GH537/:
• product.md — 22 numbered behavior invariants covering discovery, dynamic rebind, the widget set we honor, vi/emacs mode handling, multi-tab independence, conflict precedence (user-Warp Warp customizations > shell bindkeys > Warp defaults), surface boundaries (shell input editor only — not the AI prompt or palettes), settings/feature-flag, and performance.
• tech.md — implementation plan grounded in current code: bootstrap-side query into a new DProtoHook::ShellBindings DCS variant (gated by a per-tab WARP_BOOTSTRAP_NONCE plus size caps and strict schema validation so arbitrary process output can’t spoof bindings), per-tab ShellBindings storage on Shell, expanded InputAction with fine-grained ZLE/readline verbs, a third shell_bindings tier in Keymap, in-app vi-mode state machine driven by dispatched widgets (with shell-reported mode as initial state and resync), settings toggle + FeatureFlag::HonorShellBindkeys for staged rollout, and telemetry with widget-name redaction (allowlist of well-known shell vocabulary; user-defined names bucket to user-defined).
Open questions deliberately surfaced for maintainer input rather than guessed: forwarding user-defined widget bodies back to the shell (deferred), AI prompt opt-in, and default-on vs feature-flagged rollout.
Linked Issue
Closes #537
ready-to-specorready-to-implement.Screenshots / Videos
Testing
Agent Mode