feat: Add Linux support and bash auto-trigger#62
feat: Add Linux support and bash auto-trigger#62andykofman wants to merge 1 commit intoStanMarek:masterfrom
Conversation
- Add support for Linux terminals (GNOME Terminal, Konsole, Tilix, Foot, Terminator, xterm) - Add bash shell integration with auto-trigger support - Update install script to detect and support bash - Add init.bash for bash PTY proxy initialization - Update shell scripts with Linux terminal environment variable detection - Change default shell fallback from zsh to bash for Linux compatibility - Add comprehensive tests for Linux terminal detection and bash installation Key Changes: * gc-terminal: Added Linux terminal variants and detection logic * install.rs: Added bash installation functions and shell detection * init.bash: New file for bash PTY proxy initialization with Linux terminal support * ghost-complete.bash: Added auto-trigger on space, /, -, ., =, : with live buffer updates * init.zsh: Added Linux terminal environment variable checks for tmux compatibility Testing: * All 59 gc-terminal tests pass * All 29 ghost-complete installation tests pass * Verified installation and functionality on Ubuntu with Ghostty terminal
|
Thank you for this contribution. This is a big step forward. I really appreciate you taking on Linux support here, especially since I’m no longer using Linux myself after switching to macOS, so this is something I wouldn’t realistically be able to add on my own. Would you be able to also provide:
That would help me understand the full user experience and review the change with more confidence |
StanMarek
left a comment
StanMarek
left a comment
There was a problem hiding this comment.
PR #62 Review — Linux Support & Bash Auto-Trigger
Appreciate the ambition here — Linux support is a real feature gap. But this PR has several issues that need addressing before it's mergeable. Ran it through code review, test coverage analysis, and error handling audit.
Critical Issues (must fix)
1. Ghostty detection demoted in non-tmux path — regression for primary terminal
gc-terminal/src/lib.rs — In the non-tmux detect_from_env branch, Ghostty detection via has_ghostty_res now runs after Kitty/WezTerm/Alacritty. Previously Ghostty was detected first via from_term_program("ghostty"). This breaks the "Ghostty is primary" invariant. The tmux branch correctly keeps Ghostty first — the non-tmux branch should match.
Fix: Move if has_ghostty_res before if has_kitty_window_id.
2. VTE_VERSION fallback misidentifies terminals as GNOME Terminal
gc-terminal/src/lib.rs — has_vte_version && term_program.is_empty() falls back to GnomeTerminal, but VTE is used by many terminals (Xfce4-terminal, Guake, LXTerminal, Sakura, etc.). The tmux path is worse: has_gnome_terminal || has_vte_version labels ANY VTE terminal as GNOME Terminal regardless of term_program.
Fix: Use Terminal::Unknown("vte-based") for the VTE-only fallback, or create a VteBased variant. Don't misidentify terminals.
3. Default shell fallback changed from /bin/zsh to /bin/bash — breaking for macOS
main.rs — This project is macOS-primary, zsh-primary (per CLAUDE.md). Changing the $SHELL-unset fallback to bash is a silent behavior change for existing users.
Fix: Use cfg!(target_os = "macos") to keep /bin/zsh on macOS, /bin/bash on Linux.
4. detect_shell() silently defaults to bash — wrong rc file modified
install.rs — When $SHELL is unset (Docker, CI, minimal installs), the installer silently writes to ~/.bashrc while printing "Detected shell: bash" — the word "Detected" implies confidence when there was none. A zsh user gets their .bashrc modified (which they never source) and wonders why nothing works.
Fix: When detection fails, bail with a clear error asking the user to specify --shell bash or --shell zsh explicitly.
High Priority Issues (should fix)
5. detect_from_env now takes 14 boolean parameters
gc-terminal/src/lib.rs — This is a positional boolean minefield. One transposition silently misdetects terminals. The test helpers already show the pain: 7 trailing false values with comments.
Fix: Introduce a DetectionEnv struct with named fields.
6. Unsupported shell silently installs bash config
install.rs — Fish/tcsh/nu users get their ~/.bashrc modified with no warning that this is useless for their actual shell. Either refuse with a clear error or don't touch files the user never sources.
7. Permission-denied handler says "Installation complete" when it isn't
install.rs — Both bash and zsh paths print a green checkmark and "Installation complete (manual shell configuration required)" on write failure. This is misleading. The exit code is 0. Scripts can't detect the failure.
Fix: Return an error with non-zero exit, and say "Installation INCOMPLETE" clearly.
8. run_uninstall() bare ? on read_to_string — no context
install.rs — Every other read_to_string in this file uses .with_context(). These two use bare ?, producing raw OS errors with no file path.
9. No bash version check despite requiring 4.4+
ghost-complete.bash — Comment says "Requires Bash 4.4+" for bind -x, but there's no version guard. macOS ships Bash 3.2. A macOS user who selects bash installation gets silent failures.
Fix: Add BASH_VERSINFO check at the top of ghost-complete.bash.
10. exec ghost-complete is unrecoverable
init.bash — If ghost-complete crashes on startup, the user's terminal session dies. On Linux (this PR's target), users may not have an alternative terminal handy. At minimum, the exec || fallback pattern should be used.
Test Coverage Gaps (should fix)
11. VTE fallback heuristic completely untested — No test for has_vte_version && term_program.is_empty() → GnomeTerminal.
12. VTE + specific terminal priority untested — Tilix sets both TILIX_ID and VTE_VERSION. No test verifies Tilix wins over the VTE fallback.
13. tmux + Linux terminal detection entirely untested — The new tmux Linux block (Konsole, Tilix, Foot, Terminator, GNOME in tmux) has zero test coverage. The tmux VTE fallback differs from non-tmux (asymmetry is untested).
14. detect_shell() untested — Drives the entire install dispatch, no unit test.
15. Bash uninstall path untested — No roundtrip install→uninstall test for bash. No cross-shell isolation test (uninstall bash doesn't remove zsh scripts).
Medium Issues
16. uninstall_from_zsh is dead code duplication — 40 lines duplicating uninstall_from with divergent behavior (removes ALL scripts vs shell-specific). Delete it, update the one test.
17. OSC sequences print as garbage without PTY proxy — ghost-complete.bash unconditionally sends OSC 7770 on every keypress. Add [[ -z "$GHOST_COMPLETE_ACTIVE" ]] && return 0 guard.
18. WEZTERM_UNIX_SOCKET missing from non-tmux env-var detection — Both init.zsh and init.bash check env vars for Kitty, Alacritty, Ghostty, and all Linux terminals in the non-tmux path, but skip WezTerm. Inconsistent with the tmux branch.
19. let _ = fs::remove_dir() swallows all errors — Not just "directory not empty". Permission errors, I/O errors silently ignored.
What's Good
- Terminal detection tests for the happy paths are solid
- Bash install lifecycle tests (create, preserve, idempotency) follow existing patterns well
- Shell init script structure mirrors the proven zsh approach
- Auto-trigger character set (space, /, -, ., =, :) is well-chosen for completion triggers
Verdict
Requesting changes. The critical items (Ghostty priority regression, VTE misidentification, default shell change, silent bash fallback) need to be fixed. The 14-bool parameter signature and missing test coverage for tmux+Linux combos are high-risk for future regressions. Good foundation but needs another pass.
2 participants
Key Changes:
Testing:
What
Why
Testing
cargo testpassescargo clippy --all-targets -- -D warningscleancargo fmt --checkclean