diff --git a/desktop/src-tauri/src/engine.rs b/desktop/src-tauri/src/engine.rs index 7912adbfd..9e45d008f 100644 --- a/desktop/src-tauri/src/engine.rs +++ b/desktop/src-tauri/src/engine.rs @@ -380,11 +380,13 @@ fn where_installers_put(engine: Engine) -> Option { let places: Vec = match engine { #[cfg(target_os = "windows")] Engine::Podman => { - // The per-user MSI first: it is the one OpenBot runs. A machine-wide install left by - // somebody else is still found by the second. + // The MSI uses Programs\Podman per user and Program Files\Podman per machine. + // Keep the legacy RedHat location for installations from the older EXE installer. [ std::env::var_os("LOCALAPPDATA") .map(|local| PathBuf::from(local).join("Programs\\Podman\\podman.exe")), + std::env::var_os("ProgramFiles") + .map(|files| PathBuf::from(files).join("Podman\\podman.exe")), std::env::var_os("ProgramFiles") .map(|files| PathBuf::from(files).join("RedHat\\Podman\\podman.exe")), ] @@ -613,6 +615,48 @@ mod tests { assert_eq!(on_path("openbot-not-a-real-binary"), None); } + #[test] + #[cfg(windows)] + fn windows_installed_podman_runs_without_an_inherited_path_entry() { + if crate::test_support::isolated_process( + "engine::tests::windows_installed_podman_runs_without_an_inherited_path_entry", + ) { + return; + } + let root = crate::test_support::temp_root("podman install locations"); + std::fs::create_dir_all(&root).unwrap(); + let source = root.join("engine.rs"); + let binary = root.join("fixture.exe"); + std::fs::write(&source, "fn main() { println!(\"1.44\"); }").unwrap(); + crate::test_support::compile_fixture(&source, &binary); + let local = root.join("Local"); + let program_files = root.join("Program Files"); + std::env::set_var("LOCALAPPDATA", &local); + std::env::set_var("ProgramFiles", &program_files); + std::env::set_var("PATH", root.join("empty-path")); + + for installation in [ + local.join("Programs/Podman/podman.exe"), + program_files.join("Podman/podman.exe"), + program_files.join("RedHat/Podman/podman.exe"), + ] { + std::fs::create_dir_all(installation.parent().unwrap()).unwrap(); + std::fs::copy(&binary, &installation).unwrap(); + assert_eq!(program(Engine::Podman).as_ref(), Some(&installation)); + let address = Address::new(Engine::Podman, Some("openbot".into())); + assert_eq!(address.parts().0, installation); + assert!(address.responds(), "resolved engine should actually run"); + assert!(tool(Engine::Podman) + .arg("--version") + .output() + .unwrap() + .status + .success()); + std::fs::remove_file(installation).unwrap(); + } + std::fs::remove_dir_all(root).unwrap(); + } + /// The provider directory has to be in *front* of PATH: a broken `docker-compose` earlier on /// somebody's PATH would otherwise be the one Podman runs. #[test] diff --git a/desktop/src-tauri/src/install.rs b/desktop/src-tauri/src/install.rs index 7e1f3336c..9a138627f 100644 --- a/desktop/src-tauri/src/install.rs +++ b/desktop/src-tauri/src/install.rs @@ -363,13 +363,25 @@ pub fn install_engine(cache: &Path) -> Result { /// Observe an actual Podman installer invocation, excluding existing engines and Compose repair. pub fn install_engine_observed( + cache: &Path, + installed: impl FnMut(bool), +) -> Result { + install_engine_with(cache, installed, install_podman, place_compose) +} + +fn install_engine_with( cache: &Path, mut installed: impl FnMut(bool), + install_podman: impl FnOnce(&Path) -> Result<(), Problem>, + place_compose: impl FnOnce(&Path) -> Result, ) -> Result { let into = crate::acquire::download_dir(cache); - // An engine somebody already has is theirs. This only ever adds what is missing. - if engine::program(Engine::Docker).is_some() || engine::program(Engine::Podman).is_some() { + // engine_ready can start an installed Podman machine, but cannot start Docker. A stopped + // Docker CLI must not skip installing the Podman that preparation will then try to run. + if engine::program(Engine::Podman).is_some() + || engine::Address::new(Engine::Docker, None).responds() + { return place_compose(&into); } @@ -642,6 +654,67 @@ mod tests { use super::*; use crate::test_support::temp_root; + #[test] + #[cfg(windows)] + fn a_stopped_docker_does_not_skip_the_podman_needed_for_setup() { + if crate::test_support::isolated_process( + "install::tests::a_stopped_docker_does_not_skip_the_podman_needed_for_setup", + ) { + return; + } + let root = temp_root("stopped-docker-install"); + let bin = root.join("bin"); + std::fs::create_dir_all(&bin).unwrap(); + let source = root.join("engine.rs"); + std::fs::write( + &source, + r#"fn main() { + if std::env::var_os("OPENBOT_TEST_DOCKER_RUNNING").is_none() { + std::process::exit(1); + } + println!("1.44"); + }"#, + ) + .unwrap(); + crate::test_support::compile_fixture(&source, &bin.join("docker.exe")); + std::env::set_var("PATH", &bin); + std::env::set_var("LOCALAPPDATA", root.join("Local")); + std::env::set_var("ProgramFiles", root.join("Program Files")); + std::env::remove_var("OPENBOT_TEST_DOCKER_RUNNING"); + + let found = engine::detect(); + assert_eq!(found.engine, Some(Engine::Docker)); + assert!(!found.responding); + assert!(engine::program(Engine::Podman).is_none()); + let mut observed = Vec::new(); + let result = install_engine_with( + &root, + |success| observed.push(success), + |_| { + // Installation supplies the engine that engine_ready will create/start. + std::fs::copy(bin.join("docker.exe"), bin.join("podman.exe")).unwrap(); + Ok(()) + }, + |_| Ok("Compose placed".into()), + ); + result.expect("setup should install its missing Podman"); + assert_eq!(observed, [true], "the Podman installer must run"); + assert!(engine::program(Engine::Podman).is_some()); + + // A responding Docker still supplies the engine; never install a replacement. + std::fs::remove_file(bin.join("podman.exe")).unwrap(); + std::env::set_var("OPENBOT_TEST_DOCKER_RUNNING", "1"); + assert!(engine::detect().responding); + let result = install_engine_with( + &root, + |_| panic!("no Podman installation should be observed"), + |_| panic!("responding Docker must be reused"), + |_| Ok("Compose placed".into()), + ); + assert_eq!(result.unwrap(), "Compose placed"); + std::fs::remove_dir_all(root).unwrap(); + } + /// Every platform this app runs on has a Compose build, or the stack cannot be raised there. #[test] fn this_platform_has_a_compose_build() { diff --git a/desktop/src-tauri/src/plan.rs b/desktop/src-tauri/src/plan.rs index 2fef5cf60..3d6c2321d 100644 --- a/desktop/src-tauri/src/plan.rs +++ b/desktop/src-tauri/src/plan.rs @@ -187,10 +187,22 @@ breaks the first time a hint or a colour is added, and breaking here means telli approved in their browser that it failed. */ pub fn token_in(output: &str) -> Option { - plain(output) - .split(|c: char| c.is_whitespace() || c == '"' || c == '\'') - .map(|word| word.trim_matches(|c: char| !c.is_ascii_alphanumeric() && c != '-' && c != '_')) - .find(|word| word.starts_with(PLAN_TOKEN_PREFIX) && word.len() > 30) + fn token_character(c: char) -> bool { + c.is_ascii_alphanumeric() || c == '-' || c == '_' + } + + let text = plain(output); + // Cursor positioning can separate a label from its token visually without a space + // in the stream. Accept the prefix after punctuation, but not inside another word. + text.match_indices(PLAN_TOKEN_PREFIX) + .filter(|(at, _)| !matches!(text[..*at].chars().next_back(), Some(c) if token_character(c))) + .map(|(at, _)| { + text[at..] + .split(|c| !token_character(c)) + .next() + .unwrap_or("") + }) + .find(|token| token.len() > 30) .map(str::to_string) } @@ -202,6 +214,12 @@ pub struct SigningIn { child: Box, writer: Box, output: std::sync::Arc>, + // ConPTY's pipe clones do not own its console. Dropping the last master closes the + // console and its child, so retain it through the code/token exchange on Windows. + #[cfg(windows)] + _master: Box, + #[cfg(windows)] + cursor_reported: bool, } /// How long to wait for the CLI to show the URL. Machine time: a container start and an HTTP call. @@ -298,6 +316,10 @@ impl SigningIn { child, writer, output, + #[cfg(windows)] + _master: pty.master, + #[cfg(windows)] + cursor_reported: false, }; let url = signing .wait_for(authorize_url_in, PATIENCE_FOR_THE_LINK) @@ -387,6 +409,16 @@ impl SigningIn { let began = Instant::now(); while began.elapsed() < patience { if let Ok(seen) = self.output.lock() { + // portable-pty enables ConPTY's INHERIT_CURSOR flag. It waits for this + // reply before emitting the child's output, and can hang on close without it. + // This hidden terminal starts at 1;1. Accumulating output also handles a + // query split across reads; reply once to the initial inheritance request. + #[cfg(windows)] + if !self.cursor_reported && seen.contains("\x1b[6n") { + self.writer.write_all(b"\x1b[1;1R").ok()?; + self.writer.flush().ok()?; + self.cursor_reported = true; + } if let Some(value) = found(&seen) { return Some(value); } @@ -451,6 +483,27 @@ const CHATGPT_STORE: &str = "/root/.langchain/chatgpt-auth.json"; const CHATGPT_LOOPBACK: u16 = 1455; const CHATGPT_RELAY: u16 = 1456; +fn publish_chatgpt_callback( + command: &mut std::process::Command, + engine: crate::engine::Engine, + windows: bool, +) { + // Windows Podman's IPv6 forward accepts TCP but closes the callback without an HTTP + // response. That prevents localhost clients from trying the working IPv4 address: + // Happy Eyeballs stops at the first successful TCP handshake (RFC 8305, sections 5/9.2). + // Publish only IPv4 there so localhost falls back after IPv6 connection refusal. + // Keep the registered localhost redirect URI and the other runtimes' bindings unchanged. + let hosts: &[&str] = if windows && engine == crate::engine::Engine::Podman { + &["127.0.0.1"] + } else { + &["127.0.0.1", "[::1]"] + }; + for host in hosts { + command.arg("-p"); + command.arg(format!("{host}:{CHATGPT_LOOPBACK}:{CHATGPT_RELAY}")); + } +} + /** The ChatGPT sign-in, as a program handed to the harness image. @@ -567,14 +620,10 @@ impl SigningInToChatGpt { * Published on loopback only, and on the number the vendor's login advertises. * * The container's relay listens on `CHATGPT_RELAY` and forwards to the login's own - * loopback bind; the browser is sent to `CHATGPT_LOOPBACK` on this machine. Both families - * are published because a browser resolving the registered `localhost` may pick either, and - * which one it picks is not ours to decide. + * loopback bind; the browser is sent to `CHATGPT_LOOPBACK` on this machine. + * publish_chatgpt_callback handles the Windows Podman IPv6 forwarding limitation. */ - for host in ["127.0.0.1", "[::1]"] { - command.arg("-p"); - command.arg(format!("{host}:{CHATGPT_LOOPBACK}:{CHATGPT_RELAY}")); - } + publish_chatgpt_callback(&mut command, engine.engine, cfg!(windows)); command.arg(image); command.arg("python"); command.arg("-u"); @@ -721,6 +770,101 @@ pub fn openai_url_in(output: &str) -> Option { #[cfg(test)] mod tests { use super::*; + use crate::engine::Engine; + + #[test] + #[cfg(windows)] + fn windows_claude_sign_in_keeps_its_terminal_until_the_flow_finishes() { + if crate::test_support::isolated_process( + "plan::tests::windows_claude_sign_in_keeps_its_terminal_until_the_flow_finishes", + ) { + return; + } + let root = crate::test_support::temp_root("claude-terminal-lifetime"); + std::fs::create_dir_all(&root).unwrap(); + let source = root.join("podman.rs"); + std::fs::write( + &source, + r#"use std::io::Write; + fn main() { + print!("\x1b]8;;https://claude.ai/oauth/authorize?synthetic=terminal-lifetime\x1b\\Sign in\x1b]8;;\x1b\\\r\n"); + println!("Paste code here if prompted"); + std::io::stdout().flush().unwrap(); + let mut input = String::new(); + std::io::stdin().read_line(&mut input).unwrap(); + assert_eq!(input.trim(), format!("{}#{}", "c".repeat(43), "s".repeat(48))); + print!("Your OAuth token (valid for 1 year):"); + std::io::stdout().flush().unwrap(); + std::thread::sleep(std::time::Duration::from_millis(60)); + println!("\x1b[40G\x1b[32msk-ant-oat01-{}\x1b[0m", "s".repeat(95)); + }"#, + ) + .unwrap(); + crate::test_support::compile_fixture(&source, &root.join("podman.exe")); + std::env::set_var("PATH", &root); + + // Bound begin and cleanup, including any destructor run before either returns. + // The fixture prints a synthetic URL and waits; no provider or container is contacted. + let (sent, received) = std::sync::mpsc::channel(); + let worker = std::thread::spawn(move || { + let result = (|| { + let (mut signing, url) = SigningIn::begin( + &crate::engine::Address::new(Engine::Podman, None), + "synthetic-sign-in-image", + )?; + assert_eq!( + url, + "https://claude.ai/oauth/authorize?synthetic=terminal-lifetime" + ); + assert!( + signing.child.try_wait().unwrap().is_none(), + "the login child must survive until the code can be supplied" + ); + let draining = std::sync::Arc::downgrade(&signing.output); + let code = format!("{}#{}", "c".repeat(43), "s".repeat(48)); + assert_eq!( + signing.finish(&code)?, + format!("sk-ant-oat01-{}", "s".repeat(95)) + ); + // Modern ClosePseudoConsole returns before its clients disconnect. The + // reader's EOF, not the master's drop, marks completed console cleanup. + // Keep this inside the deadline before deleting the fixture executable. + while draining.strong_count() != 0 { + std::thread::sleep(Duration::from_millis(10)); + } + Ok::<_, String>(()) + })(); + let _ = sent.send(result); + }); + received + .recv_timeout(Duration::from_secs(10)) + .expect("begin and cleanup must finish without a terminal teardown deadlock") + .expect("the synthetic login should provide a URL"); + worker.join().unwrap(); + std::fs::remove_dir_all(root).unwrap(); + } + + #[test] + fn chatgpt_callback_uses_ipv4_only_for_windows_podman() { + for (engine, windows, hosts) in [ + (Engine::Podman, true, vec!["127.0.0.1"]), + (Engine::Podman, false, vec!["127.0.0.1", "[::1]"]), + (Engine::Docker, true, vec!["127.0.0.1", "[::1]"]), + (Engine::Docker, false, vec!["127.0.0.1", "[::1]"]), + ] { + let mut command = crate::quiet::command(engine.binary()); + publish_chatgpt_callback(&mut command, engine, windows); + let args: Vec<_> = command + .get_args() + .map(|arg| arg.to_str().unwrap()) + .collect(); + let expected: Vec = hosts + .into_iter() + .flat_map(|host| ["-p".into(), format!("{host}:1455:1456")]) + .collect(); + assert_eq!(args, expected, "{engine:?}, Windows={windows}"); + } + } /// Fixtures are composed from the prefix rather than written out, so no credential-shaped /// literal sits in this repository for a scanner to find or a person to copy. @@ -911,6 +1055,16 @@ mod tests { assert_eq!(authorize_url_in(""), None); } + #[test] + fn token_after_a_cursor_positioned_label_is_found_in_full() { + let token = format!("{PLAN_TOKEN_PREFIX}01-{}", "s".repeat(95)); + let output = format!( + "Your OAuth token (valid for 1 year):\x1b[1G\x1b[32m{token}\x1b[0m\nStore this token safely." + ); + assert_eq!(token_in(&output), Some(token.clone())); + assert_eq!(token_in(&format!("other-{token}")), None); + } + /// The stripper has to survive what a TUI actually emits, including a bare ESC pair. #[test] fn escapes_come_out_and_the_words_stay() { diff --git a/desktop/src/ProviderPicker.tsx b/desktop/src/ProviderPicker.tsx index 32a211b83..8ca636bfd 100644 --- a/desktop/src/ProviderPicker.tsx +++ b/desktop/src/ProviderPicker.tsx @@ -485,23 +485,25 @@ export function ProviderPicker({ Opens {row.name} in your browser. Nothing is typed here and no key is stored.

- - {!busy && - (row.id === "openai" || row.id === "anthropic") && - held.saved?.modelSessions?.[row.id] !== false && ( - - )} +
+ + {!busy && + (row.id === "openai" || row.id === "anthropic") && + held.saved?.modelSessions?.[row.id] !== false && ( + + )} +
{busy && progress && (

{progress} diff --git a/desktop/src/styles.css b/desktop/src/styles.css index 8f19393b9..ed57eb1c7 100644 --- a/desktop/src/styles.css +++ b/desktop/src/styles.css @@ -400,6 +400,12 @@ details > summary:hover { margin-top: 0.9rem; } +.provider-sign-in-actions { + display: flex; + flex-wrap: wrap; + gap: 0.6rem; +} + .segmented { display: inline-flex; background: var(--sunk);