Skip to content

Commit 2fbeb1b

Browse files
authored
fix(desktop): copy agent-computer into the deployment so the server starts (#813)
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.
1 parent 68fd9f6 commit 2fbeb1b

3 files changed

Lines changed: 187 additions & 2 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,15 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged.
3232
`code_required` (below). A server and a computer must be upgraded together: an older computer has
3333
no `/sign-in/code`.
3434

35+
### A new desktop install starts its server again
36+
37+
A desktop install of v0.2.0 stopped at start with `server stopped straight away (exit status: 1)` and
38+
`Cannot find module '../../../agent-computer/src/egress'`. The server imports its egress rules from
39+
`agent-computer`, and the desktop app did not copy that folder when it laid out the deployment. It
40+
does now, and a test fails if the deployment's code imports anything the desktop app does not copy.
41+
An install laid out by an earlier app is asked to finish installing when it starts; installing
42+
downloads the same release again with the missing folder. Settings and data are kept.
43+
3544
### A new Bot is made from a sheet that asks where it runs first
3645

3746
New Bot opens a sheet instead of the four-screen wizard. It asks where the Bot will run, with Run it

‎desktop/src-tauri/src/deployment.rs‎

Lines changed: 155 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,10 @@ const IMAGES: &str = "container-images.json";
3030
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)]
3131
pub struct Installed {
3232
pub version: String,
33+
/// The top-level entries this app laid out. A stamp written before this existed reads as
34+
/// empty, which never matches, so that deployment is laid out again.
35+
#[serde(default)]
36+
pub layout: Vec<String>,
3337
}
3438

