From 5c8a495b9fd72fe88f8d3dae5c7bf2c4f5f62d07 Mon Sep 17 00:00:00 2001 From: vikingowl Date: Tue, 12 Aug 2025 06:00:10 +0200 Subject: [PATCH] Revert "[feat] enhance progress logging, introduce TTY-aware banners, and implement hardened SHA-256 verification for model downloads" This reverts commit e954902aa93b8d4bdcd723972ce0d3d510649ff4. --- src/main.rs | 14 ----- src/models.rs | 145 ------------------------------------------------ src/progress.rs | 49 +--------------- 3 files changed, 2 insertions(+), 206 deletions(-) diff --git a/src/main.rs b/src/main.rs index c1f6fd2..6ead4d7 100644 --- a/src/main.rs +++ b/src/main.rs @@ -313,11 +313,6 @@ fn run() -> Result<()> { // Route subsequent INFO/WARN/DEBUG logs through the cliclack/indicatif area polyscribe::progress::set_global_progress_manager(&pm); - // Show a friendly intro banner (TTY-aware via cliclack). Ignore errors. - if !polyscribe::is_quiet() { - let _ = cliclack::intro("PolyScribe"); - } - // Determine formats let out_formats = if args.out_format.is_empty() { OutputFormats::all() @@ -492,15 +487,6 @@ fn run() -> Result<()> { if had_error { polyscribe::elog!("One or more inputs failed"); } } - // Outro banner summarizing result; ignore errors. - if !polyscribe::is_quiet() { - if had_error { - let _ = cliclack::outro("Completed with errors. Some inputs failed."); - } else { - let _ = cliclack::outro("All done. Outputs written."); - } - } - if had_error { std::process::exit(2); } let _elapsed = start_overall.elapsed(); Ok(()) diff --git a/src/models.rs b/src/models.rs index 9b26704..a355ac1 100644 --- a/src/models.rs +++ b/src/models.rs @@ -616,30 +616,6 @@ fn download_one_model_with_progress(client: &Client, models_dir: &Path, entry: & } if final_path.exists() { let _ = std::fs::remove_file(&final_path); } std::fs::rename(&tmp_path, &final_path).with_context(|| format!("Failed to move into place: {}", final_path.display()))?; - // Hardened verification after save - if let Some(expected) = &entry.sha256 { - match compute_file_sha256_hex(&final_path) { - Ok(rehash) => { - if !rehash.eq_ignore_ascii_case(expected) { - let _ = std::fs::remove_file(&final_path); - return Err(anyhow!( - "Downloaded file failed SHA-256 verification after save for {}: expected {}, got {}. The file has been removed. Please try downloading again. If the problem persists, check your network connection and disk space, or report this issue.", - entry.name, - expected, - rehash - )); - } - } - Err(e) => { - let _ = std::fs::remove_file(&final_path); - return Err(anyhow!( - "Failed to verify downloaded file {}: {}. The file has been removed. Please try again.", - final_path.display(), - e - )); - } - } - } item.set_progress(1.0); item.finish_with("done"); return Ok(()); @@ -692,32 +668,6 @@ fn download_one_model_with_progress(client: &Client, models_dir: &Path, entry: & if final_path.exists() { let _ = std::fs::remove_file(&final_path); } std::fs::rename(&tmp_path, &final_path) .with_context(|| format!("Failed to move into place: {}", final_path.display()))?; - - // Hardened verification: recompute SHA-256 from the saved file and compare to expected. - if let Some(expected) = &entry.sha256 { - match compute_file_sha256_hex(&final_path) { - Ok(rehash) => { - if !rehash.eq_ignore_ascii_case(expected) { - let _ = std::fs::remove_file(&final_path); - return Err(anyhow!( - "Downloaded file failed SHA-256 verification after save for {}: expected {}, got {}. The file has been removed. Please try downloading again. If the problem persists, check your network connection and disk space, or report this issue.", - entry.name, - expected, - rehash - )); - } - } - Err(e) => { - let _ = std::fs::remove_file(&final_path); - return Err(anyhow!( - "Failed to verify downloaded file {}: {}. The file has been removed. Please try again.", - final_path.display(), - e - )); - } - } - } - item.finish_with("done"); Ok(()) } @@ -822,30 +772,6 @@ fn download_one_model(client: &Client, models_dir: &Path, entry: &ModelEntry) -> } std::fs::rename(&tmp_path, &final_path) .with_context(|| format!("Failed to move into place: {}", final_path.display()))?; - // Hardened verification after save - if let Some(expected) = &entry.sha256 { - match compute_file_sha256_hex(&final_path) { - Ok(rehash) => { - if !rehash.eq_ignore_ascii_case(expected) { - let _ = std::fs::remove_file(&final_path); - return Err(anyhow!( - "Downloaded file failed SHA-256 verification after save for {}: expected {}, got {}. The file has been removed. Please try downloading again. If the problem persists, check your network connection and disk space, or report this issue.", - entry.name, - expected, - rehash - )); - } - } - Err(e) => { - let _ = std::fs::remove_file(&final_path); - return Err(anyhow!( - "Failed to verify downloaded file {}: {}. The file has been removed. Please try again.", - final_path.display(), - e - )); - } - } - } qlog!("Saved: {}", final_path.display()); return Ok(()); } @@ -911,32 +837,6 @@ fn download_one_model(client: &Client, models_dir: &Path, entry: &ModelEntry) -> } std::fs::rename(&tmp_path, &final_path) .with_context(|| format!("Failed to move into place: {}", final_path.display()))?; - - // Hardened verification: recompute SHA-256 from the saved file and compare to expected. - if let Some(expected) = &entry.sha256 { - match compute_file_sha256_hex(&final_path) { - Ok(rehash) => { - if !rehash.eq_ignore_ascii_case(expected) { - let _ = std::fs::remove_file(&final_path); - return Err(anyhow!( - "Downloaded file failed SHA-256 verification after save for {}: expected {}, got {}. The file has been removed. Please try downloading again. If the problem persists, check your network connection and disk space, or report this issue.", - entry.name, - expected, - rehash - )); - } - } - Err(e) => { - let _ = std::fs::remove_file(&final_path); - return Err(anyhow!( - "Failed to verify downloaded file {}: {}. The file has been removed. Please try again.", - final_path.display(), - e - )); - } - } - } - qlog!("Saved: {}", final_path.display()); Ok(()) } @@ -1374,49 +1274,4 @@ mod tests { panic!("progress manager did not expose current state"); } } - - #[test] - fn test_wrong_hash_deletes_temp_and_errors() { - use std::sync::{Mutex, OnceLock}; - static ENV_LOCK: OnceLock> = OnceLock::new(); - let _guard = ENV_LOCK.get_or_init(|| Mutex::new(())).lock().unwrap(); - - let tmp_models = tempdir().unwrap(); - let tmp_base = tempdir().unwrap(); - - // Prepare source model file content and a pre-existing local file to trigger update - let model_name = "tiny.en-q5_1"; - let src_path = tmp_base.path().join(format!("ggml-{}.bin", model_name)); - let content = b"model data"; - fs::write(&src_path, content).unwrap(); - let wrong_sha = "0000000000000000000000000000000000000000000000000000000000000000".to_string(); - - let local_path = tmp_models.path().join(format!("ggml-{}.bin", model_name)); - let original = b"old local"; - fs::write(&local_path, original).unwrap(); - - unsafe { - std::env::set_var("POLYSCRIBE_MODELS_BASE_COPY_DIR", tmp_base.path()); - } - - // Construct a ModelEntry with wrong expected sha and call the downloader directly - let client = Client::builder().build().unwrap(); - let entry = ModelEntry { - name: model_name.to_string(), - base: "tiny".to_string(), - subtype: "en-q5_1".to_string(), - size: content.len() as u64, - sha256: Some(wrong_sha), - repo: "ggerganov/whisper.cpp".to_string(), - }; - let res = super::download_one_model(&client, tmp_models.path(), &entry); - assert!(res.is_err(), "expected error due to wrong hash"); - - let final_path = tmp_models.path().join(format!("ggml-{}.bin", model_name)); - let tmp_path = tmp_models.path().join(format!("ggml-{}.bin.part", model_name)); - assert!(final_path.exists(), "existing local file should remain when new download fails"); - let preserved = fs::read(&final_path).unwrap(); - assert_eq!(preserved, original, "existing local file must be preserved"); - assert!(!tmp_path.exists(), ".part file should be deleted on hash mismatch"); - } } diff --git a/src/progress.rs b/src/progress.rs index d67e4aa..56a2e06 100644 --- a/src/progress.rs +++ b/src/progress.rs @@ -359,58 +359,13 @@ impl ProgressManager { /// Print a line above the bars safely (TTY-aware). Falls back to eprintln! when disabled. pub fn println_above_bars(&self, line: &str) { - // Try to interpret certain INFO lines as a stable title + dynamic message. - // Examples to match: - // - "INFO: Fetching online data: listing models from ggerganov/whisper.cpp..." - // -> header = "INFO: Fetching online data"; info = "listing models from ..." - // - "INFO: Downloading tiny.en-q5_1 (252 MiB | https://...)..." - // -> header = "INFO: Downloading"; info = rest - // - "INFO: Total 1/3" (defensive): header = "INFO: Total"; info = rest - let parsed: Option<(String, String)> = { - let s = line.trim(); - if let Some(rest) = s.strip_prefix("INFO: ") { - // Case A: explicit title followed by colon - if let Some((title, body)) = rest.split_once(':') { - let title_clean = format!("INFO: {}", title.trim()); - let body_clean = body.trim().to_string(); - Some((title_clean, body_clean)) - } else if let Some(rest2) = rest.strip_prefix("Downloading ") { - Some(("INFO: Downloading".to_string(), rest2.trim().to_string())) - } else if let Some(rest2) = rest.strip_prefix("Total") { - Some(("INFO: Total".to_string(), rest2.trim().to_string())) - } else { - // Fallback: use first word as title, remainder as body - let mut it = rest.splitn(2, ' '); - let first = it.next().unwrap_or("").trim(); - let remainder = it.next().unwrap_or("").trim(); - if !first.is_empty() { - Some((format!("INFO: {}", first), remainder.to_string())) - } else { - None - } - } - } else { - None - } - }; - match &self.inner { ProgressInner::Noop => eprintln!("{}", line), ProgressInner::Single(s) => { - if let Some((title, body)) = parsed.as_ref() { - s.header.set_message(title.clone()); - s.info.set_message(body.clone()); - } else { - let _ = s._mp.println(line); - } + let _ = s._mp.println(line); } ProgressInner::Multi(m) => { - if let Some((title, body)) = parsed.as_ref() { - m.header.set_message(title.clone()); - m.info.set_message(body.clone()); - } else { - let _ = m._mp.println(line); - } + let _ = m._mp.println(line); } } }