From 58f74c08e0ba0695f687daedee7574a5863e7351 Mon Sep 17 00:00:00 2001 From: Dusty Date: Tue, 20 Oct 2026 14:50:05 -0700 Subject: [PATCH] fix(desktop): copy agent-computer into the deployment so the server starts Since #673 the server imports its egress rules from agent-computer/src/egress, and scripts/start-local-chrome-computer.ts imports agent-computer/src/browser-runtime. The desktop app lays the deployment out from an allowlist of top-level folders that never included agent-computer, so every new desktop install of v0.2.0 had a server that exited at start with "Cannot find module '../../../agent-computer/src/egress'". - Copy agent-computer with the rest of the deployment. - Add a test that walks the real tree and fails when code the desktop unpacks imports, by relative path, anything it does not unpack. - Record the laid-out folders in the deployment stamp. A stamp written by an earlier app (version only) no longer matches, so Install lays the same release out again, and Start sends such an install back to Install instead of running a server that cannot load. --- CHANGELOG.md | 9 ++ desktop/src-tauri/src/deployment.rs | 157 ++++++++++++++++++++++++++- desktop/src-tauri/src/preparation.rs | 23 ++++ 3 files changed, 187 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ef3f64d9a6..7b1e6a8afc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,15 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. `code_required` (below). A server and a computer must be upgraded together: an older computer has no `/sign-in/code`. +### A new desktop install starts its server again + +A desktop install of v0.2.0 stopped at start with `server stopped straight away (exit status: 1)` and +`Cannot find module '../../../agent-computer/src/egress'`. The server imports its egress rules from +`agent-computer`, and the desktop app did not copy that folder when it laid out the deployment. It +does now, and a test fails if the deployment's code imports anything the desktop app does not copy. +An install laid out by an earlier app is asked to finish installing when it starts; installing +downloads the same release again with the missing folder. Settings and data are kept. + ### A new Bot is made from a sheet that asks where it runs first New Bot opens a sheet instead of the four-screen wizard. It asks where the Bot will run, with Run it diff --git a/desktop/src-tauri/src/deployment.rs b/desktop/src-tauri/src/deployment.rs index e08cc85d6f..563d8474e9 100644 --- a/desktop/src-tauri/src/deployment.rs +++ b/desktop/src-tauri/src/deployment.rs @@ -30,6 +30,10 @@ const IMAGES: &str = "container-images.json"; #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)] pub struct Installed { pub version: String, + /// The top-level entries this app laid out. A stamp written before this existed reads as + /// empty, which never matches, so that deployment is laid out again. + #[serde(default)] + pub layout: Vec, } /// The tarball GitHub publishes for a tag. @@ -141,6 +145,7 @@ pub fn record(root: &Path, version: &str) -> std::io::Result<()> { std::fs::create_dir_all(root)?; let stamp = Installed { version: version.to_string(), + layout: layout(), }; std::fs::write( stamp_path(root), @@ -157,16 +162,31 @@ pub fn record(root: &Path, version: &str) -> std::io::Result<()> { /// The image manifest is the one exception. A deployment laid down by an app that predates it has a /// stamp that matches and no manifest, and re-fetching is a better answer than an error about a /// file the person has never heard of. +/// +/// So is a stamp that laid out less than this app copies. An app that left a folder out wrote a +/// stamp that still matches its version, and Repair would otherwise keep the gap forever. pub fn needs_fetch(root: &Path, wanted: &str) -> bool { if !images_path(root).exists() { return true; } match installed(root) { - Some(found) => found.version != wanted, + // A deployment laid out by an app that copied less is missing something this one runs. + Some(found) => found.version != wanted || found.layout != layout(), None => true, } } +/// What this app lays out, in a fixed order, as recorded in the stamp. +fn layout() -> Vec { + let mut entries: Vec = REQUIRED + .iter() + .chain(ALSO_COPIED.iter()) + .map(|entry| entry.to_string()) + .collect(); + entries.sort(); + entries +} + /// Everything the three host processes and Compose need from the tree. /// /// Named rather than "the whole repository" because most of it is not needed to run: the charts, the @@ -175,8 +195,12 @@ pub fn needs_fetch(root: &Path, wanted: &str) -> bool { pub const REQUIRED: [&str; 4] = ["docker-compose.yml", "server", "app", "worker"]; /// The rest of what a deployment needs, which is not what it is checked for. -pub const ALSO_COPIED: [&str; 8] = [ +pub const ALSO_COPIED: [&str; 9] = [ "shared", + // Not a workspace, but the server imports its egress rules from `agent-computer/src` and + // `scripts/start-local-chrome-computer.ts` its browser runtime. Without it the server exits at + // start with `Cannot find module '../../../agent-computer/src/egress'`. + "agent-computer", // A root workspace the server imports as `@openbot/toolkit`. Without it, `bun install // --frozen-lockfile` stops with `Workspace not found "toolkit"`. "toolkit", @@ -531,6 +555,117 @@ mod tests { assert!(!docs, "the rest of the tree is not part of a deployment"); } + /// Relative module specifiers in one source file: `from "..."`, `import("...")` and + /// `require("...")`, with either quote. Package names are not relative and are skipped. + fn relative_imports(source: &str) -> Vec { + let mut found = Vec::new(); + for opener in [ + "from \"", + "from '", + "import(\"", + "import('", + "require(\"", + "require('", + ] { + let quote = opener.chars().last().unwrap(); + for (at, _) in source.match_indices(opener) { + let rest = &source[at + opener.len()..]; + if let Some(end) = rest.find(quote) { + let specifier = &rest[..end]; + if specifier.starts_with("./") || specifier.starts_with("../") { + found.push(specifier.to_owned()); + } + } + } + } + found + } + + fn sources_under(dir: &Path, into: &mut Vec) { + let Ok(entries) = std::fs::read_dir(dir) else { + return; + }; + for entry in entries.flatten() { + let path = entry.path(); + let name = entry.file_name().to_string_lossy().into_owned(); + if path.is_dir() { + // Tests and build output run from a checkout, never from a deployment. + if !matches!( + name.as_str(), + "node_modules" | "dist" | "tests" | "test" | "__tests__" + ) { + sources_under(&path, into); + } + } else if [".ts", ".tsx", ".js", ".mjs", ".cjs"] + .iter() + .any(|suffix| name.ends_with(suffix)) + && !name.contains(".test.") + && !name.contains(".spec.") + { + into.push(path); + } + } + } + + /// `#673` made the server import `../../../agent-computer/src/egress`, and the unpacker kept + /// copying only the folders listed here, so every new Desktop install of v0.2.0 had a server + /// that exited at start with `Cannot find module`. A checkout has every folder, which is why + /// nothing caught it. This walks the real tree: whatever the unpacked code imports by a + /// relative path has to be something the unpacker keeps. + #[test] + fn everything_the_unpacked_code_imports_is_unpacked_too() { + let repo = Path::new(env!("CARGO_MANIFEST_DIR")) + .join("../..") + .canonicalize() + .unwrap(); + let kept: Vec<&str> = REQUIRED.iter().chain(ALSO_COPIED.iter()).copied().collect(); + let mut files = Vec::new(); + for top in &kept { + sources_under(&repo.join(top), &mut files); + } + assert!( + files + .iter() + .any(|file| file.starts_with(repo.join("server"))), + "the walk found no server sources, so it proves nothing" + ); + + let mut missing = Vec::new(); + for file in &files { + let Ok(source) = std::fs::read_to_string(file) else { + continue; + }; + for specifier in relative_imports(&source) { + let mut resolved = file.parent().unwrap().to_path_buf(); + for part in Path::new(&specifier).components() { + match part { + std::path::Component::ParentDir => { + resolved.pop(); + } + std::path::Component::Normal(name) => resolved.push(name), + _ => {} + } + } + let top = resolved + .strip_prefix(&repo) + .ok() + .and_then(|inside| inside.components().next()) + .map(|first| first.as_os_str().to_string_lossy().into_owned()); + if !top.as_deref().is_some_and(|top| kept.contains(&top)) { + missing.push(format!( + "{} imports {specifier}", + file.strip_prefix(&repo).unwrap_or(file).display() + )); + } + } + } + assert!( + missing.is_empty(), + "a deployment would not contain what these import:\n{}", + missing.join("\n") + ); + } + #[test] fn a_recorded_version_is_not_fetched_again() { let dir = std::env::temp_dir().join(format!("openbot-dep-same-{}", std::process::id())); @@ -544,6 +679,24 @@ mod tests { std::fs::remove_dir_all(&dir).ok(); } + /// v0.2.0 installs were laid out by an app that did not copy `agent-computer`, and their stamp + /// matches the version they run, so Repair never fetched again and the server never started. + /// A stamp says what it laid out; one that laid out less than this app copies is fetched once. + #[test] + fn a_deployment_laid_out_by_an_app_that_copied_less_is_fetched_again() { + let dir = std::env::temp_dir().join(format!("openbot-dep-layout-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(images_path(&dir), "{}").unwrap(); + // Exactly what an earlier app wrote. + std::fs::write(stamp_path(&dir), r#"{"version":"v0.2.0"}"#).unwrap(); + assert!(needs_fetch(&dir, "v0.2.0")); + assert_eq!(installed(&dir).unwrap().version, "v0.2.0"); + + record(&dir, "v0.2.0").unwrap(); + assert!(!needs_fetch(&dir, "v0.2.0")); + std::fs::remove_dir_all(&dir).ok(); + } + #[test] fn an_interrupted_fetch_is_replaced_rather_than_trusted() { // Files present, stamp absent: what an interrupted extract leaves behind. The stamp is diff --git a/desktop/src-tauri/src/preparation.rs b/desktop/src-tauri/src/preparation.rs index f29c50b6e7..0747dae969 100644 --- a/desktop/src-tauri/src/preparation.rs +++ b/desktop/src-tauri/src/preparation.rs @@ -322,6 +322,15 @@ fn require_with( if let Some(problem) = stack::deployment_problem(root) { return Err(required(problem)); } + // A deployment laid out by an app that copied less still matches its own fingerprint. Install + // lays it out again; Start cannot. + if deployment::installed(root) + .is_some_and(|found| deployment::needs_fetch(root, &found.version)) + { + return Err(required( + "The deployment was laid out by an earlier OpenBot and is missing files this version runs.", + )); + } let bun = dependencies_ready(root)?; verify(&record.images)?; Ok(bun) @@ -449,6 +458,20 @@ fn main() { ); } + /// A v0.2.0 install prepared by an app that did not copy `agent-computer` still matched its + /// fingerprint, so Start ran a server that could not load. Start now sends it back to Install, + /// which lays the deployment out again. + #[test] + fn a_deployment_laid_out_by_an_earlier_app_requires_installation_before_start() { + let f = Fixture::new(); + // Exactly what an earlier app wrote, in place before it prepared the installation. + std::fs::write(deployment::stamp_path(&f.root), r#"{"version":"v-test"}"#).unwrap(); + f.prepared(None); + let problem = require_with(&f.root, None, |_| Ok(())) + .expect_err("a deployment missing folders this app copies cannot start"); + assert!(problem.said.contains("Finish installing")); + } + #[test] fn cached_dependencies_with_unsupported_runtime_require_installation_before_start() { let f = Fixture::new();