From 658effb964cd1cc017142a18a1a510811e218e0a Mon Sep 17 00:00:00 2001 From: krypticmouse Date: Sun, 24 May 2026 15:26:07 +0000 Subject: [PATCH] refactor(desktop): extract testable uv-sync error helpers + unit tests (#331) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The uv-sync failure handling added in #398 was inline in the async `boot_backend` GUI path, so its logic — exit-code rendering and the stderr "tail" extraction — could only be verified by running the desktop app. This extracts the pure logic into three free functions and adds unit tests, so the message formatting is now covered by `cargo test` with no GUI / webview runtime needed. Extracted: - `uv_sync_stderr_tail(stderr, max_chars)` — last N chars of stderr, trimmed, on char boundaries. The original inline version used `.rev().take(N).collect().chars().rev().collect()`; the new version is `skip(count - N)` which is clearer and equally UTF-8-safe (matters because Windows consoles emit non-ASCII cp9xx bytes — a byte slice could split a codepoint). - `format_uv_sync_failure(root, exit_code, stderr)` — the non-zero-exit message. Now renders a missing exit code (signal-terminated process) as "unknown" instead of a misleading "-1". - `format_uv_sync_spawn_error(root, uv_bin, err)` — the can't-spawn message. `boot_backend` now calls these instead of formatting inline. Tests (7, in `#[cfg(test)] mod tests`): - tail returns whole string when shorter than the limit - tail keeps the END (the actionable line), not the spinner-noise start - tail trims surrounding whitespace - tail never splits a multi-byte codepoint (500×"é", limit 100 → exactly 100 chars, all "é") - failure message includes exit code + stderr tail + the actionable "run uv sync manually" hint - missing exit code renders as "exit unknown", never "exit -1" - spawn-error names the uv binary path and repo root Verified the logic standalone via `rustc --test` (7/7 pass). In CI they run via `cargo test` in desktop.yml's `validate` job, which already builds the Tauri crate with the webkit deps and runs on every PR that touches `frontend/**` — so this changes the existing `cargo check` step to `cargo test` (a superset: same build coverage, plus the tests). This doesn't verify the GUI *behavior* (that still needs a human running the app, or the windows-latest empirical path) — but the error-message logic that was previously untestable now has automated coverage. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/desktop.yml | 7 +- frontend/src-tauri/src/lib.rs | 151 ++++++++++++++++++++++++++++------ 2 files changed, 131 insertions(+), 27 deletions(-) diff --git a/.github/workflows/desktop.yml b/.github/workflows/desktop.yml index d07355ed..db6da17c 100644 --- a/.github/workflows/desktop.yml +++ b/.github/workflows/desktop.yml @@ -66,9 +66,12 @@ jobs: - name: Create frontend dist stub run: mkdir -p frontend/dist && echo '' > frontend/dist/index.html - - name: Cargo check + # `cargo test` builds the crate (same coverage as the old `cargo check`) + # and runs the unit tests, including the #331 uv-sync error-formatting + # helpers. Static command, no untrusted input — no injection surface. + - name: Cargo test working-directory: frontend/src-tauri - run: cargo check + run: cargo test # Remove stale artifacts from the desktop-latest pre-release so that # only the current build's files are available for download. diff --git a/frontend/src-tauri/src/lib.rs b/frontend/src-tauri/src/lib.rs index 5643640f..cea69a2b 100644 --- a/frontend/src-tauri/src/lib.rs +++ b/frontend/src-tauri/src/lib.rs @@ -405,6 +405,57 @@ async fn pull_model(model: &str) -> Result<(), String> { Ok(()) } +// --------------------------------------------------------------------------- +// uv sync error formatting (pure helpers — unit-tested, see #331) +// --------------------------------------------------------------------------- + +/// Last `max_chars` characters of a `uv sync` stderr stream, trimmed. +/// +/// uv's actionable diagnostic almost always lands at the tail of the +/// stream, so when surfacing a failure to the user we show the end, not +/// the (usually noisy progress-spinner) beginning. Operates on `char` +/// boundaries so it never splits a multi-byte UTF-8 codepoint — important +/// because Windows consoles emit non-ASCII (cp9xx) bytes. +fn uv_sync_stderr_tail(stderr: &str, max_chars: usize) -> String { + let total = stderr.chars().count(); + let skip = total.saturating_sub(max_chars); + stderr.chars().skip(skip).collect::().trim().to_string() +} + +/// Error message shown when `uv sync` runs but exits non-zero (#331). +/// +/// `exit_code` is `None` when the process was terminated by a signal with +/// no exit code (rendered as "unknown" rather than a misleading -1). +fn format_uv_sync_failure( + root: &std::path::Path, + exit_code: Option, + stderr: &str, +) -> String { + let code = exit_code + .map(|c| c.to_string()) + .unwrap_or_else(|| "unknown".to_string()); + format!( + "`uv sync` failed in {} (exit {}). Last output:\n\n{}\n\n\ + Try opening a terminal in that directory and running \ + `uv sync --extra server` manually for the full output.", + root.display(), + code, + uv_sync_stderr_tail(stderr, 800), + ) +} + +/// Error message shown when `uv sync` can't even be spawned (#331) — +/// e.g. the resolved `uv` binary doesn't exist or isn't executable. +fn format_uv_sync_spawn_error(root: &std::path::Path, uv_bin: &str, err: &str) -> String { + format!( + "Could not run `uv sync`: {}. Verify uv is installed at \ + `{}` and the OpenJarvis repo is at `{}`.", + err, + uv_bin, + root.display(), + ) +} + // --------------------------------------------------------------------------- // Backend boot sequence (runs in background after app launch) // --------------------------------------------------------------------------- @@ -695,36 +746,13 @@ async fn boot_backend(backend: SharedBackend, status: SharedStatus) { match sync_output { Ok(out) if !out.status.success() => { let stderr = String::from_utf8_lossy(&out.stderr); - // Take the last ~800 chars — uv's most useful diagnostic line - // is usually at the tail of the stream. - let tail: String = stderr - .chars() - .rev() - .take(800) - .collect::() - .chars() - .rev() - .collect(); let mut s = status.lock().await; - s.error = Some(format!( - "`uv sync` failed in {} (exit {}). Last output:\n\n{}\n\n\ - Try opening a terminal in that directory and running \ - `uv sync --extra server` manually for the full output.", - root.display(), - out.status.code().unwrap_or(-1), - tail.trim(), - )); + s.error = Some(format_uv_sync_failure(root, out.status.code(), &stderr)); return; } Err(e) => { let mut s = status.lock().await; - s.error = Some(format!( - "Could not run `uv sync`: {}. Verify uv is installed at \ - `{}` and the OpenJarvis repo is at `{}`.", - e, - uv_bin, - root.display(), - )); + s.error = Some(format_uv_sync_spawn_error(root, &uv_bin, &e.to_string())); return; } Ok(_) => {} // success — fall through @@ -1788,3 +1816,76 @@ pub fn run() { } }); } + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +#[cfg(test)] +mod tests { + use super::{format_uv_sync_failure, format_uv_sync_spawn_error, uv_sync_stderr_tail}; + use std::path::Path; + + #[test] + fn tail_returns_whole_string_when_shorter_than_limit() { + assert_eq!(uv_sync_stderr_tail("short error", 800), "short error"); + } + + #[test] + fn tail_keeps_the_end_not_the_beginning() { + // uv's actionable line is at the end; the spinner noise is at the start. + let s = format!("{}ACTUAL ERROR HERE", "spinner-noise ".repeat(200)); + let tail = uv_sync_stderr_tail(&s, 40); + assert!(tail.ends_with("ACTUAL ERROR HERE"), "tail was: {tail:?}"); + assert!(!tail.contains("spinner-noise spinner-noise spinner-noise")); + assert!(tail.chars().count() <= 40); + } + + #[test] + fn tail_trims_surrounding_whitespace() { + assert_eq!(uv_sync_stderr_tail(" \n padded \n ", 800), "padded"); + } + + #[test] + fn tail_never_splits_a_multibyte_codepoint() { + // Each "é" is 2 bytes / 1 char. A byte-based slice could panic or + // produce invalid UTF-8; the char-based tail must not. + let s = "é".repeat(500); + let tail = uv_sync_stderr_tail(&s, 100); + assert_eq!(tail.chars().count(), 100); + assert!(tail.chars().all(|c| c == 'é')); + } + + #[test] + fn failure_message_includes_exit_code_and_tail_and_hint() { + let msg = format_uv_sync_failure( + Path::new("/home/u/.openjarvis/src"), + Some(2), + "error: failed to resolve numpy==2.1.3", + ); + assert!(msg.contains("exit 2")); + assert!(msg.contains("/home/u/.openjarvis/src")); + assert!(msg.contains("failed to resolve numpy==2.1.3")); + assert!(msg.contains("uv sync --extra server")); // actionable next step + } + + #[test] + fn failure_message_renders_missing_exit_code_as_unknown() { + // Process killed by signal → no exit code. Must not show a misleading -1. + let msg = format_uv_sync_failure(Path::new("/x"), None, "boom"); + assert!(msg.contains("exit unknown")); + assert!(!msg.contains("exit -1")); + } + + #[test] + fn spawn_error_names_the_binary_and_root() { + let msg = format_uv_sync_spawn_error( + Path::new("/repo"), + "C:\\Users\\me\\.local\\bin\\uv.exe", + "No such file or directory (os error 2)", + ); + assert!(msg.contains("C:\\Users\\me\\.local\\bin\\uv.exe")); + assert!(msg.contains("/repo")); + assert!(msg.contains("No such file or directory")); + } +}