Skip to content

Fix/windows terminal shell - #80

Open
xiaomo-xty wants to merge 4 commits into
kyle-ssg:mainfrom
xiaomo-xty:fix/windows-terminal-shell
Open

Fix/windows terminal shell#80
xiaomo-xty wants to merge 4 commits into
kyle-ssg:mainfrom
xiaomo-xty:fix/windows-terminal-shell

Conversation

@xiaomo-xty

Copy link
Copy Markdown

What & why

Kyde currently reaches several Unix-only assumptions when built and used on Windows:

  • The embedded terminal reads $SHELL and falls back to /bin/zsh, which cannot spawn on Windows.
  • The app-level ConfirmKey binding consumes Enter before the focused terminal can submit a command.
  • The single-instance implementation uses Unix-domain sockets.
  • Update verification and tests assume shasum, Unix executable bits, LF line endings, or / path separators.

This PR makes those paths portable:

  • Use %COMSPEC% for Windows terminal sessions, with cmd.exe as a fallback.
  • Add a terminal-scoped Enter action that writes \r to the PTY, plus a GPUI dispatch regression test.
  • Preserve executable modes from Git's index on Windows and make line-ending/path assertions platform-independent.
  • Compute update SHA-256 checksums in-process. sha2 is already in Kyde's normal dependency graph.
  • Compile the Unix-socket single-instance guard only on Unix; Windows keeps the previous multi-instance behavior until it has native IPC.

How to test

On Windows:

  1. Put the Windows SDK directory containing fxc.exe on PATH.
  2. Run cargo build.
  3. Run cargo test --all.
  4. Open a repository, toggle the terminal, enter echo hello, and press Enter.
  5. Partially stage an executable file and confirm its index mode is preserved.

Checks completed on Windows:

  • cargo fmt --all -- --check
  • cargo clippy --all-targets -- -D warnings
  • cargo test --all
  • cargo build

Speed & footprint

  • No new work added to a hot path (per-keystroke highlight, per-frame render, per-file-select diff) - or if there is, it's justified and has a perf_* guard test.
  • No new always-on dependency that bloats the binary / resident RAM. sha2 was already present in Kyde's normal dependency graph.
  • Startup still feels instant (the app should open before you finish letting go of the mouse).
  • N/A - docs / chore / pure refactor with no runtime impact.

Checklist

  • cargo build is clean (no new warnings).
  • cargo test is green (cargo test perf if you touched a hot path).
  • No vendor code or assets copied from Zed or any GPL source (patterns only - see README -> Reference material).
  • CHANGELOG.md is updated; README.md / CLAUDE.md do not need changes for this behavior.

little_suns added 4 commits August 1, 2026 10:34
Gate Unix-domain-socket single-instance support to Unix targets, leaving Windows on the existing multi-instance behavior. Compare the new explicit-directory tree test with PathBuf values so native separators do not fail the suite.
Give Enter a Terminal-scoped action so it shadows the app-level confirmation binding and writes carriage return to the PTY. Add a GPUI key-dispatch regression test.
@themis-gatacre-games

Copy link
Copy Markdown

AI Review: Ship it

A solid portability PR that makes Kyde's terminal, single-instance guard, update checksum, and tests work on Windows without regressing Unix. The changes are well-scoped: each platform fork is cleanly gated with #[cfg], the in-process SHA-256 computation is correct (verified the hardcoded test digest), and the new TerminalEnter action + gpui dispatch test is a good catch for a real key-swallowing bug.

Correctness 5/5
Test coverage 4/5
Code quality 5/5
Product impact 4/5
Walkthrough
  • Terminal shell selection - default_shell() now reads %COMSPEC% on Windows (falling back to cmd.exe) instead of the Unix-only $SHELL / /bin/zsh.
  • Enter key relay - New TerminalEnter action bound in the Terminal context shadows the app-root ConfirmKey, preventing Enter from being swallowed; includes a gpui dispatch regression test.
  • Single-instance guard - instance module and all its call sites are #[cfg(unix)]-gated; Windows keeps multi-instance behavior until native IPC is added.
  • In-process SHA-256 - verify_checksum now uses the sha2 crate (already in the dep graph) instead of shelling out to shasum, making updates work on Windows.
  • Git mode preservation - stage_content_mode on non-Unix reads the mode from git ls-files --stage instead of always returning 100644, preserving executable bits through partial staging.
  • Test portability - core.autocrlf=false in git test fixtures, PathBuf-based assertions in kyde-tree, #[cfg(unix)] on symlink-dependent shellcmd tests.
How to verify
  1. On Windows: open a repo, toggle the terminal (Ctrl+``), type echo hello`, press Enter - the command should execute (not be swallowed).
  2. On macOS/Linux: same test - confirm no regression in terminal Enter behavior.
  3. On Windows: partially stage an executable file (git update-index --chmod=+x first), confirm the staged entry keeps 100755 mode.
  4. Trigger a self-update on Windows and confirm the SHA-256 verification passes (no shasum dependency).
  5. On macOS/Linux: confirm the single-instance guard still works (kyde . in a second terminal should activate the existing window).

Automate: cargo test --all on both Windows and Linux/macOS CI runners.

Product take: This unblocks Windows as a real target. The Enter-key fix is the highest-value change - without it the embedded terminal is unusable. The rest is necessary plumbing to get cargo build and cargo test green on Windows. Solid infrastructure work.

Assumptions & unverified claims
  • The sha2 crate is stated to already be in Kyde's dependency graph (via a transitive dep); I verified it appears in Cargo.lock but did not confirm it adds zero binary size overhead.
  • The PR claims all four checks (fmt, clippy, test, build) pass on Windows; I reviewed the code but could not run a Windows build to confirm.
  • alacritty_terminal's Shell::new with cmd.exe on Windows is assumed to work correctly (the PTY options include a Windows-only escape_args: true field, suggesting alacritty has Windows support).

A PR that teaches an app to speak both /bin/zsh and cmd.exe - truly bilingual. reviewed at 7807cc2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant