Expand a leading ~, and say what the failing command said
Three things, two of which are the same failure seen from opposite ends. **A working directory of `~/repos/ai-app` never worked.** Everything crossing to the remote side is single-quoted, which is right for paths, model names and prompts alike -- unquoted they would be shell syntax rather than data. It is wrong for exactly one character: `~` means "expand me", and quoting is what stops expansion. So the remote shell was handed the literal four-character directory `~` and correctly said it did not exist, which reads as the path being wrong rather than the quoting. Paths now go through `quote_path`, which emits `"$HOME"` for a leading `~/` and single-quotes the rest. The variable expands, the expansion is not re-split or globbed because it is double-quoted, and nothing after it gains a meaning -- there is a test that pushes a quote-and-semicolon injection through the tilde branch and gets back one absurd path rather than three commands. `$HOME` is set by every shell this can land in, so this does not depend on the remote side being POSIX; verified by running the generated script under both sh and fish, which is what the dev VM actually uses. **The phone could not have told you any of that.** The exit report kept the last line of stderr, and a shell's error message ends with a blank line -- so the last line was empty, the report was a bare exit status, and the seven lines of fish complaining sat in the server's log where nobody holding a phone is looking. It now keeps the last 50 lines in a ring and reports them with blank lines trimmed from both ends. The tests use the real fish `cd` failure as their fixture. **The status bar was unreadable.** `isAppearanceLightStatusBars` was hardcoded to `true` -- dark icons -- which was right against the default light surface and wrong the moment the app wore Mocha. It now asks the scheme's own background for its luminance, so changing the palette cannot reintroduce it. **And the address field takes `user@host:port`.** One field rather than two, because that is how an address is written everywhere else and a port that is nearly always 22 does not deserve its own box on a phone keyboard. Absent means absent rather than 22: the backend already decides that default, and writing it here would be a second answer in a second place. A colon only means "port" when it can -- brackets for IPv6 as ssh writes them, otherwise exactly one colon followed by digits. Looked at on the emulator: the status bar, and the form, whose label I then shortened because it wrapped onto a second line and made that field taller than the two beside it.
This commit is contained in:
1 parent
31135e3f22
commit
2a1bc84c1e
4 files changed
+202
-26
No files matched your search
@@ -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
|
||||
|
||||
@@ -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<String?>(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<String, Int?> {
|
||||
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
|
||||
}
|
||||
@@ -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>) -> 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 <id>` 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::<String>::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<Value> {
|
||||
}
|
||||
}))
|
||||
}
|
||||
|
||||
#[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<String> = 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<String> = ["", " ", ""].iter().map(|l| l.to_string()).collect();
|
||||
assert_eq!(tail_of(&kept), "");
|
||||
assert_eq!(tail_of(&VecDeque::new()), "");
|
||||
}
|
||||
}
|
||||
+62
-1
@@ -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 /'");
|
||||
|
||||
Reference in new issue
Block a user