From 19bc919aaa78e6644c8e0c2f47524e262b07d21e Mon Sep 17 00:00:00 2001 From: ruv Date: Wed, 19 Aug 2026 21:52:55 -0400 Subject: [PATCH 1/2] fix(ruvllm-cli): resolve GGUF globs via HF file listing + alias routing (PIR WP0b, #846) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GGUF weight downloads failed two ways (deferred in 946275a61, blocks WP9 #841): 1. get_files_to_download() pushed an unexpanded glob ("*Q4_K_M.gguf") that was sent to HF as a literal filename -> 404. Now the repo's actual file list is fetched (hf-hub ApiRepo::info(), with a curl fallback against GET /api/models//tree/ in the same HF_TOKEN-honoring idiom as the 307-redirect fix) and the quant pattern is matched against real filenames. Multi-part GGUF (…-q4_k_m-00001-of-00003.gguf) is handled — all parts are downloaded in order — because the flagship `qwen` alias (Qwen2.5-14B) only ships Q4_K_M as a 3-part split; failing on multi-part would leave the primary model unusable. Matching is case-insensitive and accepts per-preset spelling variants (f16/fp16). Aux files (tokenizer.json etc.) are filtered by the listing, so GGUF-only repos no longer fail on files they don't have. 2. The `phi` alias maps to microsoft/Phi-4-mini-instruct (safetensors-only), yet the default q4k quant forced the GGUF path. The registry gains an optional gguf_repo twin (bartowski GGUF repos for phi/mistral/llama; all verified live against the HF tree API) and resolve_weights_repo() routes quantized requests there. chat/serve use the same resolution so the cache key matches download's. Repos with no GGUF files and no twin now fail early with the repo's actual file inventory and an actionable hint, instead of a 404 on a glob. ADR-259's "Honest gap" is updated: the 307 redirect was fixed 2026-06-18 (946275a61, PR #590); this closes the remaining GGUF gap. Tests: 11 new unit tests (tree-JSON fixture parsing, multi-part glob expansion + ordering, uppercase/lowercase naming, fp16 variant, no-GGUF and missing-quant error listings, aux filtering, alias-routing decisions). Verified live: `download microsoft/Phi-4-mini-instruct` routes to the twin and fetches the real 2.5GB Q4_K_M; `download microsoft/phi-2` fails early listing available files. Refs #846, #841, ADR-259. Co-Authored-By: claude-flow --- crates/ruvllm-cli/src/commands/chat.rs | 10 +- crates/ruvllm-cli/src/commands/download.rs | 357 ++++++++++++++++-- crates/ruvllm-cli/src/commands/serve.rs | 4 +- crates/ruvllm-cli/src/models.rs | 84 +++++ ...DR-259-ruvllm-darwin-mode-local-mutator.md | 17 +- 5 files changed, 439 insertions(+), 33 deletions(-) diff --git a/crates/ruvllm-cli/src/commands/chat.rs b/crates/ruvllm-cli/src/commands/chat.rs index ca2c7a88d..4a2ad66c1 100644 --- a/crates/ruvllm-cli/src/commands/chat.rs +++ b/crates/ruvllm-cli/src/commands/chat.rs @@ -49,9 +49,11 @@ pub async fn run( draft_model: Option<&str>, speculative_lookahead: usize, ) -> Result<()> { - let model_id = resolve_model_id(model); let quant = QuantPreset::from_str(quantization) .ok_or_else(|| anyhow::anyhow!("Invalid quantization format: {}", quantization))?; + // Resolve to the repo that hosts the weights for this quantization + // (GGUF twin for safetensors-only aliases) — must match `download`'s cache key. + let model_id = crate::models::resolve_weights_repo(model, quant); // Print header print_header(&model_id, system_prompt, max_tokens, temperature); @@ -80,7 +82,11 @@ pub async fn run( "{}", "Loading draft model for speculative decoding...".yellow() ); - let draft = load_model(&resolve_model_id(draft_id), quant, cache_dir)?; + let draft = load_model( + &crate::models::resolve_weights_repo(draft_id, quant), + quant, + cache_dir, + )?; if let Some(info) = draft.model_info() { println!( diff --git a/crates/ruvllm-cli/src/commands/download.rs b/crates/ruvllm-cli/src/commands/download.rs index 30d1df383..1982f5fba 100644 --- a/crates/ruvllm-cli/src/commands/download.rs +++ b/crates/ruvllm-cli/src/commands/download.rs @@ -12,7 +12,7 @@ use hf_hub::{Repo, RepoType}; use indicatif::{ProgressBar, ProgressStyle}; use std::path::{Path, PathBuf}; -use crate::models::{get_model, resolve_model_id, QuantPreset}; +use crate::models::{get_model, resolve_model_id, resolve_weights_repo, QuantPreset}; /// Run the download command pub async fn run( @@ -22,10 +22,14 @@ pub async fn run( revision: Option<&str>, cache_dir: &str, ) -> Result<()> { - let model_id = resolve_model_id(model); let quant = QuantPreset::from_str(quantization) .ok_or_else(|| anyhow::anyhow!("Invalid quantization format: {}", quantization))?; + // Route quantized (GGUF) requests to the repo that actually hosts GGUF + // weights (registry `gguf_repo` twin for safetensors-only aliases). + let base_id = resolve_model_id(model); + let model_id = resolve_weights_repo(model, quant); + println!(); println!( "{} {} ({})", @@ -33,6 +37,13 @@ pub async fn run( model_id, quant ); + if model_id != base_id { + println!( + " {} {} hosts safetensors only; using GGUF twin repo", + "Note:".dimmed(), + base_id + ); + } println!(); // Get model info if available @@ -62,8 +73,12 @@ pub async fn run( api.repo(Repo::new(model_id.clone(), RepoType::Model)) }; - // Determine files to download - let files_to_download = get_files_to_download(&model_id, quant); + // List the repo's actual files and expand the quant pattern against them + // (previously an unexpanded glob like "*Q4_K_M.gguf" was sent literally -> 404). + let remote_files = list_repo_files(&repo, &model_id, revision) + .await + .with_context(|| format!("Failed to list files for {model_id} on HuggingFace"))?; + let files_to_download = get_files_to_download(&model_id, quant, &remote_files)?; // Create cache directory let model_cache_dir = PathBuf::from(cache_dir).join("models").join(&model_id); @@ -74,6 +89,11 @@ pub async fn run( // Download each file for file_name in &files_to_download { let target_path = model_cache_dir.join(file_name); + if let Some(parent) = target_path.parent() { + tokio::fs::create_dir_all(parent) + .await + .context("Failed to create cache subdirectory")?; + } // Check if file exists if target_path.exists() && !force { @@ -189,28 +209,176 @@ fn download_via_curl(model_id: &str, revision: Option<&str>, file_name: &str) -> Ok(out) } -/// Get list of files to download for a model and quantization -fn get_files_to_download(model_id: &str, quant: QuantPreset) -> Vec { - let mut files = vec![ - "tokenizer.json".to_string(), - "tokenizer_config.json".to_string(), - "config.json".to_string(), - ]; +/// List a repo's files via the hf-hub API, falling back to a `curl` of the +/// HF tree endpoint (`GET /api/models//tree/`) — the same fallback +/// idiom as `download_via_curl`, honoring `HF_TOKEN`. +async fn list_repo_files( + repo: &hf_hub::api::tokio::ApiRepo, + model_id: &str, + revision: Option<&str>, +) -> Result> { + match repo.info().await { + Ok(info) => Ok(info.siblings.into_iter().map(|s| s.rfilename).collect()), + Err(e) => list_files_via_curl(model_id, revision) + .with_context(|| format!("hf-hub file listing also failed: {e}")), + } +} - // Add model weights based on quantization - if model_id.contains("GGUF") || quant != QuantPreset::None { - // Look for GGUF files - files.push(format!("*{}", quant.gguf_suffix())); - } else { - // SafeTensors format +/// Fallback file listing via `curl` against the HF tree API. +fn list_files_via_curl(model_id: &str, revision: Option<&str>) -> Result> { + let rev = revision.unwrap_or("main"); + let url = format!("https://huggingface.co/api/models/{model_id}/tree/{rev}?recursive=true"); + let mut args = vec!["-L".to_string(), "--fail".to_string(), "-sS".to_string()]; + if let Ok(token) = std::env::var("HF_TOKEN") { + args.push("-H".to_string()); + args.push(format!("Authorization: Bearer {token}")); + } + args.push(url.clone()); + let output = std::process::Command::new("curl") + .args(&args) + .output() + .with_context(|| format!("curl not available to list {url}"))?; + if !output.status.success() { + anyhow::bail!("curl file listing failed ({}) for {url}", output.status); + } + parse_tree_file_paths(&String::from_utf8_lossy(&output.stdout)) +} + +/// Parse file paths out of the HF tree API JSON (`[{"type":"file","path":...},...]`). +fn parse_tree_file_paths(json: &str) -> Result> { + let entries: serde_json::Value = + serde_json::from_str(json).context("Invalid JSON from HF tree API")?; + let entries = entries + .as_array() + .ok_or_else(|| anyhow::anyhow!("HF tree API did not return an array"))?; + Ok(entries + .iter() + .filter(|e| e.get("type").and_then(|t| t.as_str()) == Some("file")) + .filter_map(|e| e.get("path").and_then(|p| p.as_str()).map(String::from)) + .collect()) +} + +/// Get list of files to download for a model and quantization, resolved +/// against the repo's actual file listing. +fn get_files_to_download( + model_id: &str, + quant: QuantPreset, + remote_files: &[String], +) -> Result> { + // Aux files: only the ones the repo actually has (GGUF repos typically + // ship none — tokenizer/config are embedded in the GGUF itself). + let mut files: Vec = [ + "tokenizer.json", + "tokenizer_config.json", + "config.json", + "special_tokens_map.json", + "generation_config.json", + ] + .iter() + .filter(|f| remote_files.iter().any(|r| r == *f)) + .map(|f| f.to_string()) + .collect(); + + // Model weights + if model_id.to_ascii_uppercase().contains("GGUF") || quant != QuantPreset::None { + files.extend(select_gguf_files(model_id, quant, remote_files)?); + } else if remote_files.iter().any(|f| f == "model.safetensors") { files.push("model.safetensors".to_string()); + } else { + // Sharded safetensors (model-00001-of-000NN.safetensors) + let mut shards: Vec = remote_files + .iter() + .filter(|f| f.starts_with("model-") && f.ends_with(".safetensors")) + .cloned() + .collect(); + if shards.is_empty() { + anyhow::bail!( + "No safetensors weights found in {model_id}.\nAvailable files:\n {}", + remote_files.join("\n ") + ); + } + shards.sort(); + files.extend(shards); } - // Add special tokens and chat template if available - files.push("special_tokens_map.json".to_string()); - files.push("generation_config.json".to_string()); + Ok(files) +} - files +/// Select the GGUF weight file(s) matching `quant` from the repo listing. +/// +/// Handles both single-file (`...-Q4_K_M.gguf`) and multi-part +/// (`...-q4_k_m-00001-of-00003.gguf`) layouts — multi-part is the norm for +/// larger models (e.g. Qwen2.5-14B), so all parts are downloaded in order. +/// Fails with the repo's actual GGUF inventory (or full file list) when +/// nothing matches, instead of 404ing on a glob. +fn select_gguf_files( + model_id: &str, + quant: QuantPreset, + remote_files: &[String], +) -> Result> { + let mut matches: Vec = remote_files + .iter() + .filter(|f| { + quant + .gguf_tags() + .iter() + .any(|tag| matches_gguf_quant(f, tag)) + }) + .cloned() + .collect(); + + if matches.is_empty() { + let ggufs: Vec<&String> = remote_files + .iter() + .filter(|f| f.to_ascii_lowercase().ends_with(".gguf")) + .collect(); + if ggufs.is_empty() { + anyhow::bail!( + "No GGUF files found in {model_id} (requested quantization: {quant}).\n\ + This repo appears to host non-GGUF weights only. Available files:\n {}\n\ + Hint: pass a GGUF repo, or use --quantization none for safetensors.", + remote_files.join("\n ") + ); + } + anyhow::bail!( + "No GGUF file matching quantization {quant} in {model_id}.\n\ + Available GGUF files:\n {}", + ggufs + .iter() + .map(|s| s.as_str()) + .collect::>() + .join("\n ") + ); + } + + // Zero-padded part numbers sort lexically (…-00001-of-00003 first). + matches.sort(); + Ok(matches) +} + +/// True if `file` is a GGUF weight file for the given lowercase quant tag, +/// either single-file (`-.gguf`) or a multi-part shard +/// (`--NNNNN-of-NNNNN.gguf`). Matching is case-insensitive. +fn matches_gguf_quant(file: &str, tag: &str) -> bool { + let f = file.to_ascii_lowercase(); + let Some(stem) = f.strip_suffix(".gguf") else { + return false; + }; + // Single-file: ends with the tag (allow "-tag", ".tag", or bare tag) + if stem == tag || stem.ends_with(&format!("-{tag}")) || stem.ends_with(&format!(".{tag}")) { + return true; + } + // Multi-part: ...--NNNNN-of-NNNNN + if let Some(idx) = stem.rfind(&format!("-{tag}-")) { + let rest = &stem[idx + tag.len() + 2..]; + if let Some((part, total)) = rest.split_once("-of-") { + return !part.is_empty() + && !total.is_empty() + && part.chars().all(|c| c.is_ascii_digit()) + && total.chars().all(|c| c.is_ascii_digit()); + } + } + false } /// Check if a model is already downloaded @@ -243,10 +411,151 @@ pub fn get_model_path(model: &str, cache_dir: &str) -> PathBuf { mod tests { use super::*; + /// HF tree API fixture: Qwen-style GGUF repo (lowercase, multi-part). + const QWEN_TREE_JSON: &str = r#"[ + {"type":"file","oid":"a","size":100,"path":".gitattributes"}, + {"type":"file","oid":"b","size":100,"path":"README.md"}, + {"type":"directory","oid":"c","path":"assets"}, + {"type":"file","oid":"d","size":1,"path":"qwen2.5-14b-instruct-q4_k_m-00002-of-00003.gguf"}, + {"type":"file","oid":"e","size":1,"path":"qwen2.5-14b-instruct-q4_k_m-00001-of-00003.gguf"}, + {"type":"file","oid":"f","size":1,"path":"qwen2.5-14b-instruct-q4_k_m-00003-of-00003.gguf"}, + {"type":"file","oid":"g","size":1,"path":"qwen2.5-14b-instruct-q8_0-00001-of-00004.gguf"}, + {"type":"file","oid":"h","size":1,"path":"qwen2.5-14b-instruct-fp16-00001-of-00008.gguf"} + ]"#; + + fn qwen_files() -> Vec { + parse_tree_file_paths(QWEN_TREE_JSON).unwrap() + } + #[test] - fn test_files_to_download() { - let files = get_files_to_download("test/model", QuantPreset::Q4K); + fn test_parse_tree_file_paths_skips_directories() { + let files = qwen_files(); + assert_eq!(files.len(), 7); + assert!(!files.iter().any(|f| f == "assets")); + assert!(files.contains(&"README.md".to_string())); + } + + #[test] + fn test_parse_tree_rejects_bad_json() { + assert!(parse_tree_file_paths("not json").is_err()); + assert!(parse_tree_file_paths(r#"{"error":"Repo not found"}"#).is_err()); + } + + #[test] + fn test_glob_expansion_multi_part_sorted() { + // The old code pushed the literal "*Q4_K_M.gguf" -> 404. The matcher + // must instead select the real (multi-part, lowercase) filenames. + let files = select_gguf_files( + "Qwen/Qwen2.5-14B-Instruct-GGUF", + QuantPreset::Q4K, + &qwen_files(), + ) + .unwrap(); + assert_eq!( + files, + vec![ + "qwen2.5-14b-instruct-q4_k_m-00001-of-00003.gguf", + "qwen2.5-14b-instruct-q4_k_m-00002-of-00003.gguf", + "qwen2.5-14b-instruct-q4_k_m-00003-of-00003.gguf", + ] + ); + } + + #[test] + fn test_glob_expansion_single_file_uppercase() { + // bartowski-style single-file uppercase naming + let files = vec![ + "README.md".to_string(), + "microsoft_Phi-4-mini-instruct-Q4_K_M.gguf".to_string(), + "microsoft_Phi-4-mini-instruct-Q8_0.gguf".to_string(), + ]; + let selected = select_gguf_files( + "bartowski/microsoft_Phi-4-mini-instruct-GGUF", + QuantPreset::Q4K, + &files, + ) + .unwrap(); + assert_eq!(selected, vec!["microsoft_Phi-4-mini-instruct-Q4_K_M.gguf"]); + } + + #[test] + fn test_f16_matches_fp16_spelling() { + let selected = select_gguf_files( + "Qwen/Qwen2.5-14B-Instruct-GGUF", + QuantPreset::F16, + &qwen_files(), + ) + .unwrap(); + assert_eq!( + selected, + vec!["qwen2.5-14b-instruct-fp16-00001-of-00008.gguf"] + ); + } + + #[test] + fn test_no_gguf_in_repo_fails_with_available_files() { + // Safetensors-only repo (the phi/microsoft case) must fail early with + // an actionable listing, not a 404 on a glob. + let files = vec![ + "config.json".to_string(), + "model-00001-of-00002.safetensors".to_string(), + "model-00002-of-00002.safetensors".to_string(), + ]; + let err = select_gguf_files("microsoft/Phi-4-mini-instruct", QuantPreset::Q4K, &files) + .unwrap_err() + .to_string(); + assert!(err.contains("No GGUF files found")); + assert!(err.contains("model-00001-of-00002.safetensors")); + assert!(err.contains("--quantization none")); + } + + #[test] + fn test_missing_quant_lists_available_ggufs() { + let files = vec!["m-q8_0.gguf".to_string()]; + let err = select_gguf_files("x/y-GGUF", QuantPreset::Q4K, &files) + .unwrap_err() + .to_string(); + assert!(err.contains("Q4_K_M")); + assert!(err.contains("m-q8_0.gguf")); + } + + #[test] + fn test_matches_gguf_quant_shapes() { + assert!(matches_gguf_quant("model-Q4_K_M.gguf", "q4_k_m")); + assert!(matches_gguf_quant("model.q4_k_m.gguf", "q4_k_m")); + assert!(matches_gguf_quant("m-q4_k_m-00001-of-00003.gguf", "q4_k_m")); + // No false positives on other quants or non-gguf files + assert!(!matches_gguf_quant("model-Q4_K_S.gguf", "q4_k_m")); + assert!(!matches_gguf_quant("model-q4_k_m.safetensors", "q4_k_m")); + assert!(!matches_gguf_quant("m-q4_k_m-partial-of-x.gguf", "q4_k_m")); + } + + #[test] + fn test_files_to_download_filters_aux_by_listing() { + // GGUF repos ship no tokenizer.json/config.json — must not request them. + let files = get_files_to_download( + "Qwen/Qwen2.5-14B-Instruct-GGUF", + QuantPreset::Q4K, + &qwen_files(), + ) + .unwrap(); + assert!(!files.contains(&"tokenizer.json".to_string())); + assert!( + files.iter().all(|f| !f.contains('*')), + "no unexpanded globs" + ); + assert_eq!(files.iter().filter(|f| f.ends_with(".gguf")).count(), 3); + } + + #[test] + fn test_files_to_download_safetensors_path() { + let remote = vec![ + "tokenizer.json".to_string(), + "config.json".to_string(), + "model.safetensors".to_string(), + ]; + let files = get_files_to_download("test/model", QuantPreset::None, &remote).unwrap(); assert!(files.contains(&"tokenizer.json".to_string())); - assert!(files.iter().any(|f| f.contains("Q4_K_M"))); + assert!(files.contains(&"model.safetensors".to_string())); } } diff --git a/crates/ruvllm-cli/src/commands/serve.rs b/crates/ruvllm-cli/src/commands/serve.rs index e4b310025..0a638b6d4 100644 --- a/crates/ruvllm-cli/src/commands/serve.rs +++ b/crates/ruvllm-cli/src/commands/serve.rs @@ -51,9 +51,11 @@ pub async fn run( quantization: &str, cache_dir: &str, ) -> Result<()> { - let model_id = resolve_model_id(model); let quant = QuantPreset::from_str(quantization) .ok_or_else(|| anyhow::anyhow!("Invalid quantization format: {}", quantization))?; + // Resolve to the repo that hosts the weights for this quantization + // (GGUF twin for safetensors-only aliases) — must match `download`'s cache key. + let model_id = crate::models::resolve_weights_repo(model, quant); println!(); println!("{}", style("RuvLLM Inference Server").bold().cyan()); diff --git a/crates/ruvllm-cli/src/models.rs b/crates/ruvllm-cli/src/models.rs index cc0121a58..15b397857 100644 --- a/crates/ruvllm-cli/src/models.rs +++ b/crates/ruvllm-cli/src/models.rs @@ -29,6 +29,12 @@ pub struct ModelDefinition { pub context_length: usize, /// Notes about the model pub notes: String, + /// GGUF "twin" repo for models whose `hf_id` only hosts safetensors. + /// When a quantized (GGUF) download is requested, weights are resolved + /// from this repo instead of `hf_id`. `None` means `hf_id` itself hosts + /// the GGUF files (or no GGUF twin is known). + #[serde(default)] + pub gguf_repo: Option, } /// Get all recommended models @@ -46,6 +52,7 @@ pub fn get_recommended_models() -> Vec { memory_gb: 9.5, context_length: 32768, notes: "Best overall performance for reasoning tasks on M4 Pro".to_string(), + gguf_repo: None, }, // Fast instruction following ModelDefinition { @@ -59,6 +66,7 @@ pub fn get_recommended_models() -> Vec { memory_gb: 4.5, context_length: 32768, notes: "Excellent speed/quality tradeoff with sliding window attention".to_string(), + gguf_repo: Some("bartowski/Mistral-7B-Instruct-v0.3-GGUF".to_string()), }, // Tiny/testing model ModelDefinition { @@ -72,6 +80,7 @@ pub fn get_recommended_models() -> Vec { memory_gb: 2.5, context_length: 16384, notes: "Surprisingly capable for its size, fast inference".to_string(), + gguf_repo: Some("bartowski/microsoft_Phi-4-mini-instruct-GGUF".to_string()), }, // Tool use model ModelDefinition { @@ -85,6 +94,7 @@ pub fn get_recommended_models() -> Vec { memory_gb: 2.2, context_length: 131072, notes: "Optimized for tool use and function calling".to_string(), + gguf_repo: Some("bartowski/Llama-3.2-3B-Instruct-GGUF".to_string()), }, // Code-specific model ModelDefinition { @@ -98,6 +108,7 @@ pub fn get_recommended_models() -> Vec { memory_gb: 4.8, context_length: 32768, notes: "Specialized for coding tasks, excellent at code completion".to_string(), + gguf_repo: None, }, // Large reasoning model (for when you have the memory) ModelDefinition { @@ -111,6 +122,7 @@ pub fn get_recommended_models() -> Vec { memory_gb: 20.0, context_length: 32768, notes: "Requires significant memory, but provides best quality".to_string(), + gguf_repo: None, }, ] } @@ -147,6 +159,27 @@ pub fn resolve_model_id(identifier: &str) -> String { } } +/// Resolve the repo that hosts the *weights* for `identifier` at `quant`. +/// +/// A quantized (GGUF) request against an alias whose `hf_id` is a +/// safetensors-only repo is routed to the registry's `gguf_repo` twin when one +/// is defined. Repos that already host GGUF files (id contains "GGUF") and +/// unquantized requests resolve as before. Unknown identifiers pass through +/// unchanged — the download command then validates against the actual HF file +/// listing and fails with the available files rather than a 404 on a glob. +pub fn resolve_weights_repo(identifier: &str, quant: QuantPreset) -> String { + let base = resolve_model_id(identifier); + if quant == QuantPreset::None || base.to_ascii_uppercase().contains("GGUF") { + return base; + } + if let Some(model) = get_model(identifier) { + if let Some(gguf_repo) = model.gguf_repo { + return gguf_repo; + } + } + base +} + /// Get model aliases map pub fn get_aliases() -> HashMap { get_recommended_models() @@ -190,6 +223,18 @@ impl QuantPreset { } } + /// Quant tags to match against actual GGUF filenames (lowercase). + /// Repos vary in spelling (e.g. Qwen ships `fp16`, bartowski ships `f16`), + /// so each preset may accept several tags. + pub fn gguf_tags(&self) -> &'static [&'static str] { + match self { + Self::Q4K => &["q4_k_m"], + Self::Q8 => &["q8_0"], + Self::F16 => &["f16", "fp16"], + Self::None => &["f32", "fp32"], + } + } + /// Get bytes per weight pub fn bytes_per_weight(&self) -> f32 { match self { @@ -241,4 +286,43 @@ mod tests { assert_eq!(QuantPreset::from_str("q4k"), Some(QuantPreset::Q4K)); assert_eq!(QuantPreset::Q4K.bytes_per_weight(), 0.5); } + + #[test] + fn test_weights_repo_gguf_twin_for_safetensors_alias() { + // `phi` -> microsoft/Phi-4-mini-instruct hosts safetensors only; a + // quantized request must route to the GGUF twin, not the base repo. + let repo = resolve_weights_repo("phi", QuantPreset::Q4K); + assert_eq!(repo, "bartowski/microsoft_Phi-4-mini-instruct-GGUF"); + assert!(repo.contains("GGUF")); + } + + #[test] + fn test_weights_repo_unquantized_stays_on_base_repo() { + assert_eq!( + resolve_weights_repo("phi", QuantPreset::None), + "microsoft/Phi-4-mini-instruct" + ); + } + + #[test] + fn test_weights_repo_native_gguf_alias_unchanged() { + assert_eq!( + resolve_weights_repo("qwen", QuantPreset::Q4K), + "Qwen/Qwen2.5-14B-Instruct-GGUF" + ); + } + + #[test] + fn test_weights_repo_unknown_id_passes_through() { + assert_eq!( + resolve_weights_repo("custom/model", QuantPreset::Q4K), + "custom/model" + ); + } + + #[test] + fn test_gguf_tags_cover_repo_spelling_variants() { + assert!(QuantPreset::F16.gguf_tags().contains(&"fp16")); + assert_eq!(QuantPreset::Q4K.gguf_tags(), &["q4_k_m"]); + } } diff --git a/docs/adr/ADR-259-ruvllm-darwin-mode-local-mutator.md b/docs/adr/ADR-259-ruvllm-darwin-mode-local-mutator.md index edc8a764a..774afdbf7 100644 --- a/docs/adr/ADR-259-ruvllm-darwin-mode-local-mutator.md +++ b/docs/adr/ADR-259-ruvllm-darwin-mode-local-mutator.md @@ -1,6 +1,6 @@ # ADR-259: ruvllm as Local Mutator Backend for Darwin Mode -**Status:** Implemented (code + unit tests + CLI; live-serve e2e blocked by a ruvllm download bug — see Implementation status) +**Status:** Implemented (code + unit tests + CLI; the download-path bugs that blocked the live-serve e2e are fixed — see Implementation status) ## Implementation status (2026-06-18) @@ -11,11 +11,16 @@ Implemented in `agent-harness-generator` (`@metaharness/darwin`): - `__tests__/ruvllm-mutator.test.ts` — 4 tests vs a real `node:http` mock (success, fence-strip, unreachable→no-op, malformed→no-op). Full darwin suite **354/354** green. -**Honest gap:** the live-serve e2e (evolve against a real local model) is **blocked by a ruvllm -2.1.0 download bug** — `ruvllm download phi` fails on `tokenizer_config.json` (the file is served -via an HTTP 307 redirect that ruvllm does not follow; `curl -L` fetches it fine). So the HTTP -*contract* is verified by unit tests, but an end-to-end run against a served model is pending a -ruvllm fix (or manual model placement). Recommend fixing the redirect-follow in `ruvllm download`. +**Honest gap (updated):** the live-serve e2e (evolve against a real local model) was blocked by +two `ruvllm download` bugs, both now fixed: +1. **HTTP 307 redirect on aux files** (`tokenizer_config.json` etc.) — **fixed 2026-06-18** in + commit `946275a61` (PR #590) via a redirect-following `curl -L` fallback. +2. **GGUF weight downloads** — the remaining gap after PR #590: `get_files_to_download()` sent an + unexpanded glob (`*Q4_K_M.gguf`) literally (404), and the GGUF alias resolved to the + safetensors repo. Fixed (PIR WP0b, issue #846) by expanding the pattern against the real + HuggingFace file listing (`/api/models//tree/`, multi-part GGUF included) and by + routing quantized requests to a registry-defined GGUF twin repo. +With both fixed, an end-to-end run against a served model is unblocked. **Value note (ADR-087):** the mutator is not the quality lever — deterministic and frontier-LLM mutators both hit the 0.985 scorer ceiling. RuvllmMutator's benefit is *operational* (fully local, From 8ff3e7c8f79e00a76ca4f6a692f31ce8a43bb6fb Mon Sep 17 00:00:00 2001 From: ruv Date: Thu, 20 Aug 2026 08:40:40 -0400 Subject: [PATCH 2/2] fix(ruvllm-cli): keep HF_TOKEN off curl argv + guard remote paths (PR #860 security review) Addresses both Phase-4 security findings on the GGUF download curl fallbacks (crates/ruvllm-cli/src/commands/download.rs): - MED (CWE-214): the Authorization: Bearer header was passed as a curl -H argument, leaving HF_TOKEN world-readable via ps / /proc//cmdline for the curl process lifetime. Both call sites (download_via_curl and list_files_via_curl) now pass the header through a curl config fed on stdin (--config -), built by curl_auth_config() which escapes quotes, backslashes, and CR/LF so a hostile token value cannot inject extra config directives. Behavior when HF_TOKEN is unset is unchanged (no config, no --config flag). - LOW: remote-controlled file names from the HF tree/siblings listing were joined into the cache path unchecked. validate_remote_file_name() rejects empty names and any non-Normal component (absolute, .., ., prefixes) before the join, and ensure_under_cache_dir() verifies the canonicalized parent still resolves under the model cache dir after create_dir_all. Tests: 5 new unit tests (hostile-name rejection incl. "../x", "/abs", "a/../../x"; containment check; curl config content + escaping); all 31 pass, clippy and fmt clean. Manually verified `ruvllm download hf-internal-testing/tiny-random-gpt2 --quantization none` end-to-end and that --config-on-stdin delivers the Authorization header. Co-Authored-By: claude-flow Claude-Session: https://claude.ai/code/session_012Jib2gQyJpqCoo2xYAbb4X --- crates/ruvllm-cli/src/commands/download.rs | 194 +++++++++++++++++++-- 1 file changed, 180 insertions(+), 14 deletions(-) diff --git a/crates/ruvllm-cli/src/commands/download.rs b/crates/ruvllm-cli/src/commands/download.rs index 1982f5fba..bccc472c5 100644 --- a/crates/ruvllm-cli/src/commands/download.rs +++ b/crates/ruvllm-cli/src/commands/download.rs @@ -88,11 +88,19 @@ pub async fn run( // Download each file for file_name in &files_to_download { + // Defense-in-depth: `file_name` originates from the repo's own file + // listing (HF tree API / siblings) and is attacker-controlled for a + // malicious repo — reject traversal before joining into the cache. + validate_remote_file_name(file_name) + .with_context(|| format!("Unsafe file path in {model_id} listing"))?; let target_path = model_cache_dir.join(file_name); if let Some(parent) = target_path.parent() { tokio::fs::create_dir_all(parent) .await .context("Failed to create cache subdirectory")?; + ensure_under_cache_dir(&model_cache_dir, parent).with_context(|| { + format!("Refusing to write {file_name} outside the model cache directory") + })?; } // Check if file exists @@ -194,17 +202,16 @@ fn download_via_curl(model_id: &str, revision: Option<&str>, file_name: &str) -> "-o".to_string(), out.to_string_lossy().to_string(), ]; - if let Ok(token) = std::env::var("HF_TOKEN") { - args.push("-H".to_string()); - args.push(format!("Authorization: Bearer {token}")); + let auth_config = hf_token_curl_config(); + if auth_config.is_some() { + args.push("--config".to_string()); + args.push("-".to_string()); } args.push(url.clone()); - let status = std::process::Command::new("curl") - .args(&args) - .status() + let output = run_curl(&args, auth_config.as_deref()) .with_context(|| format!("curl not available to fetch {url}"))?; - if !status.success() { - anyhow::bail!("curl fallback failed ({status}) for {url}"); + if !output.status.success() { + anyhow::bail!("curl fallback failed ({}) for {url}", output.status); } Ok(out) } @@ -229,14 +236,13 @@ fn list_files_via_curl(model_id: &str, revision: Option<&str>) -> Result) -> Result/cmdline` (CWE-214). +fn hf_token_curl_config() -> Option { + std::env::var("HF_TOKEN") + .ok() + .filter(|t| !t.is_empty()) + .map(|t| curl_auth_config(&t)) +} + +/// Render an Authorization header as a curl config line, escaping the +/// characters curl's double-quoted config strings treat specially so a +/// hostile/odd token value cannot inject additional config directives. +fn curl_auth_config(token: &str) -> String { + let escaped = token + .replace('\\', "\\\\") + .replace('"', "\\\"") + .replace('\n', "\\n") + .replace('\r', "\\r"); + format!("header = \"Authorization: Bearer {escaped}\"\n") +} + +/// Run `curl` with `args`, feeding `stdin_config` (a curl config carrying the +/// auth header) on stdin when present. Stderr is inherited so `-sS` errors +/// stay visible; stdout is captured for callers that parse it. +fn run_curl(args: &[String], stdin_config: Option<&str>) -> Result { + use std::io::Write as _; + use std::process::{Command, Stdio}; + + let mut cmd = Command::new("curl"); + cmd.args(args) + .stdin(if stdin_config.is_some() { + Stdio::piped() + } else { + Stdio::null() + }) + .stdout(Stdio::piped()) + .stderr(Stdio::inherit()); + let mut child = cmd.spawn().context("failed to spawn curl")?; + if let Some(config) = stdin_config { + child + .stdin + .take() + .expect("stdin is piped when a config is supplied") + .write_all(config.as_bytes()) + .context("failed to write curl config to stdin")?; + // stdin handle dropped here -> EOF for `--config -` + } + child.wait_with_output().context("failed to wait for curl") +} + +/// Defense-in-depth guard for file names taken from a repo's own listing +/// (HF tree API `path` / `siblings[].rfilename`): reject empty names and any +/// component that is not a plain path segment (absolute paths, `..`, `.`, +/// Windows prefixes) before the name is joined into the cache directory. +fn validate_remote_file_name(file_name: &str) -> Result<()> { + use std::path::Component; + + if file_name.is_empty() { + anyhow::bail!("empty file name in repo listing"); + } + if Path::new(file_name) + .components() + .any(|c| !matches!(c, Component::Normal(_))) + { + anyhow::bail!("suspicious file path in repo listing: {file_name:?}"); + } + Ok(()) +} + +/// Verify that `target_parent` (already created) canonicalizes to a location +/// under the canonicalized model cache dir — catches anything the component +/// check missed (e.g. symlinked subdirectories escaping the cache). +fn ensure_under_cache_dir(cache_root: &Path, target_parent: &Path) -> Result<()> { + let root = cache_root + .canonicalize() + .context("failed to canonicalize model cache directory")?; + let parent = target_parent + .canonicalize() + .context("failed to canonicalize download target directory")?; + if !parent.starts_with(&root) { + anyhow::bail!( + "download target {} escapes model cache directory {}", + parent.display(), + root.display() + ); + } + Ok(()) +} + /// Parse file paths out of the HF tree API JSON (`[{"type":"file","path":...},...]`). fn parse_tree_file_paths(json: &str) -> Result> { let entries: serde_json::Value = @@ -547,6 +644,75 @@ mod tests { assert_eq!(files.iter().filter(|f| f.ends_with(".gguf")).count(), 3); } + #[test] + fn test_validate_remote_file_name_rejects_hostile_names() { + for hostile in [ + "../x", + "/abs", + "a/../../x", + "", + "..", + "./x/../y", + "/etc/passwd", + ] { + assert!( + validate_remote_file_name(hostile).is_err(), + "should reject {hostile:?}" + ); + } + } + + #[test] + fn test_validate_remote_file_name_accepts_normal_names() { + for ok in [ + "model-Q4_K_M.gguf", + "tokenizer.json", + "subdir/model-00001-of-00003.gguf", + "a/b/c.txt", + ] { + assert!( + validate_remote_file_name(ok).is_ok(), + "should accept {ok:?}" + ); + } + } + + #[test] + fn test_ensure_under_cache_dir_containment() { + let base = std::env::temp_dir().join(format!("ruvllm-guard-test-{}", std::process::id())); + let root = base.join("cache"); + let inside = root.join("sub"); + let outside = base.join("outside"); + std::fs::create_dir_all(&inside).unwrap(); + std::fs::create_dir_all(&outside).unwrap(); + + assert!(ensure_under_cache_dir(&root, &inside).is_ok()); + assert!(ensure_under_cache_dir(&root, &root).is_ok()); + assert!(ensure_under_cache_dir(&root, &outside).is_err()); + + std::fs::remove_dir_all(&base).ok(); + } + + #[test] + fn test_curl_auth_config_keeps_token_in_header_line() { + let cfg = curl_auth_config("hf_abc123"); + assert_eq!(cfg, "header = \"Authorization: Bearer hf_abc123\"\n"); + } + + #[test] + fn test_curl_auth_config_escapes_special_characters() { + // Quotes, backslashes, and newlines must not break out of the quoted + // config string (which would allow injecting extra curl directives). + let cfg = curl_auth_config("a\"b\\c\nd\re"); + assert_eq!( + cfg, + "header = \"Authorization: Bearer a\\\"b\\\\c\\nd\\re\"\n" + ); + // Exactly one config line regardless of token content. + assert_eq!(cfg.matches('\n').count(), 1); + assert!(cfg.ends_with('\n')); + } + #[test] fn test_files_to_download_safetensors_path() { let remote = vec![