From 4bde0723558e88c62e9ff638408d9abaada3215d Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Sun, 8 Nov 2026 23:59:30 +0100 Subject: [PATCH] refactor(desktop): setup, install and probe code: shared helpers, honest docs No behavior change. - ProbeOutcome::version replaces two copies of the same match in setup.rs, and classify_response reads the status JSON with serde_json's indexing (same answer for every body, a missing key is Null). - local_server::is_env_name is the one test for a variable name, used by the login shell's environment and by ~/.codeman/.env. - setup.rs has basename() for its two command-line scans; the install scan no longer formats the script prefix once per word. - agents.rs: refused() names the statuses that end the polling, and answer() takes the body it already has instead of a closure. - Small simplifications in gui_install.rs (Run::matches, follow, the download error text) and a let-else in local_server::launch. - Docs that no longer matched the code: probe.rs said the probe never runs in a loop, but the This Mac tab re-asks while it waits for an install (every 3 s) or for a starting server (every second); local_server.rs covered only Start; FIND_TIMEOUT and exec named one caller of several. Co-Authored-By: Claude Opus 5.5 --- packages/desktop/src/agents.rs | 37 +++++++++++++------------ packages/desktop/src/gui_install.rs | 12 ++++----- packages/desktop/src/local_server.rs | 40 +++++++++++++++------------- packages/desktop/src/probe.rs | 30 ++++++++++++++------- packages/desktop/src/setup.rs | 39 ++++++++++++--------------- 5 files changed, 85 insertions(+), 73 deletions(-) diff --git a/packages/desktop/src/agents.rs b/packages/desktop/src/agents.rs index ca4e1c13..fb4da6d4 100644 --- a/packages/desktop/src/agents.rs +++ b/packages/desktop/src/agents.rs @@ -115,19 +115,22 @@ fn session_cookie(header: &str) -> Option<&str> { (name == COOKIE_NAME && !value.is_empty()).then_some(pair) } +/// A refused login: Codeman counts each against the login rate limit, so the +/// polling ends. +fn refused(status: u16) -> bool { + matches!(status, 401 | 403 | 429) +} + /// What a status code says about asking again. -fn answer(status: u16, body: impl FnOnce() -> Option) -> Poll { +fn answer(status: u16, body: Option<&str>) -> Poll { match status { - 200 => body() - .as_deref() - .and_then(count_working) - .map_or(Poll::Failed, |working| { - Poll::Seen(Activity { - working: working > 0, - ..Activity::default() - }) - }), - 401 | 403 | 429 => Poll::Refused, + 200 => body.and_then(count_working).map_or(Poll::Failed, |working| { + Poll::Seen(Activity { + working: working > 0, + ..Activity::default() + }) + }), + s if refused(s) => Poll::Refused, _ => Poll::Failed, } } @@ -192,7 +195,7 @@ impl Poller { let Some((status, body)) = self.ask(&url) else { return Poll::Failed; }; - let mut activity = match answer(status, || body) { + let mut activity = match answer(status, body.as_deref()) { Poll::Seen(activity) => activity, other => return other, }; @@ -203,7 +206,7 @@ impl Poller { self.alerts = alerts; } } - Some((401 | 403 | 429, _)) => return Poll::Refused, + Some((s, _)) if refused(s) => return Poll::Refused, // A server from before the list has no alerts to report. Some((404, _)) => self.alerts = (false, false), _ => {} @@ -264,17 +267,17 @@ mod tests { fn a_body_that_is_not_the_list_is_no_answer() { assert_eq!(count_working(""), None); assert_eq!(count_working(r#"{"success":false,"error":"nope"}"#), None); - assert_eq!(answer(200, || Some("".into())), Poll::Failed); - assert_eq!(answer(200, || None), Poll::Failed); + assert_eq!(answer(200, Some("")), Poll::Failed); + assert_eq!(answer(200, None), Poll::Failed); } #[test] fn only_a_login_refusal_ends_the_polling() { for status in [401, 403, 429] { - assert_eq!(answer(status, || None), Poll::Refused, "{status}"); + assert_eq!(answer(status, None), Poll::Refused, "{status}"); } for status in [301, 404, 500, 502, 503] { - assert_eq!(answer(status, || None), Poll::Failed, "{status}"); + assert_eq!(answer(status, None), Poll::Failed, "{status}"); } } diff --git a/packages/desktop/src/gui_install.rs b/packages/desktop/src/gui_install.rs index 831e6355..73430701 100644 --- a/packages/desktop/src/gui_install.rs +++ b/packages/desktop/src/gui_install.rs @@ -100,8 +100,8 @@ impl Run { /// not one: it has no `--progress`. pub fn matches(self, command: &str) -> bool { let suffix = format!("/{}/{SCRIPT_FILE}", self.folder()); - let mut words = command.split_whitespace(); - words.any(|w| w.ends_with(&suffix)) && command.split_whitespace().any(|w| w == "--progress") + command.split_whitespace().any(|w| w.ends_with(&suffix)) + && command.split_whitespace().any(|w| w == "--progress") } /// A process of this run, if it is running. @@ -269,11 +269,12 @@ fn download_installer(script: &Path) -> Result<(), String> { if out.code == Some(0) && script.is_file() { return Ok(()); } + const FAILED: &str = "Could not download the installer. Check the internet connection, then try again."; let said = out.stderr.trim(); Err(if said.is_empty() { - "Could not download the installer. Check the internet connection, then try again.".to_string() + FAILED.to_string() } else { - format!("Could not download the installer. Check the internet connection, then try again.\n{said}") + format!("{FAILED}\n{said}") }) } @@ -571,9 +572,8 @@ pub fn follow(run: Run, stale: impl Fn() -> bool, send: impl Fn(Value)) { return; } let running = run.process().is_some(); - let log = progress.feed(&read_more(&mut file, &path, &mut offset)); + let mut log = progress.feed(&read_more(&mut file, &path, &mut offset)); if !running || progress.done { - let mut log = log; log.extend(progress.finish()); let cancelled = CANCELLED.load(Ordering::SeqCst); if progress.done { diff --git a/packages/desktop/src/local_server.rs b/packages/desktop/src/local_server.rs index b55d0271..5e629d54 100644 --- a/packages/desktop/src/local_server.rs +++ b/packages/desktop/src/local_server.rs @@ -1,4 +1,7 @@ //! "Start Codeman on this Mac": `codeman web -d` through the user's login shell. +//! The same login-shell machinery answers whether Codeman is installed +//! (`installed`) and hands its environment to the graphical install +//! (`login_environment`); `exec_bounded` is the crate's bounded process runner. //! //! A double-clicked app gets launchd's bare PATH (`/usr/bin:/bin:...`), which //! finds neither `codeman` nor `node`, and the server inherits whatever @@ -143,7 +146,8 @@ fn pump(pipe: Option, stream: usize, tx: mpsc::Send }); } -/// `exec_bounded` for a start. +/// `exec_bounded` with the start's timeout message. The installed check and +/// `login_environment` run through it too, and drop its error. fn exec(cmd: Command, deadline: Instant) -> Result { exec_bounded( cmd, @@ -248,13 +252,16 @@ fn parse_env(output: &str) -> Option> { None } +/// A portable variable name: a letter or `_`, then letters, digits or `_`. +pub fn is_env_name(name: &str) -> bool { + let mut chars = name.chars(); + chars.next().is_some_and(|c| c.is_ascii_alphabetic() || c == '_') + && chars.all(|c| c.is_ascii_alphanumeric() || c == '_') +} + fn env_entry(line: &str) -> Option<(&str, &str)> { let (name, value) = line.split_once('=')?; - let mut chars = name.chars(); - let first = chars.next()?; - let portable = - (first.is_ascii_alphabetic() || first == '_') && chars.all(|c| c.is_ascii_alphanumeric() || c == '_'); - portable.then_some((name, value)) + is_env_name(name).then_some((name, value)) } /// Ask `shell` for its environment with `flags`. The outer error ends the @@ -339,16 +346,13 @@ fn run_in_shell( /// 127 means "no codeman here" and moves on to the next way of asking; the /// interactive profile is asked only when `interactive` allows it. fn launch(script: &str, shell: &Path, args: &[String], deadline: Instant, interactive: bool) -> Result { - let attempts = match flavor(shell) { - Flavor::Posix => { - let result = run_in_shell(script, shell, false, args, deadline)?; - if result.code != Some(127) || !interactive { - return Ok(result); - } - // Not on the login PATH; try the interactive profile too. - return run_in_shell(script, shell, true, args, deadline); + let Flavor::Capture(attempts) = flavor(shell) else { + let result = run_in_shell(script, shell, false, args, deadline)?; + if result.code != Some(127) || !interactive { + return Ok(result); } - Flavor::Capture(attempts) => attempts, + // Not on the login PATH; try the interactive profile too. + return run_in_shell(script, shell, true, args, deadline); }; let mut last_run = None; let mut capture_error = None; @@ -386,8 +390,8 @@ const FIND_SCRIPT: &str = r#"if command -v codeman >/dev/null 2>&1; then exit 0; if [ -f "$HOME/.codeman/app/dist/index.js" ] && command -v node >/dev/null 2>&1; then exit 0; fi exit 127"#; -/// How long the This Mac tab waits for the login shell to answer whether -/// Codeman is installed. +/// How long one look at the login shell may take: whether Codeman is +/// installed (the This Mac tab), or its environment for the graphical install. const FIND_TIMEOUT: Duration = Duration::from_secs(15); /// `installed` with the shell given, so tests need not touch `$SHELL`. @@ -453,7 +457,7 @@ mod tests { }) .output }; - // stdout that ended without a newline used to be glued to stderr. + // stdout that ends without a newline: stderr still starts a line of its own. assert_eq!( said("Starting Codeman", "Error: port 3000 is in use\n"), "Starting Codeman\nError: port 3000 is in use" diff --git a/packages/desktop/src/probe.rs b/packages/desktop/src/probe.rs index 12119f86..48ac7503 100644 --- a/packages/desktop/src/probe.rs +++ b/packages/desktop/src/probe.rs @@ -2,8 +2,10 @@ //! //! One `GET /api/status`, five seconds at most, run off the UI thread. //! The answer is classified into something the connect page can explain. It is -//! asked once per user action, never in a loop: Codeman counts a request -//! without valid credentials against the login rate limit (10 per IP). +//! asked on a user action, or by one of the This Mac tab's short waits (for an +//! install to bring Codeman up, for a server that is starting) that end at the +//! first answer, never as a standing poll: Codeman counts a request without +//! valid credentials against the login rate limit (10 per IP). use serde::Serialize; use std::time::Duration; @@ -82,6 +84,16 @@ pub enum ProbeOutcome { }, } +impl ProbeOutcome { + /// The server's version, when it answered as Codeman. + pub fn version(&self) -> Option { + match self { + ProbeOutcome::Ok { version } => Some(version.clone()), + _ => None, + } + } +} + /// Classify an HTTP answer to `/api/status`. `request` is the URL that /// was asked, `location` and `retry_after` are those response headers. pub fn classify_response( @@ -94,12 +106,10 @@ pub fn classify_response( ) -> ProbeOutcome { match status { 200 => { - let parsed: Option = serde_json::from_str(body).ok(); - let version = parsed.as_ref().and_then(|v| { - (v.get("success")? == &serde_json::Value::Bool(true)) - .then(|| v.get("data")?.get("version")?.as_str().map(str::to_string)) - .flatten() - }); + let version = serde_json::from_str::(body) + .ok() + .filter(|v| v["success"] == true) + .and_then(|v| v["data"]["version"].as_str().map(str::to_string)); match version { Some(version) => ProbeOutcome::Ok { version }, None => ProbeOutcome::NotCodeman { status }, @@ -742,8 +752,8 @@ mod tests { } /// A Codeman that sends its status headers and then stalls (a blocked - /// event loop, a lossy link) did not answer in time. It is not "something - /// else on this port", which is what an empty body read as before. + /// event loop, a lossy link) did not answer in time: a Timeout, not + /// NotCodeman ("something else on this port"). #[test] fn a_body_that_stalls_is_a_timeout() { use std::io::{Read, Write}; diff --git a/packages/desktop/src/setup.rs b/packages/desktop/src/setup.rs index 75e83593..68514f39 100644 --- a/packages/desktop/src/setup.rs +++ b/packages/desktop/src/setup.rs @@ -182,10 +182,7 @@ pub fn parse_env_file(text: &str) -> HashMap { let Some((name, value)) = line.split_once('=') else { continue; }; - let mut chars = name.chars(); - let portable = chars.next().is_some_and(|c| c.is_ascii_alphabetic() || c == '_') - && chars.all(|c| c.is_ascii_alphanumeric() || c == '_'); - if !portable { + if !local_server::is_env_name(name) { continue; } let mut value = value.trim(); @@ -335,12 +332,17 @@ pub fn is_codeman_server(command: &str) -> bool { server_args(command).is_some() } +/// The last path component of a command-line word. +fn basename(word: &str) -> &str { + word.rsplit('/').next().unwrap_or(word) +} + /// What follows `web` on a Codeman server's command line, or None for any /// other command. fn server_args(command: &str) -> Option> { let words: Vec<&str> = command.split_whitespace().collect(); let is_entry = |w: &str| { - let name = w.rsplit('/').next().unwrap_or(w); + let name = basename(w); w.ends_with("dist/index.js") || w.ends_with("src/index.ts") || name == "codeman" || name == "aicodeman" }; // Any entry word that `web` follows: an earlier word may only look like @@ -359,14 +361,14 @@ fn server_args(command: &str) -> Option> { /// the port it asked for once the installer runs. pub fn install_in_progress(command: &str) -> Option> { let words: Vec<&str> = command.split_whitespace().collect(); - let name = |w: &&str| w.rsplit('/').next().unwrap_or(w).to_string(); + let script = format!("{LOCAL_SCRIPT_NAME}-"); if words.iter().any(|w| { - let n = name(w); - n.starts_with(&format!("{LOCAL_SCRIPT_NAME}-")) && n.ends_with(".command") + let n = basename(w); + n.starts_with(&script) && n.ends_with(".command") }) { return Some(None); } - if words.iter().any(|w| name(w).starts_with("codeman-install.")) || gui_install::Run::Codeman.matches(command) { + if words.iter().any(|w| basename(w).starts_with("codeman-install.")) || gui_install::Run::Codeman.matches(command) { let port = words .iter() .position(|w| *w == "--port") @@ -539,10 +541,7 @@ fn found_server(url: &Url, answer: &ProbeOutcome, default: &Url) -> FoundServer }; FoundServer { url: url.to_string(), - version: match answer { - ProbeOutcome::Ok { version } => Some(version.clone()), - _ => None, - }, + version: answer.version(), state, is_default: url == default, } @@ -702,11 +701,10 @@ pub fn inspect(depth: Depth) -> LocalStatus { }; let answer = probe::probe(&default, auth.as_deref()); - // Every address one of this user's Codeman servers listens at. The one at - // the default address has just answered; the others are asked now, once - // each. With nothing at the default address, the first of them that - // answers is what the tab opens, as it always was, so each gets the - // probe's usual time; otherwise they are only listed, and get less. + // The other addresses this user's Codeman servers listen at, asked once + // each. With nothing at the default address, the first that answers is + // what the tab opens (`shown`), so each gets the probe's usual time; + // otherwise they are only listed, and get less. let patience = if answer == ProbeOutcome::Refused { probe::TIMEOUT } else { @@ -742,10 +740,7 @@ pub fn inspect(depth: Depth) -> LocalStatus { Depth::Interactive => local_server::installed(true), }; let state = local_state(&outcome, !servers.is_empty(), !listening.is_empty(), installed); - let version = match &outcome { - ProbeOutcome::Ok { version } => Some(version.clone()), - _ => None, - }; + let version = outcome.version(); LocalStatus { url: url.to_string(), state,