fix(windows): preserve native key input through conpty

[Pimpmuckl]

Summary

On Windows, Herdr receives the full native record for each physical key, but currently reduces it to a cross-platform key and modifiers before routing it. When Herdr later reconstructs input for ConPTY, details such as the scan code, control state, and repeat count are already gone. This is why physical keys such as Escape can become unreliable in native Windows applications.

This keeps the original Windows record attached to Herdr's semantic key:

Windows KEY_EVENT_RECORD
          |
          v
Herdr semantic key + original record
          |
          +-- Herdr keybinding consumes it --> stop
          |
          `-- unhandled, legacy Windows pane
                    |
                    v
             ConPTY Win32 record form --> Codex / native app

Herdr's semantic routing remains authoritative. At the pane boundary, unconsumed physical input for a legacy Windows ConPTY pane is encoded from the original record instead of reconstructed from the reduced key.

What this fixes

• physical Windows keys losing their scan code, control state, or repeat information inside Herdr
• physical Escape failures such as [WINDOWS} esc key doesnt work properly, while using codex #2077
• the same physical-input class for keys such as Shift+Enter and Ctrl+Space
• our sanity in doing per-key/per-combination workarounds one by one

What this does not fix

• the raw pane send-text Escape reproduction from Escape can still consume the following key in native Windows panes #1929. It contains bytes, not a physical key record
• Ctrl+Break, which follows a separate signal path
• semantic or negotiated input paths: pane send-keys, Kitty keyboard, modifyOtherKeys, API input, paste, IME/text input, remote input, and POSIX panes keep their existing behavior

Background: Discussion #1934.

Note on earlier fixes

2a20e90 should stay because it fixes how physical Escape reaches Herdr in the first place.

Whether the Shift+Enter workaround from #1909 should stay depends on whether pane send-keys is expected to preserve modified keys on Windows.

• If it is, keep the workaround and we should start working on a follow up by expanding the same approach for other generated keys that legacy VT cannot represent faithfully.
• If it is not, the Shift+Enter-specific workaround can be removed.

Either way, the whole PR #1909 cannot be reverted as-is because the code in this PR still relies on the part that respects the keyboard format requested by the application.

Validation

• just check (129 Windows-focused tests and 21 client transport tests)
• native Windows dogfood covering Escape, Shift+Enter, modified keys, key repeats, pane switching, AltGr, dead keys and normal Codex use for multiple heavy-usage days

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Windows key metadata is added to terminal input models and wire events, preserved during Win32 translation, used for repeat routing and retained-selection handling, and encoded through a generalized ConPTY fallback.

Changes

Windows key provenance

Layer / File(s)	Summary
Input contracts and wire conversion src/input/model.rs, src/input/mod.rs, src/input/parse.rs, src/protocol/wire.rs	Adds WindowsKeyRecord, attaches it to TerminalKey, and preserves it through ClientInputEvent::WindowsKey conversion and serialization.
Windows input provenance translation src/client/input/windows_vti.rs	Emits provenance-bearing Windows key events for eligible native records, keeps excluded paths semantic-only, and updates translation coverage.
Repeat identity and retained-selection routing src/app/mod.rs, src/app/input/clipboard.rs	Uses native Windows key identity for repeat detection and updates Ctrl-C suppression, pane routing, release, and focus-loss tests.
ConPTY fallback encoding src/platform/windows.rs, src/pane/terminal.rs	Replaces the Shift+Enter-only encoder with a generalized Windows-record-aware ConPTY fallback.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Win32Input
  participant WindowsVti
  participant ClientInputEvent
  participant App
  participant FocusedPane
  Win32Input->>WindowsVti: provide key record
  WindowsVti->>ClientInputEvent: emit WindowsKey with provenance
  ClientInputEvent->>App: deliver key metadata
  App->>App: derive native identity and classify repeats
  App->>FocusedPane: forward routed key event

Loading

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes preserving native Windows key input for ConPTY, matching the primary change.
Description check	✅ Passed	The description directly explains the native Windows key record preservation, ConPTY encoding changes, scope, and validation.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

The PR preserves native Windows key records through semantic routing and forwards unconsumed physical input to legacy ConPTY panes.

• Adds a versioned WindowsKey client input event carrying the original Win32 key record.
• Tracks native key identity and ownership across presses, repeats, releases, suppression, and pane changes.
• Encodes preserved records at the ConPTY boundary while retaining the semantic Shift+Enter fallback.
• Bumps the wire protocol and associated schema and test expectations from 18 to 19.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the prior compatibility issue is resolved by bumping the wire protocol and all corresponding advertised protocol values to 19.

Important Files Changed

Filename	Overview
src/client/input/windows_vti.rs	Preserves eligible physical Win32 key records while keeping IME, Ctrl+Break, Alt-code, and semantic-only paths separate.
src/input/model.rs	Extends semantic terminal keys with optional native Windows provenance and synchronizes record direction with event kind.
src/app/mod.rs	Uses scan-code-based identities to route native repeats and releases to the pane that received the original press.
src/platform/windows.rs	Encodes preserved native key records for ConPTY and retains the semantic Shift+Enter fallback.
src/protocol/wire.rs	Adds the serialized Windows key event and bumps the incompatible wire protocol change to version 19.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Windows KEY_EVENT_RECORD] --> B[Semantic key plus native record]
    B --> C{Consumed by Herdr routing?}
    C -->|Yes| D[Stop]
    C -->|No| E{Legacy Windows ConPTY pane?}
    E -->|Yes| F[Encode original Win32 record]
    E -->|No| G[Use existing semantic input path]

Loading

_{Reviews (10): Last reviewed commit: "fix(windows): preserve suppressed native..." | Re-trigger Greptile}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/protocol/wire.rs (1)

1133-1154: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Fix the Windows key test fixture or assertion.

record.key_down is initialized to false, but the test expects Some(true). to_raw_input_event() preserves the record unchanged, so this test fails deterministically. Set key_down: true for a press event, or assert Some(false) if release metadata is intentional.

Proposed fix

         let record = crate::input::WindowsKeyRecord {
-            key_down: false,
+            key_down: true,

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6a72c09d-fd13-4c85-8fd0-4c4b3761614c

📥 Commits

Reviewing files that changed from the base of the PR and between 965aae9 and 3597a09.

📒 Files selected for processing (4)

• docs/next/api/herdr-api.schema.json
• src/protocol/wire.rs
• tests/api_ping.rs
• tests/cli/sessions.rs

[ogulcancelik]

requesting changes because key repeats can lose the final release. for example, holding escape could send “escape down” but never “escape up,” leaving it stuck in the app. please fix and cover this case.

[Pimpmuckl]

requesting changes because key repeats can lose the final release. for example, holding escape could send “escape down” but never “escape up,” leaving it stuck in the app. please fix and cover this case.

Fixed in 82c5881e (edit: 664ef47d, after rebase)

Grouped Windows repeats are now split into one native record per press/repeat event. This means holding Escape for example keeps the native key down and repeat events, while also making sure that the final native key up reaches the application. This also works if focus is lost while the key is still held.

Added coverage for holding Escape through repeats, releasing it normally and losing focus while it is held.

One note on the protocol version: I intentionally kept this at 19 even though v0.7.5 uses 17 and master already moved to 18. The new WindowsKey event cannot be decoded by an older protocol-18 server. Keeping 19 means mixed preview builds fail during the handshake instead of connecting successfully and then breaking on the first native key event.

Depending how you fold that in a release, that can be pushed back down to 18 to keep the guidance for one-bump-per-release true.

[coderabbitai]

🧹 Nitpick comments (1)

src/app/mod.rs (1)

215-231: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider a brief comment explaining the scan-code bit-packing scheme.

0xf0000 + scan_code + (enhanced_bit << 16) deliberately lands in the valid Unicode scalar range up to 0x10FFFF (Plane 15 private-use area) so it never collides with a real char::from_u32 result while still fitting KeyCode::Char. This is a neat but non-obvious trick; a short comment would save future readers from re-deriving why 0xf0000 and the range bound were chosen.

💬 Example comment

     const ENHANCED_KEY: u32 = 0x0100;
+    // Pack scan_code + enhanced-key bit into a scalar in 0xF0000..=0x10FFFF
+    // (Unicode Plane 15/16 private-use range) so it can round-trip through
+    // `KeyCode::Char` without ever colliding with a real typed character.
     let native_code = key.windows_record.and_then(|record| {

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: db98d550-b8ae-4bfa-a500-81164352cef7

📥 Commits

Reviewing files that changed from the base of the PR and between 82c5881 and 664ef47.

📒 Files selected for processing (13)

• docs/next/api/herdr-api.schema.json
• src/app/input/clipboard.rs
• src/app/mod.rs
• src/client/input/windows_vti.rs
• src/input/mod.rs
• src/input/model.rs
• src/input/parse.rs
• src/pane/terminal.rs
• src/platform/windows.rs
• src/protocol/wire.rs
• tests/api_ping.rs
• tests/cli/sessions.rs
• tests/support/mod.rs

[ogulcancelik]

the escape repeat/release fix looks good, but requesting changes because standalone printable keys in default win32 input mode still lose native provenance. the key-down becomes a text event, while the physical release has no tracked press and is dropped. this leaves the pr’s broader physical-input guarantee incomplete and makes behavior depend on how windows groups repeats. please preserve printable physical records outside actual text/ime, paste, and vt framing, and cover a printable press/repeat/release cycle.

[Pimpmuckl]

the escape repeat/release fix looks good, but requesting changes because standalone printable keys in default win32 input mode still lose native provenance. the key-down becomes a text event, while the physical release has no tracked press and is dropped. this leaves the pr’s broader physical-input guarantee incomplete and makes behavior depend on how windows groups repeats. please preserve printable physical records outside actual text/ime, paste, and vt framing, and cover a printable press/repeat/release cycle.

Fixed in 851cde10 and d2010187.

This sits where Herdr decodes Win32 input. Physical key events are no longer reduced to plain text before routing. Herdr can still handle them as normal keybindings, but when it forwards an unhandled key to ConPTY, the original Windows record is still available.

Text and IME input, paste and VT framing keep their existing paths.

The tests now cover:

• normal printable press and release
• grouped key repeats
• separate count-one repeat records
• switching panes while a key is held
• the final release reaching the pane that received the first press

The separate count-one case is why d2010187 treats another native Press for an already-held key as a Repeat. Otherwise switching panes could move the key to the new pane and send its release to the wrong place.

I also removed the protocol 19 commits. I was trying to tackle a lot of edge cases with local reviewers and those kept tripping up over the explicit release repo rule. There's no real right or wrong here imo, Greptile's note on mismatched client vs server is correct, but release rule makes sense, too. So can bring it back or not, should mostly be fine either way.

Note that I haven't tested this explicitly for more than a few minutes locally.

@coderabbitai full review

[coderabbitai]

@Pimpmuckl: Re-running a full review of the updated PR, with particular attention to printable-key provenance, repeat classification, held-key pane ownership, and preservation of text/IME/paste/VT paths.

✅ Action performed

Full review finished.

[coderabbitai]

🧹 Nitpick comments (1)

src/app/mod.rs (1)

1857-1937: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Consider adding a state-invariant assertion to this identity/state test.

This test exercises pressed_key_identity (an identity refactor) together with adversarial workspace state (mid-hold focus change, then focus loss). Per path instructions, identity/state refactors should validate with AppState::assert_invariants_for_test() (or Workspace::assert_invariants_for_test()) in adversarial test state, in addition to the existing byte-output assertions.

As per path instructions: "For identity or state refactors, use AppState::assert_invariants_for_test() or Workspace::assert_invariants_for_test() with adversarial test state where applicable."

Source: Path instructions

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f258bcac-865c-4781-b764-3bfc11bca743

📥 Commits

Reviewing files that changed from the base of the PR and between b411274 and d201018.

📒 Files selected for processing (9)

• src/app/input/clipboard.rs
• src/app/mod.rs
• src/client/input/windows_vti.rs
• src/input/mod.rs
• src/input/model.rs
• src/input/parse.rs
• src/pane/terminal.rs
• src/platform/windows.rs
• src/protocol/wire.rs

[Pimpmuckl]

🧹 Nitpick comments (1)

src/app/mod.rs (1)> 1857-1937: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Consider adding a state-invariant assertion to this identity/state test.
This test exercises pressed_key_identity (an identity refactor) together with adversarial workspace state (mid-hold focus change, then focus loss). Per path instructions, identity/state refactors should validate with AppState::assert_invariants_for_test() (or Workspace::assert_invariants_for_test()) in adversarial test state, in addition to the existing byte-output assertions.
As per path instructions: "For identity or state refactors, use AppState::assert_invariants_for_test() or Workspace::assert_invariants_for_test() with adversarial test state where applicable."

Source: Path instructions

ℹ️ Review info

Generic AppState invariant assertion suggested here does not cover pressed_terminal_keys, since that state lives directly on App.

A direct check that the release and focus-loss paths clear all held-key ownership does make sense, so I that is added in ccc75b4f.

[ogulcancelik]

i pushed fb8827e2 to your branch. it keeps count-one native presses suppressed when the physical key has no pane owner, adds regression coverage for retained-selection ctrl-c, and restores protocol 19 because protocol 18 shipped in preview. just check passes locally. could you verify the windows behavior on your setup—especially held escape, printable repeats/releases, retained-selection ctrl-c, and pane switching—and confirm it looks right before merge?

[Pimpmuckl]

i pushed fb8827e2 to your branch. it keeps count-one native presses suppressed when the physical key has no pane owner, adds regression coverage for retained-selection ctrl-c, and restores protocol 19 because protocol 18 shipped in preview. just check passes locally. could you verify the windows behavior on your setup—especially held escape, printable repeats/releases, retained-selection ctrl-c, and pane switching—and confirm it looks right before merge?

Tested fb8827e on native Windows and everything looks right.

• held Escape repeats correctly and stops cleanly on release
• printable repeats and releases work without stuck or dropped input
• input remains owned by the original pane when switching panes mid-hold
• with copy_on_select disabled, held Ctrl+C copied and cleared a retained selection without interrupting the running
  process, toast included
• the next standalone Ctrl+C reached the pane normally and stopped the process
• Shift+Enter, modified keys, AltGr, dead keys, pane switching, and normal Codex use also behaved correctly

Looks good from my side for now, I'll keep running it locally for a bit.

edit:
Will try as I get more ideas:

• clipboard paste matched direct Codex for single-line, internal LF, internal CRLF, and trailing CRLF input
• printable repeats stop correctly when losing focus on the herdr running terminal
• overlapping physical key holds and reversed release works as intended
• navigation and keys like home/end/insert/del work, page up/down is correctly scrolling up/down and is intercepted by herdr

Only thing I'm missing is numpad buttons

[ogulcancelik]

Superseded by #2142. We preserved the core Windows implementation and credited you as co-author, then refactored it around a shared input model and lease state machine so the final change is easier to review as one maintainer-owned commit. Thank you for the implementation, repeated Windows testing, and patience through the review.

If you have time, please verify the exact #2142 head on Windows as well.