diff --git a/src/invocation.rs b/src/invocation.rs index 83398ff1..47b06142 100644 --- a/src/invocation.rs +++ b/src/invocation.rs @@ -43,12 +43,7 @@ pub fn no_run_command(project_id: &str) -> String { } fn resolves_on_path(bin: &str) -> bool { - let Some(paths) = crate::local::shell_env::search_path() else { - return false; - }; - std::env::split_paths(&paths) - .filter(|dir| !dir.as_os_str().is_empty()) - .any(|dir| dir.join(bin).is_file()) + crate::local::shell_env::find_on_path(bin).is_some() } fn quote_for_shell(path: &str) -> String { diff --git a/src/local/harness/claude.rs b/src/local/harness/claude.rs index 8a5db340..0d54b19a 100644 --- a/src/local/harness/claude.rs +++ b/src/local/harness/claude.rs @@ -30,8 +30,7 @@ use tokio::io::{AsyncBufReadExt, BufReader}; use tokio::process::Command; use super::detect::{ - bin_version, find_on_path, nonempty_str, parse_version, read_json, HarnessAuthState, - HarnessInfo, ModelInfo, + bin_version, nonempty_str, parse_version, read_json, HarnessAuthState, HarnessInfo, ModelInfo, }; use super::options::{ HarnessOptions, OptionChoice, PermissionMode, PlanActivation, REASONING_DEFAULT_ID, @@ -46,6 +45,7 @@ use crate::local::chat::{ }; use crate::local::claude::{SpawnConfig, SpawnSpec, TurnEvent}; use crate::local::opencode::ensure_playbook; +use crate::local::shell_env::find_on_path; /// FALLBACK model list, used only when the `list_models` control request fails /// (a CLI too old to answer it, or a spawn/timeout failure). The primary source diff --git a/src/local/harness/codex.rs b/src/local/harness/codex.rs index b94f8da2..9077b697 100644 --- a/src/local/harness/codex.rs +++ b/src/local/harness/codex.rs @@ -38,8 +38,8 @@ use tokio::io::{AsyncBufReadExt, BufReader}; use tokio::process::Command; use super::detect::{ - bin_version, find_on_path, jwt_payload, nonempty_str, parse_version, read_json, - resolve_symlinks, title_case, HarnessInfo, ModelInfo, + bin_version, jwt_payload, nonempty_str, parse_version, read_json, resolve_symlinks, title_case, + HarnessInfo, ModelInfo, }; use super::options::{ resolve_reasoning, HarnessOptions, OptionChoice, PermissionMode, PlanActivation, @@ -57,6 +57,7 @@ use crate::local::chat::{ }; use crate::local::codex::{CodexClient, JsonRpcError, ServerReqKind, TurnEvent}; use crate::local::opencode::ensure_playbook; +use crate::local::shell_env::find_on_path; use crate::store::{Store, StoredChatMessage}; // FALLBACK model table, used only when the app-server catalog is unreachable diff --git a/src/local/harness/detect.rs b/src/local/harness/detect.rs index 0b76e117..5ac8f73f 100644 --- a/src/local/harness/detect.rs +++ b/src/local/harness/detect.rs @@ -166,16 +166,6 @@ impl HarnessInfo { } } -pub(super) fn find_on_path(bin: &str) -> Option { - let paths = crate::local::shell_env::search_path()?; - std::env::split_paths(&paths) - // An empty component (`PATH=":/usr/bin"`) means cwd — never a place to - // pick up a binary we are about to execute. - .filter(|dir| !dir.as_os_str().is_empty()) - .map(|dir| dir.join(bin)) - .find(|c| c.is_file()) -} - /// Dereference symlinks to the real installed binary. Installers commonly drop /// a lone symlink into `~/.local/bin`, but some CLIs locate sibling helper /// executables relative to the path they were *invoked as*, without resolving diff --git a/src/local/latex.rs b/src/local/latex.rs index 50ef3d9d..501d23a8 100644 --- a/src/local/latex.rs +++ b/src/local/latex.rs @@ -19,6 +19,7 @@ use std::sync::mpsc; use std::time::{Duration, Instant}; use crate::error::{anyhow, Result}; +use crate::local::shell_env::{find_on_path, search_path}; /// The TeX engine a document is written for. Overleaf's default is pdfLaTeX and /// so is ours; a document says otherwise with a `% !TeX program` line. @@ -133,15 +134,27 @@ pub struct Compilation { pub log: String, } -/// True when a binary answers `--version` on this machine's PATH. +/// True when a file of that name sits on the shell's PATH — not the process's, +/// which in app mode is launchd's and has no TeX on it. That it runs is not +/// checked; `find_engine` probes on every `.tex` tab and spawning five engines +/// to ask their versions cost more than the case it caught. fn on_path(binary: &str) -> bool { - Command::new(binary) - .arg("--version") - .stdin(Stdio::null()) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status() - .is_ok_and(|status| status.success()) + find_on_path(binary).is_some() +} + +/// A TeX tool, with the shell's PATH in its environment: latexmk finds the +/// engine, biber and bibtex on PATH, so resolving latexmk alone is not enough. +fn tex_command(binary: &str) -> Command { + // biber is never probed — it is chosen from what a pass wrote — so a bare + // name here is what makes a machine without it fail as ENOENT at spawn. + let mut command = match find_on_path(binary) { + Some(path) => Command::new(path), + None => Command::new(binary), + }; + if let Some(paths) = search_path() { + command.env("PATH", paths); + } + command } /// Pick how to compile a document written for `program`. Split from the PATH @@ -191,8 +204,9 @@ pub fn find_engine() -> Option<&'static str> { .find(|binary| on_path(binary)) } -/// Guidance for the no-engine case. A full distribution comes first because it -/// is what matches Overleaf — every engine, biber, and the whole of CTAN. +/// Guidance for the no-engine case. Tectonic comes first because it is the one +/// a user can finish, and its XeTeX-only trade is named here because this is +/// the last screen that can say so — the hint is gone once an engine exists. pub fn install_hint() -> String { let distribution = if cfg!(target_os = "macos") { "MacTeX" @@ -200,9 +214,10 @@ pub fn install_hint() -> String { "TeX Live" }; format!( - "No LaTeX toolchain found on PATH. Install {distribution} for the same \ - engines and packages Overleaf runs, or Tectonic for a smaller \ - XeTeX-only setup that needs no distribution." + "No LaTeX toolchain found on PATH. Install Tectonic — one self-contained \ + binary that fetches each document's packages, though it runs only XeTeX. \ + For every engine and package, install {distribution} instead, which is \ + what Overleaf runs." ) } @@ -210,7 +225,7 @@ pub fn install_hint() -> String { /// known to work — a wrong command pasted into a terminal is worse than none. pub fn install_command() -> Option<&'static str> { if cfg!(target_os = "macos") { - Some("brew install --cask mactex") + Some("brew install tectonic") } else { None } @@ -298,7 +313,7 @@ fn run_bibliography( // cwd is the aux dir (bibtex refuses to write outside it), so `.` no longer // means the paper's directory — both search paths have to say so. let search = format!("{}:", source_dir.to_string_lossy()); - let mut command = Command::new(tool); + let mut command = tex_command(tool); command .arg(stem) .current_dir(outdir) @@ -353,7 +368,7 @@ struct Run { } fn run_pass(binary: &str, dir: &Path, args: &[String]) -> Result { - let mut command = Command::new(binary); + let mut command = tex_command(binary); command.args(args).current_dir(dir); run_command(command, binary) } @@ -838,20 +853,46 @@ mod tests { } #[test] - fn the_no_engine_guidance_points_at_a_full_distribution_first() { + fn the_no_engine_guidance_points_at_tectonic_first() { let hint = install_hint(); + let distribution = if cfg!(target_os = "macos") { + "MacTeX" + } else { + "TeX Live" + }; + let tectonic = hint.find("Tectonic").expect("the recommendation is named"); + let fallback = hint.find(distribution).expect("the fallback is named"); assert!( - hint.contains("Overleaf"), - "parity is the reason to install it" + tectonic < fallback, + "the install a user can finish comes first" ); - // The prose must not duplicate the copyable command. + // The caveat has to sit in Tectonic's own clause: recommending it + // without one sends a user to an engine that silently retypesets their + // pdfLaTeX document, and this hint is the only place that says so. + let xetex = hint.find("XeTeX").expect("the limitation is named"); + assert!(tectonic < xetex && xetex < fallback); + // The prose must not duplicate the copyable command, but the command + // must install what the prose leads with. assert!(!hint.contains("brew install")); #[cfg(target_os = "macos")] - assert_eq!(install_command(), Some("brew install --cask mactex")); + assert_eq!(install_command(), Some("brew install tectonic")); #[cfg(not(target_os = "macos"))] assert_eq!(install_command(), None); } + #[test] + fn a_tex_tool_is_handed_the_path_its_own_children_need() { + // `sh` stands in for a TeX tool; all that matters is that it is on PATH. + let command = tex_command("sh"); + let (_, value) = command + .get_envs() + .find(|(key, _)| *key == "PATH") + .expect("the child is given an explicit PATH"); + assert_eq!(value, search_path().as_deref()); + // And the tool itself is resolved, not left to the child's own lookup. + assert!(Path::new(command.get_program()).is_absolute()); + } + #[test] fn tectonic_does_not_treat_recoverable_errors_as_fatal() { // microtype under XeTeX errors with "switching it off" and carries on diff --git a/src/local/opencode.rs b/src/local/opencode.rs index ae7a0165..39a7ca94 100644 --- a/src/local/opencode.rs +++ b/src/local/opencode.rs @@ -31,15 +31,8 @@ const HEALTH_TIMEOUT: Duration = Duration::from_secs(30); /// `opencode` on PATH, else the installer's default drop location. pub fn find_opencode() -> Result { - if let Some(paths) = crate::local::shell_env::search_path() { - // An empty component (`PATH=":/usr/bin"`) means cwd — never a place to - // pick up a binary we are about to execute. Mirrors `find_on_path`. - for dir in std::env::split_paths(&paths).filter(|dir| !dir.as_os_str().is_empty()) { - let candidate = dir.join("opencode"); - if candidate.is_file() { - return Ok(candidate); - } - } + if let Some(found) = crate::local::shell_env::find_on_path("opencode") { + return Ok(found); } if let Some(home) = dirs::home_dir() { let fallback = home.join(".opencode").join("bin").join("opencode"); diff --git a/src/local/shell_env.rs b/src/local/shell_env.rs index f6330f17..681abebb 100644 --- a/src/local/shell_env.rs +++ b/src/local/shell_env.rs @@ -17,7 +17,8 @@ //! `publish-branch` worker — still inherit the process environment. use std::collections::HashMap; -use std::ffi::OsString; +use std::ffi::{OsStr, OsString}; +use std::path::PathBuf; use std::sync::OnceLock; /// Deliberately short. These are the variables whose divergence makes the app @@ -35,11 +36,29 @@ pub fn var(key: &str) -> Option { .or_else(|| std::env::var_os(key)) } -/// The PATH to search for harness binaries and hand to harness children. +/// The PATH to search for the binaries orx spawns, and to hand its children. pub fn search_path() -> Option { var("PATH") } +/// Where `binary` lives, or None when this machine has no such tool. The path +/// is returned as it sits on PATH; a caller that needs the real binary behind a +/// symlink composes with `resolve_symlinks`. +pub fn find_on_path(binary: &str) -> Option { + search_in(&search_path()?, binary) +} + +/// Split from the PATH lookup so the search is testable without a probe. +fn search_in(paths: &OsStr, binary: &str) -> Option { + // A relative entry (`""`, meaning cwd, or `bin`) names no fixed directory — + // it resolves against whichever cwd is current, so it is never a place to + // pick up a binary. + std::env::split_paths(paths) + .filter(|dir| dir.is_absolute()) + .map(|dir| dir.join(binary)) + .find(|candidate| candidate.is_file()) +} + /// Hand the imported variables to a child process. Every `orx` child re-resolves /// its directories from its own environment and only app mode ever probes, so /// without this a supervisor spawned by the app would write to the default @@ -103,6 +122,29 @@ mod tests { format!("nvm loaded\n{M}{payload}{M}") } + #[test] + fn the_search_skips_relative_entries_and_takes_the_first_absolute_hit() { + let root = std::env::temp_dir().join(format!("orx-path-search-{}", std::process::id())); + let (early, late) = (root.join("early"), root.join("late")); + std::fs::create_dir_all(&early).expect("early"); + std::fs::create_dir_all(&late).expect("late"); + std::fs::write(early.join("tool"), "").expect("early tool"); + std::fs::write(late.join("tool"), "").expect("late tool"); + + let paths = + std::env::join_paths([PathBuf::new(), PathBuf::from("bin"), early.clone(), late]) + .expect("join"); + assert_eq!(search_in(&paths, "tool"), Some(early.join("tool"))); + assert_eq!(search_in(&paths, "absent"), None); + + // A relative entry is rejected even when it does resolve: cargo runs + // tests from the package root, so `src/main.rs` is a real hit here. + let relative = std::env::join_paths([PathBuf::from("src")]).expect("join"); + assert_eq!(search_in(&relative, "main.rs"), None); + + std::fs::remove_dir_all(&root).ok(); + } + #[test] fn reads_every_imported_variable() { let vars = parse_probe(