From 0c9f4baa3ac633eafddc6b0d7fb9cd322d5e01bd Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Fri, 31 Jul 2026 16:30:33 +0800 Subject: [PATCH] fix(cli): make the PATH install reversible, honest, and safe to migrate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up on the review of #277. Seven fixes, no change to what the feature is for. An AppImage copy is now claimed with a marker file instead of being inferred from "am I an AppImage right now". Keying off the runtime meant that a user who moved from the AppImage to the tarball hit their own copy, read it as somebody else's binary, and never got another install for as long as that file sat there. The Windows uninstaller takes {app} back out of HKCU\Environment. Nothing did before: the entry is written by the app at runtime, so Inno never knew it existed and every uninstall grew the user's PATH by one dead entry. Unix has no equivalent hook and still leaves its symlink behind; that is now stated in the module docs rather than left to be discovered. An occupied candidate directory no longer ends the scan, and every platform now reports whether the install actually wins the lookup. `Occupied` on /opt/homebrew/bin used to mean giving up while ~/.local/bin sat free, and Windows — which appends to PATH and so never collides — reported `Installed` even when an existing tty7 earlier on PATH kept beating it. A new `InstalledShadowed` names the winner. `cargo run --release` no longer repoints the developer's real tty7 at a build tree. `cfg!(debug_assertions)` only covered the debug half of that. The Windows registry PATH is read, matched, and written as UTF-16 throughout. It went through `to_string_lossy` before, so a value the registry holds but Rust cannot represent as a String would have been written back with U+FFFD in place of its characters — the exact PATH corruption the surrounding code is careful to avoid. Two tests mutated $HOME and $PATH while the rest of the binary's tests ran beside them, and src/ui/home.rs mutates $HOME too. `candidate_dirs` takes home as a parameter, `place` takes its mode, and the PATH-joining and registry- joining rules are pure functions — so no test in this module touches the environment any more. 5 tests become 11, and the Windows joining logic is covered on every platform. Also: the config flag reaches Settings → About and both features docs instead of being config.json-only, startup reads config.json once instead of twice, and the CLI's strip failure warns like its sibling instead of being swallowed. --- .github/scripts/bundle-linux.sh | 2 +- .github/scripts/windows-installer.iss | 65 +++ docs/features.md | 1 + docs/features.zh-CN.md | 1 + src/core/cli_install.rs | 582 ++++++++++++++++++++------ src/main.rs | 6 +- src/ui/app.rs | 7 + src/ui/settings.rs | 40 ++ 8 files changed, 563 insertions(+), 141 deletions(-) diff --git a/.github/scripts/bundle-linux.sh b/.github/scripts/bundle-linux.sh index 82e1eef8..be906ff1 100755 --- a/.github/scripts/bundle-linux.sh +++ b/.github/scripts/bundle-linux.sh @@ -36,7 +36,7 @@ chmod +x "$STAGE/tty7" # Release builds keep symbols (thin LTO, no profile strip); drop them here so # the archive isn't ~100 MB of debug info. strip "$STAGE/tty7-app" || echo "⚠️ strip unavailable — shipping unstripped binary" -strip "$STAGE/tty7" || true +strip "$STAGE/tty7" || echo "⚠️ strip unavailable — shipping unstripped CLI" mkdir -p "$STAGE/completions" cp assets/completions/*.json "$STAGE/completions/" cp LICENSE "$STAGE/LICENSE" diff --git a/.github/scripts/windows-installer.iss b/.github/scripts/windows-installer.iss index 387fd655..1cb9047c 100644 --- a/.github/scripts/windows-installer.iss +++ b/.github/scripts/windows-installer.iss @@ -74,6 +74,7 @@ Source: "{#StageDir}\tty7-app.exe"; DestDir: "{app}"; Flags: ignoreversion ; The CLI. `core::cli_install` adds {app} to the user's PATH at first launch, ; so this is not registered as an [Env] change here — the portable zip has no ; installer to do it, and one code path serving both is one behaviour to debug. +; The uninstaller takes that entry back out; see RemoveAppDirFromUserPath below. Source: "{#StageDir}\tty7.exe"; DestDir: "{app}"; Flags: ignoreversion Source: "{#StageDir}\completions\*"; DestDir: "{app}\completions"; Flags: ignoreversion recursesubdirs Source: "{#StageDir}\LICENSE.txt"; DestDir: "{app}"; Flags: ignoreversion @@ -116,3 +117,67 @@ begin SW_HIDE, ewWaitUntilTerminated, ResultCode); Result := ''; end; + +(* Take {app} back out of the user's PATH. + + `core::cli_install` puts it there at first launch rather than the installer + doing it, because the portable zip has no installer — but that leaves nobody + to undo it, and an uninstall that permanently grows the user's PATH by one + dead entry is not an uninstall. So the removal lives here, on the one install + shape that has an uninstaller at all. (Unix has no equivalent hook: deleting + the .app or the tarball leaves the symlink behind for the user to remove.) + + HKCU even for an all-users install: cli_install only ever writes the user + hive, so that is the only place an entry can be. On a machine where several + users ran tty7, this clears the one uninstalling — the others keep a dead + entry, which is the price of not needing elevation to install in the first + place. + + Entries are compared case-insensitively and ignoring a trailing backslash, + and every other entry is written back verbatim: this is somebody's PATH, and + we are here to remove one thing from it, not to tidy it. No + WM_SETTINGCHANGE broadcast — the entry now names a deleted directory, so + nothing is waiting on the news, and it is gone from new shells at next + sign-in regardless. *) +procedure RemoveAppDirFromUserPath(); +var + Existing, Rebuilt, Raw, Entry, Target: String; + P: Integer; +begin + if not RegQueryStringValue(HKEY_CURRENT_USER, 'Environment', 'Path', Existing) then + exit; + + Target := RemoveBackslashUnlessRoot(ExpandConstant('{app}')); + Rebuilt := ''; + (* The trailing ';' makes the last entry look like every other one. *) + Existing := Existing + ';'; + repeat + P := Pos(';', Existing); + Raw := Copy(Existing, 1, P - 1); + Existing := Copy(Existing, P + 1, Length(Existing)); + Entry := Trim(Raw); + if (Entry <> '') and + (CompareText(RemoveBackslashUnlessRoot(Entry), Target) <> 0) then + begin + if Rebuilt <> '' then + Rebuilt := Rebuilt + ';'; + Rebuilt := Rebuilt + Raw; + end; + until Existing = ''; + + if Rebuilt = '' then + RegDeleteValue(HKEY_CURRENT_USER, 'Environment', 'Path') + (* Inno cannot read a value's type back, so infer it: a PATH holding a '%' is + one that has to stay expandable, and rewriting it as REG_SZ would freeze + every other entry's variable at today's value. *) + else if Pos('%', Rebuilt) > 0 then + RegWriteExpandStringValue(HKEY_CURRENT_USER, 'Environment', 'Path', Rebuilt) + else + RegWriteStringValue(HKEY_CURRENT_USER, 'Environment', 'Path', Rebuilt); +end; + +procedure CurUninstallStepChanged(CurUninstallStep: TUninstallStep); +begin + if CurUninstallStep = usUninstall then + RemoveAppDirFromUserPath(); +end; diff --git a/docs/features.md b/docs/features.md index 278bf194..4c5c6f09 100644 --- a/docs/features.md +++ b/docs/features.md @@ -64,6 +64,7 @@ it never wraps or replaces the agent. - **Copy Session ID** — put the agent's native session id on the clipboard, beside *Copy Working Directory*, for pasting into `codex resume`, a bug report, or another tool - **Context feed** — palette commands send the current selection or the repo's `git diff` to the running agent as a ready-made prompt - **Tray icon** — a system tray / menu bar item that flips to an attention state the moment any agent needs your input; its menu lists every agent pane (brand avatar + status dot, click to reveal), switches the notification policy, and offers *Quit and Stop Daemon* alongside the plain session-keeping quit (`show_tray_icon`, on by default) +- **`tty7` on PATH** — the CLI ships inside every installer and is put on PATH at launch, so a script or a coding agent can drive tty7 from any terminal. Inside a tty7 pane it works regardless, since panes inherit the app's environment. On Unix it is a symlink into whichever of `/opt/homebrew/bin`, `/usr/local/bin`, `~/.local/bin`, `~/bin`, `~/.cargo/bin` your PATH already covers; on Windows the install directory is appended to your user PATH, and the uninstaller takes it back out. A `tty7` you installed yourself is left alone, never replaced. Off via Settings → About or `install_cli_on_path: false` in `config.json` ## SSH diff --git a/docs/features.zh-CN.md b/docs/features.zh-CN.md index 4669c04a..692cf9e5 100644 --- a/docs/features.zh-CN.md +++ b/docs/features.zh-CN.md @@ -61,6 +61,7 @@ Aider、Amp、OpenCode 等约 17 个）并在其外围加功能 —— 绝不包 - **复制 Session ID** —— 把 agent 的原生 session id 复制到剪贴板，就在 *Copy Working Directory* 旁边，方便粘进 `codex resume`、bug 报告或别的工具 - **上下文回填** —— 面板命令把当前选区或仓库 `git diff` 打包成 prompt 直接喂给正在跑的 agent - **托盘图标** —— 系统托盘 / 菜单栏常驻图标，任何 agent 等你输入时立即切换为提醒态；菜单列出所有 agent pane（品牌头像 + 状态点，点击直达）、可切换通知策略，并在保留会话的普通退出之外提供 *Quit and Stop Daemon*（`show_tray_icon`，默认开启） +- **`tty7` 上 PATH** —— CLI 随每个安装包一起发布，启动时自动放到 PATH 上，脚本和 coding agent 在任何终端里都能驱动 tty7。tty7 自己的 pane 里则一定可用，因为 pane 继承 app 的环境。Unix 上是往 `/opt/homebrew/bin`、`/usr/local/bin`、`~/.local/bin`、`~/bin`、`~/.cargo/bin` 中你 PATH 已经覆盖的那个目录里放一个软链；Windows 上是把安装目录追加到用户 PATH，卸载时再摘掉。你自己装的 `tty7` 一律保持原样，不会被覆盖。关掉：设置 → About，或 `config.json` 里 `install_cli_on_path: false` ## SSH diff --git a/src/core/cli_install.rs b/src/core/cli_install.rs index 3d98c23b..b8b05fe6 100644 --- a/src/core/cli_install.rs +++ b/src/core/cli_install.rs @@ -21,7 +21,15 @@ //! //! Nothing here is fatal. Every failure path logs and returns; a user whose //! system resists all of it still has a working GUI, just no `tty7` on PATH. +//! +//! Undoing it is asymmetric too. The Windows uninstaller strips the PATH entry +//! back out (see the `[Code]` section of `windows-installer.iss`). Unix has no +//! uninstall hook to hang that off — dragging the `.app` to the Trash or +//! deleting the tarball leaves the symlink behind, dangling. An upgrade heals +//! it (a dangling link still names `tty7`, so the next launch repoints it); a +//! real uninstall leaves one broken entry the user removes by hand. +use std::ffi::{OsStr, OsString}; use std::path::{Path, PathBuf}; /// What a run of [`install`] did, for the log line and for tests. @@ -31,7 +39,8 @@ pub enum Outcome { Disabled, /// No CLI beside the GUI — a hand-assembled tree, or a stripped bundle. NoBundledCli, - /// A debug build: panes were wired up, the system was left untouched. + /// A debug build, or a binary sitting in a cargo build tree: panes were + /// wired up, the system was left untouched. DevBuild, /// Already reachable as `tty7`, pointing at this install. AlreadyInstalled(PathBuf), @@ -39,7 +48,11 @@ pub enum Outcome { Installed(PathBuf), /// Installed somewhere the user's PATH does not currently cover. InstalledOffPath(PathBuf), - /// Something is already called `tty7` on PATH and it is not ours to move. + /// Installed, but an earlier PATH entry holds a different `tty7` that keeps + /// winning the lookup. + InstalledShadowed { ours: PathBuf, winner: PathBuf }, + /// Every directory we would write to is already taken by someone else's + /// `tty7`, and none of them is ours to move. Occupied(PathBuf), /// Nowhere to write. Failed(String), @@ -57,8 +70,11 @@ const CLI_NAME: &str = "tty7"; /// added here reaches every shell opened in this session — including the very /// first one, and including the case where the on-disk half below fails /// outright. -pub fn install() -> Outcome { - let outcome = install_inner(); +/// +/// Takes the config flag rather than loading it, so startup reads `config.json` +/// once for this and the daemon decision that follows it. +pub fn install(enabled: bool) -> Outcome { + let outcome = install_inner(enabled); match &outcome { Outcome::Disabled | Outcome::NoBundledCli | Outcome::DevBuild => { log::debug!("cli install skipped: {outcome:?}") @@ -70,6 +86,12 @@ pub fn install() -> Outcome { outside a tty7 pane", p.display() ), + Outcome::InstalledShadowed { ours, winner } => log::warn!( + "installed the tty7 CLI at {}, but `tty7` outside a tty7 pane still resolves to {} — \ + remove that one, or reorder your PATH, to reach the bundled CLI", + ours.display(), + winner.display() + ), Outcome::Occupied(p) => log::info!( "leaving the existing `tty7` at {} alone; the bundled CLI was not installed", p.display() @@ -79,28 +101,71 @@ pub fn install() -> Outcome { outcome } -fn install_inner() -> Outcome { - if !crate::core::config::Config::load().install_cli_on_path { +fn install_inner(enabled: bool) -> Outcome { + if !enabled { return Outcome::Disabled; } let Some(cli) = bundled_cli() else { return Outcome::NoBundledCli; }; + // Snapshot PATH *before* the prepend below, so the shadow check at the end + // asks "what would the user's shell have found", not "what did we just put + // in front of everything". + let user_path = path_dirs(); + // Panes reach the CLI through the daemon's inherited environment even when // the on-disk half below is refused, so do this first and unconditionally. if let Some(dir) = cli.parent() { prepend_to_process_path(dir); } - // A dev build gets the environment half and nothing else. `target/debug/` + // A dev build gets the environment half and nothing else. A build tree // holds a `tty7` too, so without this a `cargo run` would point the user's - // real `tty7` at a debug binary — and the isolated instances the dev-verify - // flow spins up would each rewrite the PATH of the machine they are meant - // to be kept away from. Panes still get the build under test, which is the - // half that development actually needs. - if cfg!(debug_assertions) { + // real `tty7` at a binary the next `cargo clean` deletes — and the isolated + // instances the dev-verify flow spins up would each rewrite the PATH of the + // machine they are meant to be kept away from. Panes still get the build + // under test, which is the half that development actually needs. + if cfg!(debug_assertions) || in_a_build_tree(&cli) { return Outcome::DevBuild; } - platform_install(&cli) + + let outcome = platform_install(&cli, &user_path); + + // Writing the file is not the same as winning the lookup. `user_path` is a + // snapshot of the *directories*, not of their contents, so scanning it now + // sees whatever we just wrote sitting in its real PATH position: find + // ourselves and there is no shadow, find someone else and there is. + // + // This is the only report Windows gets. It appends to the user's PATH + // rather than placing a file, so it never collides with another `tty7` and + // never has a reason to say `Occupied` — but an existing one earlier on + // PATH still beats it, and "installed" alone would be a lie. + let ours = match &outcome { + Outcome::AlreadyInstalled(p) | Outcome::Installed(p) | Outcome::InstalledOffPath(p) => { + p.clone() + } + _ => return outcome, + }; + match first_cli_on(&user_path) { + Some(winner) if winner != ours => Outcome::InstalledShadowed { ours, winner }, + _ => outcome, + } +} + +/// The first `tty7` the user's shell would find, if any. +/// +/// `is_file` follows symlinks on purpose: a dangling link left by an install +/// that has since been deleted is not something that wins a lookup, so it must +/// not count as a shadow. +fn first_cli_on(dirs: &[PathBuf]) -> Option { + dirs.iter() + .map(|d| d.join(CLI_NAME)) + .find(|candidate| candidate.is_file()) +} + +fn path_dirs() -> Vec { + std::env::var_os("PATH") + .map(|p| std::env::split_paths(&p).collect()) + .unwrap_or_default() } /// The CLI shipped alongside this GUI, if there is one. @@ -117,52 +182,92 @@ fn bundled_cli() -> Option { cli.is_file().then_some(cli) } +/// Whether this executable is sitting in a cargo build directory. +/// +/// `cfg!(debug_assertions)` alone catches `cargo run` but not +/// `cargo run --release`, which would otherwise aim the developer's real `tty7` +/// at a build tree. Matches both `target/release/` and the +/// `target//release/` shape that `--target` produces. +fn in_a_build_tree(cli: &Path) -> bool { + let Some(profile_dir) = cli.parent() else { + return false; + }; + let named_after_a_profile = profile_dir + .file_name() + .is_some_and(|n| n == "debug" || n == "release"); + named_after_a_profile + && profile_dir + .ancestors() + .any(|a| a.file_name() == Some("target".as_ref())) +} + /// Make the CLI reachable from this process's children (the daemon, and so /// every pane) without waiting for the on-disk install to take effect. +fn prepend_to_process_path(dir: &Path) { + let current = std::env::var_os("PATH").unwrap_or_default(); + match path_with_dir_first(¤t, dir) { + // SAFETY: single-threaded startup — this runs from `main` before the + // daemon is spawned and before gpui's executor exists, so there is no + // concurrent reader of the environment. + Some(Ok(path)) => unsafe { std::env::set_var("PATH", path) }, + Some(Err(e)) => log::warn!("could not extend PATH with {}: {e}", dir.display()), + None => {} + } +} + +/// The PATH `dir` belongs at the front of, or `None` when it is already listed. /// /// Prepended rather than appended so it wins over a stale copy left on PATH by /// an older install — inside a tty7 pane, `tty7` should mean the tty7 you are /// sitting in. -fn prepend_to_process_path(dir: &Path) { - let current = std::env::var_os("PATH").unwrap_or_default(); - let already = std::env::split_paths(¤t).any(|p| p == dir); - if already { - return; +/// +/// Split out from the `set_var` above so the joining rule can be tested without +/// a test mutating the process environment out from under its neighbours. +fn path_with_dir_first( + current: &OsStr, + dir: &Path, +) -> Option> { + if std::env::split_paths(current).any(|p| p == dir) { + return None; } let joined = std::iter::once(dir.to_path_buf()) - .chain(std::env::split_paths(¤t)) + .chain(std::env::split_paths(current)) .collect::>(); - match std::env::join_paths(joined) { - // SAFETY: single-threaded startup — this runs from `main` before the - // daemon is spawned and before gpui's executor exists, so there is no - // concurrent reader of the environment. - Ok(path) => unsafe { std::env::set_var("PATH", path) }, - Err(e) => log::warn!("could not extend PATH with {}: {e}", dir.display()), - } + Some(std::env::join_paths(joined)) } // ---- Unix ------------------------------------------------------------------ #[cfg(unix)] -fn platform_install(cli: &Path) -> Outcome { - let path_dirs = path_dirs(); - let candidates = candidate_dirs(&path_dirs); +fn platform_install(cli: &Path, user_path: &[PathBuf]) -> Outcome { + let home = std::env::var_os("HOME").map(PathBuf::from); + let candidates = candidate_dirs(user_path, home.as_deref()); + let mode = Mode::current(); + let mut occupied = None; let mut last_error = None; for dir in &candidates { - match place(dir, cli) { + match place(dir, cli, mode) { Ok(Placement::Already(p)) => return Outcome::AlreadyInstalled(p), Ok(Placement::Wrote(p)) => { - return if path_dirs.contains(dir) { + return if user_path.contains(dir) { Outcome::Installed(p) } else { Outcome::InstalledOffPath(p) }; } - Ok(Placement::Occupied(p)) => return Outcome::Occupied(p), + // Someone else's `tty7` lives here. Keep going rather than giving + // up: a later candidate may be free, and if ours still loses the + // lookup the shadow check in `install_inner` says so. + Ok(Placement::Occupied(p)) => { + occupied.get_or_insert(p); + } Err(e) => last_error = Some(format!("{}: {e}", dir.display())), } } + if let Some(p) = occupied { + return Outcome::Occupied(p); + } Outcome::Failed(last_error.unwrap_or_else(|| "no writable directory on PATH".into())) } @@ -179,10 +284,12 @@ fn platform_install(cli: &Path) -> Outcome { /// `~/.local/bin` is the fallback and is offered even when PATH does not list /// it: an unreachable install the log names is a better outcome than no install /// at all, and it is the one directory here we can always create. +/// +/// `home` is a parameter rather than a `$HOME` read so tests can exercise this +/// without mutating the environment of every test running beside them. #[cfg(unix)] -fn candidate_dirs(path_dirs: &[PathBuf]) -> Vec { - let home = std::env::var_os("HOME").map(PathBuf::from); - let under_home = |rel: &str| home.as_ref().map(|h| h.join(rel)); +fn candidate_dirs(path_dirs: &[PathBuf], home: Option<&Path>) -> Vec { + let under_home = |rel: &str| home.map(|h| h.join(rel)); let preferred: Vec = [ Some(PathBuf::from("/opt/homebrew/bin")), @@ -213,22 +320,51 @@ enum Placement { Occupied(PathBuf), } -/// AppImage mounts itself at a fresh `/tmp/.mount_XXXX` every run, so a symlink -/// into the bundle is dangling the moment the app exits. Copy there instead. +/// Symlink, or copy for the one build that cannot be linked to. +/// +/// An AppImage mounts itself at a fresh `/tmp/.mount_XXXX` every run, so a +/// symlink into the bundle is dangling the moment the app exits. #[cfg(unix)] -fn running_from_appimage() -> bool { - std::env::var_os("APPIMAGE").is_some() +#[derive(Clone, Copy, PartialEq, Eq)] +enum Mode { + Symlink, + Copy, } #[cfg(unix)] -fn place(dir: &Path, cli: &Path) -> std::io::Result { +impl Mode { + /// The AppImage runtime sets `$APPIMAGE` to the bundle's own path. + fn current() -> Mode { + if std::env::var_os("APPIMAGE").is_some() { + Mode::Copy + } else { + Mode::Symlink + } + } +} + +/// The marker that says a real file under our name is a copy *we* made. +/// +/// [`Mode::Copy`] leaves a plain binary behind, indistinguishable from a +/// `cargo install` build or a package manager's — so the only honest way to +/// know it is ours is to have said so at the time. Keying off "am I an AppImage +/// right now" instead would strand the file forever the moment the user moved +/// to a tarball install: the copy would read as someone else's and never be +/// replaced. +#[cfg(unix)] +fn copy_marker(dir: &Path) -> PathBuf { + dir.join(format!(".{CLI_NAME}.installed-by-tty7")) +} + +#[cfg(unix)] +fn place(dir: &Path, cli: &Path, mode: Mode) -> std::io::Result { std::fs::create_dir_all(dir)?; let target = dir.join(CLI_NAME); match std::fs::symlink_metadata(&target) { Ok(meta) if meta.file_type().is_symlink() => { let points_at = std::fs::read_link(&target)?; - if points_at == cli { + if mode == Mode::Symlink && points_at == cli { return Ok(Placement::Already(target)); } // Replace only a link that is still aimed at something named @@ -239,13 +375,14 @@ fn place(dir: &Path, cli: &Path) -> std::io::Result { return Ok(Placement::Occupied(target)); } } - // A real file: a `cargo install` build, a package manager's copy, or - // the AppImage copy we made ourselves. Only the last is ours to touch. + // A real file: a `cargo install` build, a package manager's copy, or a + // copy we made ourselves on a previous launch. Only the last is ours to + // touch, and only the marker can tell us which it is. Ok(_) => { - if !running_from_appimage() { + if !copy_marker(dir).is_file() { return Ok(Placement::Occupied(target)); } - if same_size(&target, cli) { + if mode == Mode::Copy && same_size(&target, cli) { return Ok(Placement::Already(target)); } } @@ -253,7 +390,7 @@ fn place(dir: &Path, cli: &Path) -> std::io::Result { Err(e) => return Err(e), } - write_atomically(dir, &target, cli)?; + write_atomically(dir, &target, cli, mode)?; Ok(Placement::Wrote(target)) } @@ -278,19 +415,18 @@ fn same_size(a: &Path, b: &Path) -> bool { /// rename is atomic, so a concurrent shell either sees the old entry or the new /// one — never neither. #[cfg(unix)] -fn write_atomically(dir: &Path, target: &Path, cli: &Path) -> std::io::Result<()> { +fn write_atomically(dir: &Path, target: &Path, cli: &Path, mode: Mode) -> std::io::Result<()> { // The temp name carries the pid so two tty7 instances starting together // cannot collide on it. let tmp = dir.join(format!(".{CLI_NAME}.{}.tmp", std::process::id())); let _ = std::fs::remove_file(&tmp); - let result = if running_from_appimage() { - std::fs::copy(cli, &tmp).and_then(|_| { + let result = match mode { + Mode::Copy => std::fs::copy(cli, &tmp).and_then(|_| { use std::os::unix::fs::PermissionsExt as _; std::fs::set_permissions(&tmp, std::fs::Permissions::from_mode(0o755)) - }) - } else { - std::os::unix::fs::symlink(cli, &tmp) + }), + Mode::Symlink => std::os::unix::fs::symlink(cli, &tmp), }; if let Err(e) = result { let _ = std::fs::remove_file(&tmp); @@ -300,18 +436,80 @@ fn write_atomically(dir: &Path, target: &Path, cli: &Path) -> std::io::Result<() let _ = std::fs::remove_file(&tmp); return Err(e); } + // Claim the copy, or disown the one we just replaced with a symlink — a + // stale marker beside a link would hand the next non-AppImage launch a + // reason to overwrite a file it should have left alone. + match mode { + Mode::Copy => { + let _ = std::fs::write( + copy_marker(dir), + format!( + "{} was installed by the tty7 app, which replaces it on upgrade.\n\ + Delete this marker to have tty7 treat that binary as yours and leave \ + it alone.\n", + target.display() + ), + ); + } + Mode::Symlink => { + let _ = std::fs::remove_file(copy_marker(dir)); + } + } Ok(()) } -#[cfg(unix)] -fn path_dirs() -> Vec { - std::env::var_os("PATH") - .map(|p| std::env::split_paths(&p).collect()) - .unwrap_or_default() -} - // ---- Windows --------------------------------------------------------------- +/// The `Path` value to write back, or `None` when `dir` is already listed. +/// +/// UTF-16 in and UTF-16 out. Round-tripping the user's PATH through `String` +/// would run it past a lossy conversion, and a value the registry holds but +/// Rust cannot represent would come back with `U+FFFD` where its characters +/// used to be — the exact "installer permanently corrupts a PATH" failure the +/// rest of this function is careful to avoid. +/// +/// Built platform-independently so the joining and matching rules are testable +/// away from a real registry. +#[cfg(any(windows, test))] +fn user_path_with_dir(existing: &[u16], dir: &[u16]) -> Option> { + const SEMICOLON: u16 = b';' as u16; + const BACKSLASH: u16 = b'\\' as u16; + + fn trim_trailing(s: &[u16], c: u16) -> &[u16] { + &s[..s.iter().rposition(|&x| x != c).map_or(0, |i| i + 1)] + } + // ASCII folding only. Drive letters and separators are all that has to + // match case-insensitively here, and full Unicode case folding on a PATH + // entry would be a way to make two distinct directories compare equal. + fn fold(c: u16) -> u16 { + const UPPER: std::ops::RangeInclusive = (b'A' as u16)..=(b'Z' as u16); + if UPPER.contains(&c) { c + 32 } else { c } + } + fn same_entry(a: &[u16], b: &[u16]) -> bool { + let (a, b) = (trim_trailing(a, BACKSLASH), trim_trailing(b, BACKSLASH)); + a.len() == b.len() && a.iter().zip(b).all(|(&x, &y)| fold(x) == fold(y)) + } + + if existing + .split(|&c| c == SEMICOLON) + .any(|e| same_entry(e, dir)) + { + return None; + } + + // A trailing `;` is legal but leaves an empty entry, which some tools read + // as "the current directory" — trim before joining. + let head = trim_trailing(existing, SEMICOLON); + + let mut out = Vec::with_capacity(head.len() + 1 + dir.len()); + out.extend_from_slice(head); + if !head.is_empty() { + out.push(SEMICOLON); + } + out.extend_from_slice(dir); + Some(out) +} + /// Append the CLI's directory to the *user's* PATH in the registry. /// /// Reads `HKCU\Environment` rather than the process PATH on purpose. The @@ -319,8 +517,8 @@ fn path_dirs() -> Vec { /// back into the user hive would copy every system entry into HKCU — the /// classic way installers permanently corrupt a PATH. #[cfg(windows)] -fn platform_install(cli: &Path) -> Outcome { - use std::os::windows::ffi::{OsStrExt as _, OsStringExt as _}; +fn platform_install(cli: &Path, _user_path: &[PathBuf]) -> Outcome { + use std::os::windows::ffi::OsStrExt as _; use windows_sys::Win32::System::Registry::{ HKEY, HKEY_CURRENT_USER, KEY_READ, KEY_WRITE, REG_EXPAND_SZ, REG_SZ, RegCloseKey, RegOpenKeyExW, RegQueryValueExW, RegSetValueExW, @@ -333,15 +531,18 @@ fn platform_install(cli: &Path) -> Outcome { return Outcome::Failed("the CLI has no parent directory".into()); }; - let wide = |s: &str| { - std::ffi::OsStr::new(s) - .encode_wide() + let wide = |s: &OsStr| { + s.encode_wide() .chain(std::iter::once(0)) .collect::>() }; + let wide_str = |s: &str| wide(OsStr::new(s)); - let subkey = wide("Environment"); - let value_name = wide("Path"); + let subkey = wide_str("Environment"); + let value_name = wide_str("Path"); + // Without its terminator: this one is data to be matched and joined, not a + // string handed to the API. + let dir_wide: Vec = dir.as_os_str().encode_wide().collect(); let mut key: HKEY = std::ptr::null_mut(); // SAFETY: all pointers below are to live locals, and every out-parameter is @@ -362,7 +563,7 @@ fn platform_install(cli: &Path) -> Outcome { // profile and means we are writing the first entry, not an error. let mut kind = 0u32; let mut bytes = 0u32; - let existing = if RegQueryValueExW( + let existing: Vec = if RegQueryValueExW( key, value_name.as_ptr(), std::ptr::null_mut(), @@ -388,33 +589,19 @@ fn platform_install(cli: &Path) -> Outcome { while buf.last() == Some(&0) { buf.pop(); } - std::ffi::OsString::from_wide(&buf) - .to_string_lossy() - .into_owned() + buf } else { // Preserve REG_EXPAND_SZ if that is what was there; a fresh value // is a plain string. kind = REG_SZ; - String::new() + Vec::new() }; - let dir_str = dir.to_string_lossy(); - if existing.split(';').any(|e| { - e.trim_end_matches('\\') - .eq_ignore_ascii_case(dir_str.trim_end_matches('\\')) - }) { + let Some(mut updated) = user_path_with_dir(&existing, &dir_wide) else { RegCloseKey(key); return Outcome::AlreadyInstalled(cli.to_path_buf()); - } - - let updated = if existing.is_empty() { - dir_str.to_string() - } else { - // Trailing `;` is legal but leaves an empty entry, which some - // tools read as "the current directory" — trim before joining. - format!("{};{dir_str}", existing.trim_end_matches(';')) }; - let updated_wide = wide(&updated); + updated.push(0); let kind = if kind == REG_EXPAND_SZ { REG_EXPAND_SZ @@ -426,8 +613,8 @@ fn platform_install(cli: &Path) -> Outcome { value_name.as_ptr(), 0, kind, - updated_wide.as_ptr().cast(), - (updated_wide.len() * 2) as u32, + updated.as_ptr().cast(), + (updated.len() * 2) as u32, ); RegCloseKey(key); if written != 0 { @@ -438,7 +625,7 @@ fn platform_install(cli: &Path) -> Outcome { // change up: Explorer caches the environment it hands to what it // launches. The timeout keeps a hung top-level window from stalling // startup — the write already landed, so this is best-effort. - let env = wide("Environment"); + let env = wide_str("Environment"); SendMessageTimeoutW( HWND_BROADCAST, WM_SETTINGCHANGE, @@ -454,14 +641,90 @@ fn platform_install(cli: &Path) -> Outcome { } #[cfg(not(any(unix, windows)))] -fn platform_install(_cli: &Path) -> Outcome { +fn platform_install(_cli: &Path, _user_path: &[PathBuf]) -> Outcome { Outcome::Failed("unsupported platform".into()) } -#[cfg(all(test, unix))] +#[cfg(test)] mod tests { use super::*; + fn wide(s: &str) -> Vec { + s.encode_utf16().collect() + } + + fn from_wide(s: &[u16]) -> String { + String::from_utf16(s).unwrap() + } + + #[test] + fn a_build_tree_binary_is_recognised_under_both_profile_layouts() { + for p in [ + "/home/dev/tty7/target/debug/tty7", + "/home/dev/tty7/target/release/tty7", + "/home/dev/tty7/target/aarch64-apple-darwin/release/tty7", + ] { + assert!(in_a_build_tree(Path::new(p)), "{p} should read as a build"); + } + for p in [ + "/Applications/tty7.app/Contents/MacOS/tty7", + "/opt/tty7/tty7", + "/usr/local/bin/tty7", + // `release` with no `target` above it is somebody's install prefix. + "/opt/tty7/release/tty7", + ] { + assert!(!in_a_build_tree(Path::new(p)), "{p} should read as shipped"); + } + } + + #[test] + fn a_directory_already_on_path_is_not_prepended_twice() { + let current = std::env::join_paths(["/usr/bin", "/opt/tty7", "/bin"]).unwrap(); + assert!(path_with_dir_first(¤t, Path::new("/opt/tty7")).is_none()); + + let added = path_with_dir_first(¤t, Path::new("/opt/new")) + .expect("a fresh directory is added") + .expect("the join succeeds"); + let dirs: Vec = std::env::split_paths(&added).collect(); + assert_eq!(dirs.first(), Some(&PathBuf::from("/opt/new"))); + assert_eq!(dirs.len(), 4); + } + + #[test] + fn the_user_path_gains_the_directory_once_and_losslessly() { + // An unpaired surrogate: legal in the registry, not representable as a + // Rust `String`. It must come back out byte for byte. + let mut existing = wide("C:\\bin;C:\\weird"); + existing.push(0xD800); + + let updated = user_path_with_dir(&existing, &wide("C:\\tty7")).expect("a new entry"); + assert_eq!( + &updated[..existing.len()], + &existing[..], + "mangled the tail" + ); + assert_eq!(&updated[existing.len()..], &wide(";C:\\tty7")[..]); + + // Idempotent, and insensitive to case and to a trailing separator. + assert!(user_path_with_dir(&updated, &wide("C:\\tty7")).is_none()); + assert!(user_path_with_dir(&updated, &wide("c:\\TTY7")).is_none()); + assert!(user_path_with_dir(&updated, &wide("C:\\tty7\\")).is_none()); + } + + #[test] + fn a_trailing_separator_does_not_become_an_empty_path_entry() { + let updated = user_path_with_dir(&wide("C:\\bin;;"), &wide("C:\\tty7")).unwrap(); + assert_eq!(from_wide(&updated), "C:\\bin;C:\\tty7"); + + let fresh = user_path_with_dir(&[], &wide("C:\\tty7")).unwrap(); + assert_eq!(from_wide(&fresh), "C:\\tty7"); + } +} + +#[cfg(all(test, unix))] +mod unix_tests { + use super::*; + fn touch(p: &Path) { std::fs::create_dir_all(p.parent().unwrap()).unwrap(); std::fs::write(p, b"#!/bin/sh\n").unwrap(); @@ -481,16 +744,9 @@ mod tests { // get the binary deleted on the next rehash. let home = tmpdir("shims"); let shims = home.join(".pyenv/shims"); - std::fs::create_dir_all(&shims).unwrap(); let local = home.join(".local/bin"); - std::fs::create_dir_all(&local).unwrap(); - let path = vec![shims.clone(), local.clone()]; - let chosen = { - // `candidate_dirs` reads $HOME for the user-relative entries. - let _guard = EnvGuard::set("HOME", &home); - candidate_dirs(&path) - }; + let chosen = candidate_dirs(&[shims.clone(), local.clone()], Some(&home)); assert!(!chosen.contains(&shims), "shim dir was offered: {chosen:?}"); assert_eq!(chosen.first(), Some(&local)); } @@ -503,12 +759,10 @@ mod tests { // Someone's own build, installed by hand. touch(&dir.join("tty7")); - match place(&dir, &bin).unwrap() { + match place(&dir, &bin, Mode::Symlink).unwrap() { Placement::Occupied(p) => assert_eq!(p, dir.join("tty7")), - other => panic!( - "clobbered a real binary: {:?}", - matches!(other, Placement::Wrote(_)) - ), + Placement::Already(_) => panic!("claimed someone else's binary as ours"), + Placement::Wrote(_) => panic!("clobbered a real binary"), } } @@ -520,11 +774,15 @@ mod tests { touch(&v1); touch(&v2); - assert!(matches!(place(&dir, &v1).unwrap(), Placement::Wrote(_))); + let m = Mode::Symlink; + assert!(matches!(place(&dir, &v1, m).unwrap(), Placement::Wrote(_))); // Second launch, same build: nothing to do. - assert!(matches!(place(&dir, &v1).unwrap(), Placement::Already(_))); + assert!(matches!( + place(&dir, &v1, m).unwrap(), + Placement::Already(_) + )); // Upgraded install: the link follows it rather than reporting a clash. - assert!(matches!(place(&dir, &v2).unwrap(), Placement::Wrote(_))); + assert!(matches!(place(&dir, &v2, m).unwrap(), Placement::Wrote(_))); assert_eq!(std::fs::read_link(dir.join("tty7")).unwrap(), v2); } @@ -537,46 +795,94 @@ mod tests { touch(&elsewhere); std::os::unix::fs::symlink(&elsewhere, dir.join("tty7")).unwrap(); - assert!(matches!(place(&dir, &bin).unwrap(), Placement::Occupied(_))); + assert!(matches!( + place(&dir, &bin, Mode::Symlink).unwrap(), + Placement::Occupied(_) + )); } #[test] - fn the_process_path_gains_the_cli_directory_once() { - let dir = tmpdir("procpath"); - let before = std::env::var("PATH").unwrap_or_default(); - prepend_to_process_path(&dir); - let after = std::env::var("PATH").unwrap(); - assert!(after.starts_with(dir.to_str().unwrap()), "{after}"); + fn a_copy_we_made_stays_ours_after_the_user_moves_off_the_appimage() { + let dir = tmpdir("appimage-migrate"); + let v1 = tmpdir("appimage-mount-1").join("tty7"); + let v2 = tmpdir("appimage-mount-2").join("tty7"); + touch(&v1); + std::fs::write(&v2, b"#!/bin/sh\n# a later build\n").unwrap(); - prepend_to_process_path(&dir); - assert_eq!(std::env::var("PATH").unwrap(), after, "added twice"); - // SAFETY: single-threaded test. - unsafe { std::env::set_var("PATH", before) }; + // An AppImage run leaves a real file behind, plus the marker that says + // whose it is. + assert!(matches!( + place(&dir, &v1, Mode::Copy).unwrap(), + Placement::Wrote(_) + )); + assert!(!dir.join("tty7").is_symlink(), "should be a real copy"); + assert!(copy_marker(&dir).is_file(), "the copy went unclaimed"); + + // Same AppImage again: the sizes match, so there is nothing to do. + assert!(matches!( + place(&dir, &v1, Mode::Copy).unwrap(), + Placement::Already(_) + )); + // A newer AppImage: replaced, not refused. + assert!(matches!( + place(&dir, &v2, Mode::Copy).unwrap(), + Placement::Wrote(_) + )); + + // The user switches to the tarball. Without the marker this would read + // as someone else's binary and the install would be stuck forever. + assert!(matches!( + place(&dir, &v2, Mode::Symlink).unwrap(), + Placement::Wrote(_) + )); + assert_eq!(std::fs::read_link(dir.join("tty7")).unwrap(), v2); + assert!( + !copy_marker(&dir).exists(), + "a symlink must not keep the copy's marker" + ); } - struct EnvGuard { - key: &'static str, - prev: Option, - } + #[test] + fn an_occupied_directory_does_not_end_the_search() { + let taken = tmpdir("scan-taken"); + let free = tmpdir("scan-free"); + let bin = tmpdir("scan-src").join("tty7"); + touch(&bin); + touch(&taken.join("tty7")); - impl EnvGuard { - fn set(key: &'static str, value: &Path) -> EnvGuard { - let prev = std::env::var_os(key); - // SAFETY: single-threaded test. - unsafe { std::env::set_var(key, value) }; - EnvGuard { key, prev } - } - } - - impl Drop for EnvGuard { - fn drop(&mut self) { - // SAFETY: single-threaded test. - unsafe { - match self.prev.take() { - Some(v) => std::env::set_var(self.key, v), - None => std::env::remove_var(self.key), - } + // Stand in for the candidate loop: the first directory is somebody + // else's, and the second one must still get the link. + let mut wrote = None; + for dir in [&taken, &free] { + if let Ok(Placement::Wrote(p)) = place(dir, &bin, Mode::Symlink) { + wrote = Some(p); + break; } } + assert_eq!(wrote, Some(free.join("tty7"))); + } + + #[test] + fn the_shadow_check_names_whoever_wins_the_lookup() { + let early = tmpdir("shadow-early"); + let ours = tmpdir("shadow-ours"); + touch(&early.join("tty7")); + touch(&ours.join("tty7")); + + let path = vec![early.clone(), ours.clone()]; + assert_eq!(first_cli_on(&path), Some(early.join("tty7"))); + // Our own directory first: no shadow. + assert_eq!( + first_cli_on(&[ours.clone(), early.clone()]), + Some(ours.join("tty7")) + ); + + // A dangling link is not something that wins a lookup. + let dangling = tmpdir("shadow-dangling"); + std::os::unix::fs::symlink(dangling.join("gone"), dangling.join("tty7")).unwrap(); + assert_eq!( + first_cli_on(&[dangling, ours.clone()]), + Some(ours.join("tty7")) + ); } } diff --git a/src/main.rs b/src/main.rs index 502af410..88967183 100644 --- a/src/main.rs +++ b/src/main.rs @@ -212,13 +212,15 @@ fn main() { #[cfg(unix)] enrich_path_from_login_shell(); + let config = crate::core::config::Config::load(); + // After the PATH enrichment above, which is what makes the candidate scan // see the user's real PATH rather than the stub a Finder launch inherits — // and before the daemon below, which forks every pane and so must already // carry the CLI's directory in its environment. - crate::core::cli_install::install(); + crate::core::cli_install::install(config.install_cli_on_path); - let restore_session = crate::core::config::Config::load().restore_session; + let restore_session = config.restore_session; let daemon_result = if restore_session { crate::daemon::spawn::ensure_running() } else { diff --git a/src/ui/app.rs b/src/ui/app.rs index 84991d71..c8daac8d 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -1908,6 +1908,13 @@ impl Tty7App { self.update_config(cx, |cfg| cfg.check_for_updates = on); } + /// Takes effect at next launch: `core::cli_install` runs once from `main`, + /// before there is a window to flip this in. Turning it off does not remove + /// a symlink already placed — the install is idempotent, not reversible. + pub(crate) fn set_install_cli_on_path(&mut self, on: bool, cx: &mut Context) { + self.update_config(cx, |cfg| cfg.install_cli_on_path = on); + } + pub(crate) fn set_dim_inactive_panes(&mut self, on: bool, cx: &mut Context) { self.update_config(cx, |cfg| cfg.dim_inactive_panes = on); } diff --git a/src/ui/settings.rs b/src/ui/settings.rs index a60e086d..b04070e8 100644 --- a/src/ui/settings.rs +++ b/src/ui/settings.rs @@ -369,6 +369,11 @@ fn settings_search_entries() -> &'static [SearchEntry] { title: "How shells work", keywords: "shell session daemon server detach persist background close quit stop delete workspace layout survive reboot tmux", }, + SearchEntry { + section: About, + title: "Command line tool", + keywords: "cli tty7 path shell command install symlink terminal iterm agent script", + }, ] } @@ -4489,6 +4494,7 @@ impl Tty7App { .try_global::() .and_then(|s| s.available.clone()); let check_for_updates = cx.global::().check_for_updates; + let install_cli_on_path = cx.global::().install_cli_on_path; let logo = Arc::new(Image::from_bytes( ImageFormat::Png, @@ -4596,6 +4602,40 @@ impl Tty7App { ), ), ) + .child( + v_flex() + .mt_6() + .gap_2() + .child(self.section_rule(cx)) + .child( + div() + .text_sm() + .font_weight(FontWeight::MEDIUM) + .text_color(foreground) + .child("Command line"), + ) + .child(div().text_sm().text_color(muted_fg).child( + "Put the bundled `tty7` command on your PATH at launch, so scripts and coding agents can drive tty7 from any terminal. Inside a tty7 pane it works either way. Turn this off if you keep your own `tty7` — one you built or installed yourself — and do not want it shadowed. Takes effect at next launch.", + )) + .child( + h_flex() + .gap_2() + .items_center() + .child( + crate::ui::theme::switch("install-cli-on-path", cx) + .checked(install_cli_on_path) + .on_click(cx.listener(|this, on: &bool, _w, cx| { + this.set_install_cli_on_path(*on, cx) + })), + ) + .child( + div() + .text_sm() + .text_color(foreground) + .child("Install the `tty7` command on PATH"), + ), + ), + ) .child( v_flex() .mt_6()