From fa5b901d8d0adbe47f0db2fa2afd512a52c51f6b Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Sun, 23 Aug 2026 06:11:12 +0800 Subject: [PATCH] fix(doctor): check the shell every new tab is going to launch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit $ cat config.json { "shell": { "program": "/nonexistent/shell" } } $ tty7 new tty7: spawning a shell: daemon refused Spawn: no such program on this machine: /nonexistent/shell $ tty7 doctor | grep config config ok Nothing can open a tab, and the verb that checks the install says the config is fine — which it is, in the only sense that row means: the file parses. The same table already reports an unusable `custom_shells` entry, and that costs a menu row. This costs every tab, every `tty7 new`, and the GUI's new-tab button. The check is the daemon's own `shell_program_problem`, moved from `daemon::pane` to `core::shells` beside `unusable_custom_shells` so both callers share one definition and the row cannot drift from the refusal it is predicting — it prints the same sentence the spawn will. Missing, a directory, and not executable are told apart, because they are three different fixes. A program given as a bare name is still not reported: the OS resolves it through PATH and guessing at that would be worse than silence — the moved comment says so and the moved tests pin it. Skipped under `-m` for the same reason the hooks row is: a path checked here would answer about the wrong machine. Verified against a live daemon across all five cases — missing, directory, non-executable, bare name, and no `shell` set — with exit 1 for the three that break and 0 for the two that do not. --- CHANGELOG.md | 10 +++++++++ crates/tty7-cli/src/commands.rs | 32 ++++++++++++++++++++++++++++- crates/tty7-core/src/core/shells.rs | 31 ++++++++++++++++++++++++++++ crates/tty7-core/src/daemon/pane.rs | 30 +-------------------------- docs/cli/reference.mdx | 10 ++++++--- 5 files changed, 80 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5557b500..d18bd88a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -153,6 +153,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **`doctor` now checks the configured shell.** A `shell` naming something + that is not there — missing, a directory, not executable — makes every new + tab and every `tty7 new` fail, while `doctor` reported `config ok`, because + the file parses perfectly well. It already checked `custom_shells`, which + only costs a menu row; the setting that costs the whole app was the one + nobody asked about. The check is the daemon's own, moved somewhere both can + reach rather than written twice, so the row says exactly what the refusal + will say. A shell given as a bare name is still left to `PATH`, and under + `-m` the row is skipped, because the shell resolves on the far machine. + - **A config file that will not load now says what is wrong and where.** `doctor` reported "NOT VALID JSON" for three different mistakes, and two of them were valid JSON: `"font_size": "big"`, or a string where an object goes, diff --git a/crates/tty7-cli/src/commands.rs b/crates/tty7-cli/src/commands.rs index f19c336e..2a79a3df 100644 --- a/crates/tty7-cli/src/commands.rs +++ b/crates/tty7-cli/src/commands.rs @@ -1882,6 +1882,32 @@ fn doctor(ctx: &Context, backend: &mut dyn Backend) -> Result { ), ]); } + // The configured shell is what every new tab and every `tty7 new` launches, + // so a `shell` naming something that is not there breaks all of them — and + // the config row above says `ok`, because the file parses perfectly well. + // `custom_shells` was already checked here and only costs a menu row; this + // one costs the whole app, and was the one nobody asked about. + // + // Local only, like the hooks row: under `-m` the shell is resolved on the + // far machine and a path checked here would answer about the wrong disk. + let shell_problem = config_ok + .then(|| { + backend.is_this_machine().then(|| { + loaded + .0 + .shell + .as_ref() + .and_then(|s| tty7_core::core::shells::program_problem(&s.program)) + }) + }) + .flatten() + .flatten(); + if let Some(problem) = &shell_problem { + rows.push(vec![ + "shell".to_string(), + format!("{problem} — every new tab and `tty7 new` fails until this is fixed"), + ]); + } if !config_dir_writable { rows.push(vec![ "config dir".to_string(), @@ -2004,6 +2030,7 @@ fn doctor(ctx: &Context, backend: &mut dyn Backend) -> Result { "ok": config_ok, "state": config_state, "dir_writable": config_dir_writable, + "shell_problem": shell_problem, }, "hooks": hooks_json(&hooks), }), @@ -2018,6 +2045,9 @@ fn doctor(ctx: &Context, backend: &mut dyn Backend) -> Result { if !config_dir_writable { eprintln!("tty7: doctor: the config directory cannot be written to"); } + if shell_problem.is_some() { + eprintln!("tty7: doctor: the configured shell cannot be launched"); + } if report.json["server"]["reachable"] == false { // doctor is the verb people run when something is not working, so an // unreachable server is *the* finding — not a row to exit 0 over: @@ -2026,7 +2056,7 @@ fn doctor(ctx: &Context, backend: &mut dyn Backend) -> Result { eprintln!("tty7: doctor: the server is unreachable"); return Ok(Outcome::Exit(1, report)); } - if !config_ok || !config_dir_writable { + if !config_ok || !config_dir_writable || shell_problem.is_some() { return Ok(Outcome::Exit(1, report)); } Ok(Outcome::Report(report)) diff --git a/crates/tty7-core/src/core/shells.rs b/crates/tty7-core/src/core/shells.rs index c300e054..cb66ba33 100644 --- a/crates/tty7-core/src/core/shells.rs +++ b/crates/tty7-core/src/core/shells.rs @@ -61,6 +61,37 @@ pub fn inventory() -> ShellInventory { inventory } +/// Why `program` could not be launched, if there is a reason to be had. +/// +/// "Program", not "shell": the same check stands in front of the configured +/// shell and of a command handed to `tty7 run`, and calling the latter a shell +/// tells someone who typed `tty7 run -- ./build.sh` that tty7 misunderstood +/// what they asked for. The caller supplies the context — the CLI prefixes +/// "spawning `…`" — so the sentence only has to carry the fact. +/// +/// Only a program given as a path can be checked here; a bare name is resolved +/// through PATH by the OS, and guessing at that would be worse than silence. +pub fn program_problem(program: &str) -> Option { + let path = std::path::Path::new(program); + if path.components().count() < 2 { + return None; + } + let Ok(meta) = std::fs::metadata(path) else { + return Some(format!("no such program on this machine: {program}")); + }; + if meta.is_dir() { + return Some(format!("that program is a directory: {program}")); + } + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + if meta.permissions().mode() & 0o111 == 0 { + return Some(format!("that program is not executable: {program}")); + } + } + None +} + /// Adds the user's own menu entries after everything that was detected. /// /// After, not among: the detected list is ordered so the shell a new tab diff --git a/crates/tty7-core/src/daemon/pane.rs b/crates/tty7-core/src/daemon/pane.rs index fee93012..8e4dd328 100644 --- a/crates/tty7-core/src/daemon/pane.rs +++ b/crates/tty7-core/src/daemon/pane.rs @@ -11,6 +11,7 @@ use portable_pty::{Child, CommandBuilder, MasterPty, PtySize, native_pty_system} use crate::core::kitty_graphics::{GraphicsSniffer, Segment, Sniffed}; use crate::core::osc::OscTokenizer; +use crate::core::shells::program_problem as shell_program_problem; use crate::daemon::protocol::{ AuthResponse, DaemonMsg, MAX_FRAME, NativeSshSpec, PaneInfo, RemoteContext, RemoteKind, ShellSpec, WinSize, @@ -127,35 +128,6 @@ struct SpawnConfig { /// "daemon refused Spawn: spawn failed: Unable to spawn … (ENOENT: No such /// file or directory)" — and the one fact that matters is buried in it. /// -/// "Program", not "shell": the same check stands in front of the configured -/// shell and of a command handed to `tty7 run`, and calling the latter a shell -/// tells someone who typed `tty7 run -- ./build.sh` that tty7 misunderstood -/// what they asked for. The caller supplies the context — the CLI prefixes -/// "spawning `…`" — so the sentence only has to carry the fact. -/// -/// Only a program given as a path can be checked here; a bare name is resolved -/// through PATH by the OS, and guessing at that would be worse than silence. -fn shell_program_problem(program: &str) -> Option { - let path = std::path::Path::new(program); - if path.components().count() < 2 { - return None; - } - let Ok(meta) = std::fs::metadata(path) else { - return Some(format!("no such program on this machine: {program}")); - }; - if meta.is_dir() { - return Some(format!("that program is a directory: {program}")); - } - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - if meta.permissions().mode() & 0o111 == 0 { - return Some(format!("that program is not executable: {program}")); - } - } - None -} - fn build_spawn_config( pane: u64, cwd: Option, diff --git a/docs/cli/reference.mdx b/docs/cli/reference.mdx index 47a1b28e..d12f0c5a 100644 --- a/docs/cli/reference.mdx +++ b/docs/cli/reference.mdx @@ -320,7 +320,7 @@ nothing, so `tty7 agents` shows it standing still and `tty7 wait` sits there until it times out. Outdated hooks fail the same quiet way. Hooks are a local install, so under `-m` the row reads `unknown`. -JSON: `{"context":{"config_dir","workspace","pane"},"server":{"reachable","dialect_ok","build","status","routes"},"config":{"ok","state","dir_writable"},"hooks":{"installed","outdated","not_installed"}}` +JSON: `{"context":{"config_dir","workspace","pane"},"server":{"reachable","dialect_ok","build","status","routes"},"config":{"ok","state","dir_writable","shell_problem"},"hooks":{"installed","outdated","not_installed"}}` — the context fields are booleans, not values, and each `hooks` field is a list of agent slugs. `context` also carries `workspace_gone` and `pane_gone` when a server answered and could be asked; both are absent when none did, so that "it @@ -329,8 +329,12 @@ false only when the file failed to parse and tty7 is running on defaults; `config.state` is the same sentence the table prints. `config.dir_writable` is probed by writing, not read off the mode bits, because a read-only mount or an ACL leaves a `0700` directory that refuses every write — false means settings -cannot be saved and `tty7 new` cannot file a workspace. Either of those false -exits `1`, so `tty7 doctor || alert` fires. The extra rows the table can show — +cannot be saved and `tty7 new` cannot file a workspace. `config.shell_problem` is null unless the +configured `shell` names something that cannot be launched — missing, a +directory, not executable — in which case every new tab and every `tty7 new` +fails; a shell given as a bare name is left to `PATH` and never reported here, +and under `-m` the field is null because the shell resolves on the far machine. +Any of those three exits `1`, so `tty7 doctor || alert` fires. The extra rows the table can show — unread keys, unusable `custom_shells` — are prose, not JSON fields. ## `ws` — workspaces