diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/MainActivity.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/MainActivity.kt index c577548..8f9bcef 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/MainActivity.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/MainActivity.kt @@ -19,6 +19,7 @@ import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableIntStateOf import androidx.compose.runtime.setValue import androidx.compose.ui.Modifier +import androidx.compose.ui.graphics.luminance import androidx.core.view.WindowCompat class MainActivity : ComponentActivity() { @@ -38,8 +39,13 @@ class MainActivity : ComponentActivity() { // through underneath it and content insets itself. Same reasoning // as dev-updater's MainActivity. enableEdgeToEdge() + // Dark status-bar icons only over a light background, decided from the scheme rather + // than fixed. It was hardcoded to `true` -- dark icons -- which was right against the + // default light surface and became unreadable the moment the app wore Catppuccin Mocha. + // Asking the colour means a future palette change cannot reintroduce that: whatever + // `background` becomes, the icons follow it. WindowCompat.getInsetsController(window, window.decorView).isAppearanceLightStatusBars = - true + AiAppColors.background.luminance() > 0.5f // Android 17+ silently drops local-network traffic without this; // requested up front because a denial is invisible at the socket diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/SetupsScreen.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/SetupsScreen.kt index b569627..7aad7d8 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/SetupsScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/SetupsScreen.kt @@ -237,7 +237,6 @@ private fun AddSetupDialog( val scope = rememberCoroutineScope() var name by remember { mutableStateOf("") } var address by remember { mutableStateOf("") } - var port by remember { mutableStateOf("") } var identity by remember { mutableStateOf("") } var tested by remember { mutableStateOf(null) } var testing by remember { mutableStateOf(false) } @@ -246,10 +245,11 @@ private fun AddSetupDialog( address .trim() .takeIf { it.isNotEmpty() } - ?.let { + ?.let { typed -> + val (host, typedPort) = splitHostAndPort(typed) SshDetails( - address = it, - port = port.trim().toIntOrNull(), + address = host, + port = typedPort, identityFile = identity.trim().ifEmpty { null }, ) } @@ -275,13 +275,10 @@ private fun AddSetupDialog( OutlinedTextField( value = address, onValueChange = { address = it }, - label = { Text("user@host (blank = this machine)") }, - singleLine = true, - ) - OutlinedTextField( - value = port, - onValueChange = { port = it }, - label = { Text("Port (blank = 22)") }, + // Just the shape. What a blank one means is said once, in the text above + // this form -- repeating it here wrapped the label onto a second line and + // made this field taller than the two beside it for no information. + label = { Text("user@host[:port]") }, singleLine = true, ) OutlinedTextField( @@ -367,3 +364,33 @@ private fun RenameDialog(setup: Setup, onDismiss: () -> Unit, onRename: (String) dismissButton = { TextButton(onClick = onDismiss) { Text("Cancel") } }, ) } + +/** + * Splits `user@host:port` into its two halves, with the port left null when none was typed. + * + * One field rather than two because that is how an address is written and read everywhere else -- + * and because a port that is almost always 22 does not deserve a box of its own on a phone + * keyboard. Null rather than 22: the backend already decides the default, and writing 22 here would + * put a second answer to that question in a second place. + * + * A colon only means "port" when it can. A bracketed IPv6 literal is unwrapped as ssh writes it, + * `[::1]:22`; a bare `::1` keeps every colon, because an address with several is an address, not an + * address and a port. So the rule is: brackets, or exactly one colon followed by digits. + */ +private fun splitHostAndPort(typed: String): Pair { + if (typed.startsWith("[")) { + val close = typed.indexOf(']') + if (close > 0) { + val host = typed.substring(1, close) + val rest = typed.substring(close + 1) + val port = rest.removePrefix(":").toIntOrNull().takeIf { rest.startsWith(":") } + return host to port + } + } + if (typed.count { it == ':' } == 1) { + val host = typed.substringBeforeLast(':') + val port = typed.substringAfterLast(':').toIntOrNull() + if (port != null && host.isNotEmpty()) return host to port + } + return typed to null +} diff --git a/server/src/session/claude.rs b/server/src/session/claude.rs index 79c8268..1b57a79 100644 --- a/server/src/session/claude.rs +++ b/server/src/session/claude.rs @@ -34,6 +34,7 @@ //! - `control_request{subtype:set_model}` answers success; //! `{subtype:interrupt}` stops the turn. +use std::collections::VecDeque; use std::path::{Path, PathBuf}; use std::sync::{Arc, Mutex}; @@ -47,6 +48,34 @@ use super::transport::{Launch, Transport}; use crate::config::{ProviderConfig, SessionConfig}; use translate::{AnswerOutcome, Translator}; +/// How much of a failing process's stderr the exit report carries. +/// +/// Enough for a shell's complaint plus the context it prints around it -- +/// fish's `cd` failure is seven lines including a caret pointing at the +/// offending line -- and bounded because this is held per session for the +/// life of the process and a chatty program would otherwise grow without +/// limit. +const STDERR_LINES_KEPT: usize = 50; + +/// The kept stderr as one block, with blank lines trimmed off both ends. +/// +/// The trailing trim is the point: a shell's error ends with a blank line, +/// so the last line of stderr is routinely empty and anything that reports +/// "the last line" reports nothing at all. +fn tail_of(kept: &VecDeque) -> String { + let lines: Vec<&str> = kept.iter().map(String::as_str).collect(); + let start = lines + .iter() + .position(|line| !line.trim().is_empty()) + .unwrap_or(lines.len()); + let end = lines + .iter() + .rposition(|line| !line.trim().is_empty()) + .map(|last| last + 1) + .unwrap_or(start); + lines[start..end].join("\n") +} + /// Where the driver remembers its CLI session id between backend runs -- /// the whole crash-recovery story: respawning with `--resume ` picks /// the conversation back up from Claude's own session files. Kept in the @@ -138,19 +167,31 @@ impl ClaudeDriver { )); // stderr is diagnostics only; surface it in the log, and keep the - // last line for the exit report below. For a remote provider this + // tail of it for the exit report below. For a remote provider this // is also where ssh's own failures arrive ("Permission denied", // "Could not resolve hostname"), which are the ones a person // actually needs to see. - let last_stderr = Arc::new(Mutex::new(String::new())); + // + // A ring of the last lines rather than the last line alone. Keeping + // one line meant keeping whatever happened to come last, and what + // comes last is very often blank -- a shell's error message ends + // with one -- so the report was a bare exit status and the actual + // complaint existed only in the server's log, which is not where + // the person holding the phone is looking. A failing `cd` cost an + // evening to exactly that. + let recent_stderr = Arc::new(Mutex::new(VecDeque::::new())); { - let last_stderr = Arc::clone(&last_stderr); + let recent_stderr = Arc::clone(&recent_stderr); let label = provider.name.clone(); tokio::spawn(async move { let mut lines = BufReader::new(stderr).lines(); while let Ok(Some(line)) = lines.next_line().await { tracing::warn!("{label} stderr: {line}"); - *last_stderr.lock().unwrap() = line; + let mut kept = recent_stderr.lock().unwrap(); + kept.push_back(line); + while kept.len() > STDERR_LINES_KEPT { + kept.pop_front(); + } } }); } @@ -172,16 +213,13 @@ impl ClaudeDriver { if let Some(status) = status && !status.success() { - let detail = last_stderr.lock().unwrap().clone(); + let detail = tail_of(&recent_stderr.lock().unwrap()); let _ = sink.send(Event::Error { - message: format!( - "{label} exited with {status}{}", - if detail.is_empty() { - String::new() - } else { - format!(": {detail}") - } - ), + message: if detail.is_empty() { + format!("{label} exited with {status}") + } else { + format!("{label} exited with {status}:\n{detail}") + }, }); } let _ = sink.send(Event::Status { @@ -365,3 +403,47 @@ fn attachment_block(session_dir: &Path, id: &str) -> Result { } })) } + +#[cfg(test)] +mod tests { + use super::*; + + /// The failure this exists for: a shell's complaint ends with a blank + /// line, so reporting "the last line of stderr" reported nothing, and + /// the phone showed a bare exit status while the reason sat in the + /// server's log. + #[test] + fn the_report_keeps_the_message_and_not_the_blank_line_after_it() { + let fish_cd_failure = [ + "cd: The directory '~/repos/ai-app' does not exist", + "", + "embedded:functions/cd.fish (line 26): ", + " builtin cd $argv", + " ^", + "in function 'cd' with arguments '~/repos/ai-app'", + "", + ]; + let kept: VecDeque = fish_cd_failure.iter().map(|l| l.to_string()).collect(); + + let report = tail_of(&kept); + assert!( + report.starts_with("cd: The directory"), + "the complaint leads: {report}", + ); + assert!( + report.ends_with("'~/repos/ai-app'"), + "the trailing blank is trimmed: {report:?}", + ); + // The blank *between* lines is part of the message and stays. + assert!(report.contains("does not exist\n\nembedded:"), "{report:?}"); + } + + /// Nothing to say is said as nothing, so the caller can tell the two + /// apart and print just the exit status. + #[test] + fn stderr_that_is_only_blank_lines_reports_as_empty() { + let kept: VecDeque = ["", " ", ""].iter().map(|l| l.to_string()).collect(); + assert_eq!(tail_of(&kept), ""); + assert_eq!(tail_of(&VecDeque::new()), ""); + } +} diff --git a/server/src/ssh.rs b/server/src/ssh.rs index d196277..60ebaf1 100644 --- a/server/src/ssh.rs +++ b/server/src/ssh.rs @@ -88,7 +88,7 @@ fn remote_script(program: &str, args: &[String], cwd: Option<&Path>) -> String { let mut script = String::new(); if let Some(cwd) = cwd { script.push_str("cd "); - script.push_str("e(&cwd.to_string_lossy())); + script.push_str("e_path(&cwd.to_string_lossy())); script.push_str(" && "); } script.push_str("exec "); @@ -100,6 +100,36 @@ fn remote_script(program: &str, args: &[String], cwd: Option<&Path>) -> String { script } +/// Quotes a path, expanding a leading `~` and nothing else. +/// +/// [`quote`] is right for every other word crossing to the remote side and +/// wrong for exactly one character. `~` means "expand me", and single +/// quotes are what stop expansion -- so a working directory typed as +/// `~/repos/ai-app` arrived as the literal four-character directory `~`, +/// and the remote shell said it did not exist. Which is true, and reads +/// like the path being wrong rather than the quoting. +/// +/// `"$HOME"` rather than handing the tilde to the shell unquoted: the +/// variable is expanded, the expansion is not re-split or globbed because +/// it is double-quoted, and everything after it stays single-quoted and +/// literal. So the one character that has to mean something keeps meaning +/// it, and nothing else gains a meaning. `$HOME` is set by every shell +/// this can land in, including the fish login shell on the dev VM, which +/// is why this does not depend on the remote shell being POSIX. +/// +/// `~user` is deliberately not handled: there is no portable expansion for +/// it, and inventing one would mean guessing another account's home +/// directory. It stays literal and fails with the shell's own message. +fn quote_path(path: &str) -> String { + if path == "~" { + return "\"$HOME\"".to_string(); + } + match path.strip_prefix("~/") { + Some(rest) => format!("\"$HOME\"/{}", quote(rest)), + None => quote(path), + } +} + /// Single-quotes one word for a POSIX shell. /// /// Everything crossing to the remote side goes through here: paths, model @@ -195,8 +225,39 @@ mod tests { assert!(!rendered.contains(&"IdentitiesOnly=yes".to_string())); } + /// The one character quoting must not swallow. + /// + /// A working directory typed as `~/repos/ai-app` was arriving as the + /// literal directory `~`, and the remote shell reported it missing -- + /// which reads as the path being wrong rather than the quoting being + /// wrong, and cost an evening on exactly that misreading. + #[test] + fn a_leading_tilde_expands_and_nothing_else_does() { + assert_eq!(quote_path("~"), "\"$HOME\""); + assert_eq!(quote_path("~/repos/ai-app"), "\"$HOME\"/'repos/ai-app'"); + // Only leading, and only its own segment: a tilde anywhere else is + // an ordinary character in a filename, and `~user` has no portable + // expansion so it stays literal and fails with the shell's message. + assert_eq!(quote_path("/tmp/~/x"), "'/tmp/~/x'"); + assert_eq!(quote_path("~user/x"), "'~user/x'"); + + // And it reaches the script the remote shell is handed. + assert_eq!( + remote_script("claude", &args(["-p"]), Some(Path::new("~/repos/ai-app"))), + "cd \"$HOME\"/'repos/ai-app' && exec 'claude' '-p'", + ); + } + #[test] fn shell_metacharacters_cross_as_data_not_syntax() { + // Expanding $HOME must not open a door for anything else: the rest + // stays single-quoted, so this remains one absurd path rather than + // three commands. + assert_eq!( + quote_path("~/'; touch /tmp/pwned; '"), + r#""$HOME"/''\''; touch /tmp/pwned; '\'''"#, + ); + assert_eq!(quote("plain"), "'plain'"); assert_eq!(quote("with space"), "'with space'"); assert_eq!(quote("; rm -rf /"), "'; rm -rf /'");