3539
/// The tarball GitHub publishes for a tag.
@@ -141,6 +145,7 @@ pub fn record(root: &Path, version: &str) -> std::io::Result<()> {
141145
std::fs::create_dir_all(root)?;
142146
let stamp = Installed {
143147
version: version.to_string(),
148+
layout: layout(),
144149
};
145150
std::fs::write(
146151
stamp_path(root),
@@ -157,16 +162,31 @@ pub fn record(root: &Path, version: &str) -> std::io::Result<()> {
157162
/// The image manifest is the one exception. A deployment laid down by an app that predates it has a
158163
/// stamp that matches and no manifest, and re-fetching is a better answer than an error about a
159164
/// file the person has never heard of.
165+
///
166+
/// So is a stamp that laid out less than this app copies. An app that left a folder out wrote a
167+
/// stamp that still matches its version, and Repair would otherwise keep the gap forever.
160168
pub fn needs_fetch(root: &Path, wanted: &str) -> bool {
161169
if !images_path(root).exists() {
162170
return true;
163171
}
164172
match installed(root) {
165-
Some(found) => found.version != wanted,
173+
// A deployment laid out by an app that copied less is missing something this one runs.
174+
Some(found) => found.version != wanted || found.layout != layout(),
166175
None => true,
167176
}
168177
}
169178

179+
/// What this app lays out, in a fixed order, as recorded in the stamp.
180+
fn layout() -> Vec<String> {
181+
let mut entries: Vec<String> = REQUIRED
182+
.iter()
183+
.chain(ALSO_COPIED.iter())
184+
.map(|entry| entry.to_string())
185+
.collect();
186+
entries.sort();
187+
entries
188+
}
189+
170190
/// Everything the three host processes and Compose need from the tree.
171191
///
172192
/// 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 {
175195
pub const REQUIRED: [&str; 4] = ["docker-compose.yml", "server", "app", "worker"];
176196

177197
/// The rest of what a deployment needs, which is not what it is checked for.
178-
pub const ALSO_COPIED: [&str; 8] = [
198+
pub const ALSO_COPIED: [&str; 9] = [
179199
"shared",
200+
// Not a workspace, but the server imports its egress rules from `agent-computer/src` and
201+
// `scripts/start-local-chrome-computer.ts` its browser runtime. Without it the server exits at
202+
// start with `Cannot find module '../../../agent-computer/src/egress'`.
203+
"agent-computer",
180204
// A root workspace the server imports as `@openbot/toolkit`. Without it, `bun install
181205
// --frozen-lockfile` stops with `Workspace not found "toolkit"`.
182206
"toolkit",
@@ -531,6 +555,117 @@ mod tests {
531555
assert!(!docs, "the rest of the tree is not part of a deployment");
532556
}
533557

558+
/// Relative module specifiers in one source file: `from "..."`, `import("...")` and
559+
/// `require("...")`, with either quote. Package names are not relative and are skipped.
560+
fn relative_imports(source: &str) -> Vec<String> {
561+
let mut found = Vec::new();
562+
for opener in [
563+
"from \"",
564+
"from '",
565+
"import(\"",
566+
"import('",
567+
"require(\"",
568+
"require('",
569+
] {
570+
let quote = opener.chars().last().unwrap();
571+
for (at, _) in source.match_indices(opener) {
572+
let rest = &source[at + opener.len()..];
573+
if let Some(end) = rest.find(quote) {
574+
let specifier = &rest[..end];
575+
if specifier.starts_with("./") || specifier.starts_with("../") {
576+
found.push(specifier.to_owned());
577+
}
578+
}
579+
}
580+
}
581+
found
582+
}
583+
584+
fn sources_under(dir: &Path, into: &mut Vec<PathBuf>) {
585+
let Ok(entries) = std::fs::read_dir(dir) else {
586+
return;
587+
};
588+
for entry in entries.flatten() {
589+
let path = entry.path();
590+
let name = entry.file_name().to_string_lossy().into_owned();
591+
if path.is_dir() {
592+
// Tests and build output run from a checkout, never from a deployment.
593+
if !matches!(
594+
name.as_str(),
595+
"node_modules" | "dist" | "tests" | "test" | "__tests__"
596+
) {
597+
sources_under(&path, into);
598+
}
599+
} else if [".ts", ".tsx", ".js", ".mjs", ".cjs"]
600+
.iter()
601+
.any(|suffix| name.ends_with(suffix))
602+
&& !name.contains(".test.")
603+
&& !name.contains(".spec.")
604+
{
605+
into.push(path);
606+
}
607+
}
608+
}
609+
610+
/// `#673` made the server import `../../../agent-computer/src/egress`, and the unpacker kept
611+
/// copying only the folders listed here, so every new Desktop install of v0.2.0 had a server
612+
/// that exited at start with `Cannot find module`. A checkout has every folder, which is why
613+
/// nothing caught it. This walks the real tree: whatever the unpacked code imports by a
614+
/// relative path has to be something the unpacker keeps.
615+
#[test]
616+
fn everything_the_unpacked_code_imports_is_unpacked_too() {
617+
let repo = Path::new(env!("CARGO_MANIFEST_DIR"))
618+
.join("../..")
619+
.canonicalize()
620+
.unwrap();
621+
let kept: Vec<&str> = REQUIRED.iter().chain(ALSO_COPIED.iter()).copied().collect();
622+
let mut files = Vec::new();
623+
for top in &kept {
624+
sources_under(&repo.join(top), &mut files);
625+
}
626+
assert!(
627+
files
628+
.iter()
629+
.any(|file| file.starts_with(repo.join("server"))),
630+
"the walk found no server sources, so it proves nothing"
631+
);
632+
633+
let mut missing = Vec::new();
634+
for file in &files {
635+
let Ok(source) = std::fs::read_to_string(file) else {
636+
continue;
637+
};
638+
for specifier in relative_imports(&source) {
639+
let mut resolved = file.parent().unwrap().to_path_buf();
640+
for part in Path::new(&specifier).components() {
641+
match part {
642+
std::path::Component::ParentDir => {
643+
resolved.pop();
644+
}
645+
std::path::Component::Normal(name) => resolved.push(name),
646+
_ => {}
647+
}
648+
}
649+
let top = resolved
650+
.strip_prefix(&repo)
651+
.ok()
652+
.and_then(|inside| inside.components().next())
653+
.map(|first| first.as_os_str().to_string_lossy().into_owned());
654+
if !top.as_deref().is_some_and(|top| kept.contains(&top)) {
655+
missing.push(format!(
656+
"{} imports {specifier}",
657+
file.strip_prefix(&repo).unwrap_or(file).display()
658+
));
659+
}
660+
}
661+
}
662+
assert!(
663+
missing.is_empty(),
664+
"a deployment would not contain what these import:\n{}",
665+
missing.join("\n")
666+
);
667+
}
668+
534669
#[test]
535670
fn a_recorded_version_is_not_fetched_again() {
536671
let dir = std::env::temp_dir().join(format!("openbot-dep-same-{}", std::process::id()));
@@ -544,6 +679,24 @@ mod tests {
544679
std::fs::remove_dir_all(&dir).ok();
545680
}
546681

682+
/// v0.2.0 installs were laid out by an app that did not copy `agent-computer`, and their stamp
683+
/// matches the version they run, so Repair never fetched again and the server never started.
684+
/// A stamp says what it laid out; one that laid out less than this app copies is fetched once.
685+
#[test]
686+
fn a_deployment_laid_out_by_an_app_that_copied_less_is_fetched_again() {
687+
let dir = std::env::temp_dir().join(format!("openbot-dep-layout-{}", std::process::id()));
688+
std::fs::create_dir_all(&dir).unwrap();
689+
std::fs::write(images_path(&dir), "{}").unwrap();
690+
// Exactly what an earlier app wrote.
691+
std::fs::write(stamp_path(&dir), r#"{"version":"v0.2.0"}"#).unwrap();
692+
assert!(needs_fetch(&dir, "v0.2.0"));
693+
assert_eq!(installed(&dir).unwrap().version, "v0.2.0");
694+
695+
record(&dir, "v0.2.0").unwrap();
696+
assert!(!needs_fetch(&dir, "v0.2.0"));
697+
std::fs::remove_dir_all(&dir).ok();
698+
}
699+
547700
#[test]
548701
fn an_interrupted_fetch_is_replaced_rather_than_trusted() {
549702
// Files present, stamp absent: what an interrupted extract leaves behind. The stamp is

‎desktop/src-tauri/src/preparation.rs‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -322,6 +322,15 @@ fn require_with(
322322
if let Some(problem) = stack::deployment_problem(root) {
323323
return Err(required(problem));
324324
}
325+
// A deployment laid out by an app that copied less still matches its own fingerprint. Install
326+
// lays it out again; Start cannot.
327+
if deployment::installed(root)
328+
.is_some_and(|found| deployment::needs_fetch(root, &found.version))
329+
{
330+
return Err(required(
331+
"The deployment was laid out by an earlier OpenBot and is missing files this version runs.",
332+
));
333+
}
325334
let bun = dependencies_ready(root)?;
326335
verify(&record.images)?;
327336
Ok(bun)
@@ -449,6 +458,20 @@ fn main() {
449458
);
450459
}
451460

461+
/// A v0.2.0 install prepared by an app that did not copy `agent-computer` still matched its
462+
/// fingerprint, so Start ran a server that could not load. Start now sends it back to Install,
463+
/// which lays the deployment out again.
464+
#[test]
465+
fn a_deployment_laid_out_by_an_earlier_app_requires_installation_before_start() {
466+
let f = Fixture::new();
467+
// Exactly what an earlier app wrote, in place before it prepared the installation.
468+
std::fs::write(deployment::stamp_path(&f.root), r#"{"version":"v-test"}"#).unwrap();
469+
f.prepared(None);
470+
let problem = require_with(&f.root, None, |_| Ok(()))
471+
.expect_err("a deployment missing folders this app copies cannot start");
472+
assert!(problem.said.contains("Finish installing"));
473+
}
474+
452475
#[test]
453476
fn cached_dependencies_with_unsupported_runtime_require_installation_before_start() {
454477
let f = Fixture::new();

0 commit comments

Comments
 (0)