From 18a4487f973e37df968e60868bf3046ad8abcd70 Mon Sep 17 00:00:00 2001 From: Abhishek Date: Sun, 23 Aug 2026 16:55:49 +0530 Subject: [PATCH 1/3] [travsr-cli] Embed capacity and progress honesty; add reindex --rebuild F1: an interactive "Full" CPU choice now resolves to an explicit 100 percent, so it overrides ambient embed.capacity config instead of deferring to it. The parse is split into a pure parse_cpu_choice so it is testable without a TTY. F4: clamp the three embed status percentages to 100 via pct_display; the raw done/total counts stay honest. ETA: remove the projected ETA everywhere it appeared (embed status fmt_eta and the live reindex bar) and keep measured elapsed. Removes the now-unused eta helper, its warm-up constants, and the eta-only tests. Add embed reindex --rebuild, which invokes the sidecar --reembed full-rebuild mode through the existing banner, progress, and cancel plumbing. --- crates/travsr-cli/src/embed.rs | 213 +++++++++++++++++++----------- crates/travsr-cli/src/progress.rs | 87 ++++-------- 2 files changed, 165 insertions(+), 135 deletions(-) diff --git a/crates/travsr-cli/src/embed.rs b/crates/travsr-cli/src/embed.rs index d0e4c81f..d10f14ad 100644 --- a/crates/travsr-cli/src/embed.rs +++ b/crates/travsr-cli/src/embed.rs @@ -120,6 +120,13 @@ pub enum EmbedCommand { /// nearest git root). #[arg(long)] db: Option, + /// Rebuild every embedding from scratch, replacing the whole index. + /// + /// Use this after switching the embedding model, when older embeddings + /// may be inconsistent with newer ones. Slower than a normal reindex, + /// which only fills in what is missing. + #[arg(long)] + rebuild: bool, /// Only embed symbol nodes with shell_number >= N (Phase 1 high-centrality pass). /// Omit to embed all pending nodes. #[arg(long)] @@ -230,6 +237,7 @@ pub fn run(cmd: EmbedCommand) -> Result<()> { } EmbedCommand::Reindex { db, + rebuild, phase1, capacity, jobs, @@ -240,7 +248,7 @@ pub fn run(cmd: EmbedCommand) -> Result<()> { max_workers: jobs, priority: priority.map(Into::into), }; - cmd_reindex(db, phase1, overrides) + cmd_reindex(db, phase1, rebuild, overrides) } EmbedCommand::Reconfigure { db, @@ -595,7 +603,6 @@ fn cmd_init( /// "Full" default). Only called on an interactive terminal. fn prompt_cpu_budget() -> Result> { use std::io::Write as _; - use travsr_plugin_host::Capacity; let cores = std::thread::available_parallelism() .map(|n| n.get()) @@ -617,28 +624,49 @@ fn prompt_cpu_budget() -> Result> { let mut input = String::new(); std::io::stdin().read_line(&mut input)?; - match input.trim() { - "" | "1" => Ok(None), // Full → defer to config/default (100%) - "2" => Ok(Some(Capacity::Percent(50))), - "3" => Ok(Some(Capacity::Percent(25))), - "4" => Ok(Some(Capacity::Auto)), - "5" => { - print!(" Percent (1-100): "); - std::io::stdout().flush()?; - let mut pct = String::new(); - std::io::stdin().read_line(&mut pct)?; - match Capacity::parse(pct.trim()) { - Some(c) => Ok(Some(c)), - None => { - println!(" (not a valid percent, using Full)"); - Ok(None) - } - } - } - _ => { - println!(" (unrecognised, using Full)"); - Ok(None) - } + let choice = input.trim().to_string(); + + // The custom (percent) sub-prompt is the only branch that needs a second + // line of input; read it here so the parse itself stays pure/testable. + let custom = if choice == "5" { + print!(" Percent (1-100): "); + std::io::stdout().flush()?; + let mut pct = String::new(); + std::io::stdin().read_line(&mut pct)?; + pct.trim().to_string() + } else { + String::new() + }; + + let parsed = parse_cpu_choice(&choice, &custom); + match choice.as_str() { + "" | "1" | "2" | "3" | "4" => {} + "5" if parsed.is_none() => println!(" (not a valid percent, using Full)"), + "5" => {} + _ => println!(" (unrecognised, using Full)"), + } + Ok(parsed) +} + +/// Pure parse of the CPU-budget menu selection, split out of +/// [`prompt_cpu_budget`] so it is testable without a TTY. `choice` is the +/// top-level answer; `custom` is the percent entered for the "5" (Custom) +/// path and is ignored otherwise. +/// +/// `None` means "user made no explicit choice" — fall through to +/// config/env/default. An explicit Full ("1"/Enter) returns `Percent(100)` so +/// the just-made interactive selection overrides ambient global config (F1); +/// returning `None` there is the bug, because `embed.capacity = auto` in config +/// would then silently win over the user's Full choice. +fn parse_cpu_choice(choice: &str, custom: &str) -> Option { + use travsr_plugin_host::Capacity; + match choice { + "" | "1" => Some(Capacity::Percent(100)), + "2" => Some(Capacity::Percent(50)), + "3" => Some(Capacity::Percent(25)), + "4" => Some(Capacity::Auto), + "5" => Capacity::parse(custom), + _ => None, } } @@ -1020,7 +1048,7 @@ fn reindex_after_init( let gov = travsr_plugin_host::resolve_governance_for_db(db_path, overrides); println!(" {}", reindex_banner(workers, &gov)); - run_reindex_with_progress(db_path, None, overrides)?; + run_reindex_with_progress(db_path, None, false, overrides)?; let embedded = query_embed_stats(db_path, &backend.id) .map(|s| s.stats.embedded) @@ -1460,10 +1488,11 @@ fn reindex_banner(workers: usize, gov: &travsr_plugin_host::EmbedGovernance) -> fn cmd_reindex( db_override: Option, phase1: Option, + rebuild: bool, overrides: travsr_plugin_host::EmbedOverrides, ) -> Result<()> { let db_path = resolve_graph_db(db_override)?; - run_reindex_locked(&db_path, phase1, &overrides) + run_reindex_locked(&db_path, phase1, rebuild, &overrides) } /// The locked reindex core: `embed.lock` guard, embed-text regen, resolved-budget @@ -1473,6 +1502,9 @@ fn cmd_reindex( fn run_reindex_locked( db_path: &Path, phase1: Option, + // `--rebuild`: clear + re-embed every node (a full rebuild), not just fill + // in what is missing. Always false on the reconfigure (WS4) path. + reembed: bool, overrides: &travsr_plugin_host::EmbedOverrides, ) -> Result<()> { // UX-015: fail fast when there is no backend to reindex with, before any @@ -1518,7 +1550,7 @@ fn run_reindex_locked( install_reindex_cancel_handler(db_path); // RFC-020: delegate to the parallel orchestrator with a live progress bar. - run_reindex_with_progress(db_path, phase1, overrides)?; + run_reindex_with_progress(db_path, phase1, reembed, overrides)?; if REINDEX_CANCELLED.load(Ordering::SeqCst) { println!( @@ -1653,7 +1685,7 @@ pub(crate) fn trigger_reindex_now( // A fresh reindex with the new budget. Blocks on embed.lock until the // cancelled run releases; resumes incrementally (no re-embed). - let result = run_reindex_locked(db_path, None, overrides); + let result = run_reindex_locked(db_path, None, false, overrides); if paused_daemon { if let Err(e) = crate::daemon_client::send_daemon_command( @@ -1678,6 +1710,7 @@ pub(crate) fn trigger_reindex_now( fn run_reindex_with_progress( db_path: &Path, phase1: Option, + reembed: bool, overrides: &travsr_plugin_host::EmbedOverrides, ) -> Result<()> { // Same fallback as `resolve_backend` (embed_catalog.rs): a repo indexed @@ -1715,7 +1748,8 @@ fn run_reindex_with_progress( }) }); - let result = travsr_plugin_host::run_parallel_reindex_blocking(db_path, phase1, overrides); + let result = + travsr_plugin_host::run_parallel_reindex_blocking(db_path, phase1, reembed, overrides); done_flag.store(true, Ordering::Relaxed); if let Some(m) = monitor { @@ -1921,18 +1955,16 @@ fn query_embed_stats(db_path: &std::path::Path, model_id: &str) -> Result String { - if nodes_per_sec < 1.0 || remaining == 0 { - return String::new(); - } - let secs = (remaining as f64 / nodes_per_sec).round() as u64; - if secs < 60 { - format!("~{secs}s remaining") - } else if secs < 3600 { - format!("~{}m remaining", secs / 60) - } else { - format!("~{}h {}m remaining", secs / 3600, (secs % 3600) / 60) +/// Display-only completion percentage, clamped to 100 (F4). The denominator +/// (`total`) is a tier snapshot that can lag the live `done` count as nodes +/// shift tiers, so `done > total` is possible; the raw `done/total` counts stay +/// honest, but the percentage must never read above 100%. An empty tier +/// (`total == 0`) reports 100% (nothing left to do). +fn pct_display(done: u64, total: u64) -> f64 { + if total == 0 { + return 100.0; } + ((done as f64 / total as f64) * 100.0).min(100.0) } fn fmt_count(n: u64) -> String { @@ -2144,16 +2176,7 @@ fn cmd_status() -> Result<()> { return Ok(()); } - let pct = stats.embedded as f64 / stats.total_symbols as f64 * 100.0; - // Phase 1 throughput ~400 nodes/sec (k8s: 109k nodes / 4.5 min). - // Phase 2 is background and ~10× slower on a loaded machine. - let nodes_per_sec: f64 = if stats.phase1_done < stats.phase1_total { - 400.0 - } else { - 40.0 - }; - let remaining = stats.total_symbols.saturating_sub(stats.embedded); - let eta = fmt_eta(remaining, nodes_per_sec); + let pct = pct_display(stats.embedded, stats.total_symbols); let pal = Palette::for_stream(std::io::stdout().is_terminal()); let bar = crate::progress::bar_of_width(pal, stats.embedded, stats.total_symbols, 36); @@ -2163,52 +2186,44 @@ fn cmd_status() -> Result<()> { fmt_count(stats.embedded), pct ); - if eta.is_empty() { + if stats.embedded >= stats.total_symbols { println!("{bar} done"); } else { - println!("{bar} {eta}"); + println!("{bar}"); } // ── per-phase breakdown ─────────────────────────────────────────────────── println!(); - let p1_pct = if stats.phase1_total > 0 { - stats.phase1_done as f64 / stats.phase1_total as f64 * 100.0 + let p1_pct = pct_display(stats.phase1_done, stats.phase1_total); + let p1_bar = crate::progress::bar_of_width(pal, stats.phase1_done, stats.phase1_total, 24); + let p1_done_marker = if stats.phase1_done >= stats.phase1_total { + " \u{2713} complete" } else { - 100.0 + "" }; - let p1_bar = crate::progress::bar_of_width(pal, stats.phase1_done, stats.phase1_total, 24); - let p1_eta = fmt_eta(stats.phase1_total.saturating_sub(stats.phase1_done), 400.0); println!( - "core symbols (centrality \u{2265}{threshold}) {} {}/{} ({:.0}%) {}", + "core symbols (centrality \u{2265}{threshold}) {} {}/{} ({:.0}%){}", p1_bar, fmt_count(stats.phase1_done), fmt_count(stats.phase1_total), p1_pct, - if p1_eta.is_empty() { - "\u{2713} complete".to_string() - } else { - p1_eta - }, + p1_done_marker, ); - let p2_pct = if stats.phase2_total > 0 { - stats.phase2_done as f64 / stats.phase2_total as f64 * 100.0 + let p2_pct = pct_display(stats.phase2_done, stats.phase2_total); + let p2_bar = crate::progress::bar_of_width(pal, stats.phase2_done, stats.phase2_total, 24); + let p2_done_marker = if stats.phase2_done >= stats.phase2_total { + " \u{2713} complete" } else { - 100.0 + "" }; - let p2_bar = crate::progress::bar_of_width(pal, stats.phase2_done, stats.phase2_total, 24); - let p2_eta = fmt_eta(stats.phase2_total.saturating_sub(stats.phase2_done), 40.0); println!( - "other symbols (centrality <{threshold}) {} {}/{} ({:.0}%) {}", + "other symbols (centrality <{threshold}) {} {}/{} ({:.0}%){}", p2_bar, fmt_count(stats.phase2_done), fmt_count(stats.phase2_total), p2_pct, - if p2_eta.is_empty() { - "\u{2713} complete".to_string() - } else { - p2_eta - }, + p2_done_marker, ); // ── HNSW index ──────────────────────────────────────────────────────────── @@ -2235,7 +2250,7 @@ fn cmd_status() -> Result<()> { "hint: no symbols embedded yet, the daemon starts embedding after semantic indexing." ); println!(" If the daemon is not running: travsr daemon start"); - } else if remaining > 0 { + } else if stats.embedded < stats.total_symbols { println!(); println!("hint: embedding is running in the background via the daemon."); println!(" Run `travsr embed status` again in a few minutes to see progress."); @@ -2775,3 +2790,51 @@ mod issue_755_tests { ); } } + +/// Track A: the CPU-budget menu parse (F1) and the clamped status percentage +/// (F4). Both are pure so they are tested without a TTY. +#[cfg(test)] +mod embed_ux_tests { + use super::*; + use travsr_plugin_host::Capacity; + + /// F1: an explicit Full ("1"/Enter) must resolve to `Percent(100)`, not + /// `None`. `None` defers to config, so a `None` here lets `embed.capacity = + /// auto` in global config silently override the just-made interactive choice + /// — the exact bug F1 fixes. + #[test] + fn full_choice_is_explicit_hundred_percent() { + assert_eq!(parse_cpu_choice("", ""), Some(Capacity::Percent(100))); + assert_eq!(parse_cpu_choice("1", ""), Some(Capacity::Percent(100))); + } + + #[test] + fn preset_choices_map_to_their_budgets() { + assert_eq!(parse_cpu_choice("2", ""), Some(Capacity::Percent(50))); + assert_eq!(parse_cpu_choice("3", ""), Some(Capacity::Percent(25))); + assert_eq!(parse_cpu_choice("4", ""), Some(Capacity::Auto)); + } + + #[test] + fn custom_choice_parses_its_percent() { + assert_eq!(parse_cpu_choice("5", "73"), Some(Capacity::Percent(73))); + // An unparseable custom percent yields no override (falls through to Full). + assert_eq!(parse_cpu_choice("5", "abc"), None); + } + + #[test] + fn junk_makes_no_explicit_choice() { + assert_eq!(parse_cpu_choice("9", ""), None); + assert_eq!(parse_cpu_choice("xyz", ""), None); + } + + /// F4: a lagging tier denominator can make `done > total`; the displayed + /// percentage must never exceed 100%. Raw counts stay honest elsewhere. + #[test] + fn pct_display_clamps_and_handles_empty_tier() { + assert_eq!(pct_display(6943, 6716), 100.0); // overshoot clamps + assert_eq!(pct_display(0, 0), 100.0); // empty tier => nothing to do + assert_eq!(pct_display(50, 100), 50.0); // normal case + assert_eq!(pct_display(0, 100), 0.0); + } +} diff --git a/crates/travsr-cli/src/progress.rs b/crates/travsr-cli/src/progress.rs index b4b41517..7d8ea092 100644 --- a/crates/travsr-cli/src/progress.rs +++ b/crates/travsr-cli/src/progress.rs @@ -5,7 +5,7 @@ //! stays clean for the final summary), adapting to context: //! //! - **TTY**: a single self-updating line — a pulsing graph-node spinner, an -//! eighth-precision bar, `done/total`, percent, elapsed, and a rough ETA. +//! eighth-precision bar, `done/total`, percent, and elapsed time. //! Brand orange while working; the final summary node flips to fresh green. //! - **Non-TTY** (pipe/CI): occasional newline-terminated lines, no control //! chars or color. @@ -80,7 +80,7 @@ impl Palette { fn track(self, s: &str) -> String { self.paint("38;2;77;77;77", s) } - /// Muted secondary text (elapsed/eta/hints). + /// Muted secondary text (elapsed/hints). pub fn dim(self, s: &str) -> String { self.paint("2", s) } @@ -240,16 +240,12 @@ impl ProgressReporter { } InitProgress::Indexing { done, total, .. } => { let pct = (done * 100).checked_div(total).unwrap_or(0); - let tail = match eta(self.start, done, total) { - Some(e) => format!("{elapsed} · eta {}", fmt_dur(e)), - None => elapsed, - }; format!( " {spinner} indexing {} {}/{} {pct}% {}", bar(pal, pct), commas(done), commas(total), - pal.dim(&tail) + pal.dim(&elapsed) ) } InitProgress::Finalizing => { @@ -288,11 +284,8 @@ impl ProgressReporter { } InitProgress::Indexing { done, total, .. } => { let pct = (done * 100).checked_div(total).unwrap_or(0); - let eta = eta(self.start, done, total) - .map(|e| format!(" eta {}", fmt_dur(e))) - .unwrap_or_default(); format!( - "indexing {}/{} ({pct}%) {elapsed}{eta}", + "indexing {}/{} ({pct}%) {elapsed}", commas(done), commas(total) ) @@ -790,32 +783,6 @@ pub fn fmt_dur(d: Duration) -> String { } } -/// Minimum samples + elapsed window before an ETA is trustworthy. UX-005: -/// extrapolating from the first tick (1 file in 4 s) produced a 46-minute ETA on -/// a job that finished in 13 s, inviting a premature Ctrl-C. Withhold the -/// estimate until throughput has stabilised. -const ETA_WARMUP_FILES: u64 = 8; -const ETA_WARMUP_SECS: f64 = 2.0; - -/// Rough ETA from average throughput so far. `None` once done, at start, or -/// still inside the warm-up window (see [`ETA_WARMUP_FILES`]). -fn eta(start: Instant, done: u64, total: u64) -> Option { - if done == 0 || done >= total { - return None; - } - let secs = start.elapsed().as_secs_f64(); - // Warm-up floor: too few samples or too short a window still gives a wild - // extrapolation. Hold the ETA back and show only elapsed until then. - if done < ETA_WARMUP_FILES || secs < ETA_WARMUP_SECS { - return None; - } - let rate = done as f64 / secs; // files/sec - if rate <= 0.0 { - return None; - } - Some(Duration::from_secs_f64((total - done) as f64 / rate)) -} - /// #724 Finding 4: scip-java generates a `javac` wrapper that expands empty /// arrays under `set -u`; that is an "unbound variable" error in bash 3.2 (the /// default `/bin/bash` on macOS) but legal in bash 4.4+. When the `bash` @@ -881,29 +848,6 @@ mod tests { assert_eq!(fmt_dur(Duration::from_secs(3720)), "1h02m"); } - #[test] - fn eta_none_at_edges() { - let start = Instant::now(); - assert!(eta(start, 0, 100).is_none()); - assert!(eta(start, 100, 100).is_none()); - assert!(eta(start, 150, 100).is_none()); - } - - #[test] - fn eta_withheld_during_warmup() { - // UX-005: a fresh start with only a handful of files done must not emit an - // ETA — the sample count is below the warm-up floor. - let start = Instant::now(); - assert!( - eta(start, 1, 566).is_none(), - "one file in must be inside the warm-up window" - ); - assert!( - eta(start, ETA_WARMUP_FILES - 1, 566).is_none(), - "still under the file floor => no ETA" - ); - } - #[test] fn bar_width_is_constant_and_clamped() { // No color so we can measure visible cells directly. @@ -953,6 +897,29 @@ mod tests { ); assert_eq!(parse_bash_version("not a version banner"), None); } + + #[test] + fn indexing_frame_drops_eta_keeps_elapsed() { + // D1: the projected ETA is gone from the live bar, but measured elapsed + // (a fact, not a projection) stays. describe_plain ignores the mode, so + // constructing in any mode is fine. + let r = ProgressReporter::new(true, false); + let line = r.describe_plain(InitProgress::Indexing { + done: 283, + total: 566, + workers: 4, + }); + assert!(line.contains("283/566"), "counts must remain: {line}"); + assert!(line.contains("(50%)"), "percent must remain: {line}"); + assert!( + !line.to_ascii_lowercase().contains("eta"), + "projected ETA must be gone: {line}" + ); + assert!( + line.trim_end().ends_with('s'), + "measured elapsed must remain: {line}" + ); + } } /// #755 item 3: the semantic heartbeat line — the signal that stops a From 4579359483554a57d21a0e93ec993fcaa0980323 Mon Sep 17 00:00:00 2001 From: Abhishek Date: Sun, 23 Aug 2026 16:56:38 +0530 Subject: [PATCH 2/3] [travsr-plugin-host] Preserve model.toml keys on init; wire reembed and floor F6: write_model_descriptor now merges onto an existing model.toml, preserving keys embed init does not own (for example a hand-set macos_engine) instead of clobbering the whole file from a closed field set. Add the --reembed spawn argument to run_parallel_reindex (CLI-only; the daemon auto-spawns pass false), and raise EMBED_MIN_VERSION to v1.6.0 along with every catalog version_fallback, so a pre-v1.6.0 sidecar hits the actionable floor refusal instead of a raw unknown-argument error (RFC-025). ADR-019 records the re-embed contract and the release ordering. Regenerates plugin-hashes.lock for the travsr-plugin-host src change. --- .../travsr-plugin-host/src/embed_catalog.rs | 99 ++++++++++++++++--- .../travsr-plugin-host/src/embed_catalog.toml | 10 +- docs/adrs/ADR-019-embed-reembed-contract.md | 96 ++++++++++++++++++ plugin-hashes.lock | 2 +- 4 files changed, 190 insertions(+), 17 deletions(-) create mode 100644 docs/adrs/ADR-019-embed-reembed-contract.md diff --git a/crates/travsr-plugin-host/src/embed_catalog.rs b/crates/travsr-plugin-host/src/embed_catalog.rs index 01a0d1a5..35b547ec 100644 --- a/crates/travsr-plugin-host/src/embed_catalog.rs +++ b/crates/travsr-plugin-host/src/embed_catalog.rs @@ -794,16 +794,18 @@ pub struct EmbedBackend { } /// RFC-025: the behavioral floor the host requires of the `travsr-embed` -/// sidecar. v1.2.0 is the first release whose content-hash CDC invalidation -/// (travsr-embed #376) replaced the blanket tombstone-delete path; a sidecar -/// below it runs a code path the current host no longer expects and fails deep -/// with a cryptic SQLite error (issue #701). Every embed catalog entry uses the -/// same `travsr-embed` binary, so the floor is a single host constant rather -/// than per-model catalog data. Bump this in the same commit that first relies -/// on a newer sidecar behavior (RFC-025 decision 3), and keep every -/// `version_fallback` in `embed_catalog.toml` at or above it (honesty test). +/// sidecar. v1.6.0 is the first release with the `--reembed` full-rebuild mode +/// that `travsr embed reindex --rebuild` depends on; a sidecar below it does not +/// understand the flag and would exit on the unknown argument instead of +/// rebuilding. (The prior floor, v1.2.0, guarded travsr-embed #376's content- +/// hash CDC path that replaced the blanket tombstone-delete; issue #701.) Every +/// embed catalog entry uses the same `travsr-embed` binary, so the floor is a +/// single host constant rather than per-model catalog data. Bump this in the +/// same commit that first relies on a newer sidecar behavior (RFC-025 +/// decision 3), and keep every `version_fallback` in `embed_catalog.toml` at or +/// above it (honesty test). pub const EMBED_MIN_VERSION: crate::sidecar_version::Semver = - crate::sidecar_version::Semver::new(1, 2, 0); + crate::sidecar_version::Semver::new(1, 6, 0); impl crate::sidecar_version::SidecarSpec for EmbedBackend { fn install_name(&self) -> &str { @@ -928,7 +930,33 @@ pub fn write_model_descriptor(model_dir: &Path, b: &EmbedBackend) -> anyhow::Res #[serde(skip_serializing_if = "str::is_empty")] family: &'a str, } - let content = toml::to_string(&Descriptor { + let path = model_dir.join("model.toml"); + + // The fields `travsr embed init` owns and rewrites on every run. Any other + // key already present in the file (for example a hand-set `macos_engine`, + // which is the sidecar's to own) is carried forward untouched, so re-running + // init no longer silently reverts a user's choice (F6). Merging onto the + // existing file also future-proofs against the next sidecar-owned key. + const INIT_OWNED_KEYS: &[&str] = &[ + "dim", + "pooling", + "query_prefix", + "n_inputs", + "truncate_dim", + "family", + ]; + + let mut table: toml::Table = std::fs::read_to_string(&path) + .ok() + .and_then(|s| toml::from_str(&s).ok()) + .unwrap_or_default(); + // Drop init-owned keys first so a value init no longer emits (e.g. `family` + // when the catalog entry has no arch) does not linger from a prior write. + for k in INIT_OWNED_KEYS { + table.remove(*k); + } + + let owned = toml::Value::try_from(Descriptor { dim: b.dim, pooling: &b.pooling, query_prefix: &b.query_prefix, @@ -937,7 +965,11 @@ pub fn write_model_descriptor(model_dir: &Path, b: &EmbedBackend) -> anyhow::Res family: &b.arch, }) .context("serialize model descriptor")?; - let path = model_dir.join("model.toml"); + if let toml::Value::Table(owned) = owned { + table.extend(owned); + } + + let content = toml::to_string(&table).context("serialize model descriptor")?; // Atomic write: the sidecar reads `model.toml` at startup and hard-errors on a // malformed descriptor, so a concurrent spawn (or the `ensure_model_descriptor` @@ -1334,6 +1366,7 @@ fn derive_phase1_threshold(db_path: &Path, fraction: f64) -> Option { /// /// RFC-021: one sidecar process loads the model once; N reader threads inside /// the sidecar feed a single inference loop. No temp-db management, no merge. +#[allow(clippy::too_many_arguments)] fn run_parallel_reindex( bin_path: &Path, db_path: &Path, @@ -1341,6 +1374,11 @@ fn run_parallel_reindex( model_id: &str, phase: PhaseFilter, quiet: bool, + // When true, the sidecar clears every stored vector for this model first and + // re-embeds all of them (`travsr embed reindex --rebuild`), instead of only + // filling in what is missing. CLI-only; the daemon's automatic spawns always + // pass false. + reembed: bool, overrides: &EmbedOverrides, ) -> anyhow::Result<()> { let model_ram_mb = lookup(model_id).map(|b| b.ram_mb as u64).unwrap_or(0); @@ -1398,6 +1436,10 @@ fn run_parallel_reindex( cmd.arg("--cancel-sentinel").arg(&sentinel_path); } + if reembed { + cmd.arg("--reembed"); + } + if let Some((flag, val)) = phase.sidecar_flag() { cmd.arg(flag).arg(val.to_string()); } @@ -1576,6 +1618,7 @@ pub fn spawn_background_reindex_phase1(db_path: &Path) -> bool { &model_id, PhaseFilter::Phase1 { threshold }, false, + false, &EmbedOverrides::default(), ) { tracing::warn!("embed Phase 1 failed: {e:#}"); @@ -1625,6 +1668,7 @@ pub fn spawn_background_reindex_phase2(db_path: &Path) -> bool { &model_id, phase, false, + false, &EmbedOverrides::default(), ) { tracing::warn!("embed Phase 2 failed: {e:#}"); @@ -1659,6 +1703,7 @@ pub fn spawn_background_reindex_all(db_path: &Path) -> bool { &model_id, PhaseFilter::All, false, + false, &EmbedOverrides::default(), ) { tracing::warn!("embed reindex-all failed: {e:#}"); @@ -1704,6 +1749,9 @@ pub fn ensure_reindex_backend_ready(db_path: &Path) -> anyhow::Result<()> { pub fn run_parallel_reindex_blocking( db_path: &Path, phase1_threshold: Option, + // `travsr embed reindex --rebuild`: clear + re-embed every node for the + // active model instead of only filling in what is missing. + reembed: bool, overrides: &EmbedOverrides, ) -> anyhow::Result<()> { let (bin_path, embed_db_path, model_id) = resolve_backend_for_reindex(db_path)?; @@ -1720,6 +1768,7 @@ pub fn run_parallel_reindex_blocking( &model_id, phase, false, + reembed, overrides, ) } @@ -1735,6 +1784,7 @@ pub fn run_parallel_reindex_blocking_quiet(db_path: &Path) -> anyhow::Result<()> &model_id, PhaseFilter::All, true, + false, &EmbedOverrides::default(), ) } @@ -2260,6 +2310,33 @@ mod tests { ); } + /// F6: re-running `embed init` must not revert a hand-set `macos_engine`. + /// The writer owns dim/pooling/etc. but preserves keys it does not own. + #[test] + fn write_descriptor_preserves_hand_set_keys() { + let dir = tempfile::tempdir().expect("tempdir"); + let b = lookup("bge-small-en-v1.5").expect("bge in catalog"); + let toml_path = dir.path().join("model.toml"); + // A prior descriptor with a hand-set sidecar-owned key plus a stale + // init-owned value that must be refreshed. + std::fs::write(&toml_path, b"dim = 1\nmacos_engine = \"tract\"\n").unwrap(); + + write_model_descriptor(dir.path(), b).expect("write descriptor"); + + let parsed: toml::Table = toml::from_str(&std::fs::read_to_string(&toml_path).unwrap()) + .expect("descriptor must parse"); + assert_eq!( + parsed.get("macos_engine").and_then(|v| v.as_str()), + Some("tract"), + "hand-set macos_engine must survive re-init" + ); + assert_eq!( + parsed.get("dim").and_then(|v| v.as_integer()), + Some(b.dim as i64), + "init-owned dim must be rewritten to the catalog value" + ); + } + #[test] fn lookup_finds_bge() { let b = lookup("bge-small-en-v1.5").expect("bge backend must be in catalog"); diff --git a/crates/travsr-plugin-host/src/embed_catalog.toml b/crates/travsr-plugin-host/src/embed_catalog.toml index 2a8a70e8..73bd1138 100644 --- a/crates/travsr-plugin-host/src/embed_catalog.toml +++ b/crates/travsr-plugin-host/src/embed_catalog.toml @@ -13,7 +13,7 @@ ram_mb = 450 init_secs = 47 binary_name = "travsr-embed" github_repo = "Travsr-com/travsr-embed" -version_fallback = "v1.2.0" +version_fallback = "v1.6.0" pooling = "cls" query_prefix = "Represent this sentence for searching relevant passages: " n_inputs = 2 @@ -40,7 +40,7 @@ ram_mb = 450 init_secs = 47 binary_name = "travsr-embed" github_repo = "Travsr-com/travsr-embed" -version_fallback = "v1.2.0" +version_fallback = "v1.6.0" pooling = "cls" query_prefix = "Represent this sentence for searching relevant passages: " n_inputs = 2 @@ -66,7 +66,7 @@ ram_mb = 200 init_secs = 11 binary_name = "travsr-embed" github_repo = "Travsr-com/travsr-embed" -version_fallback = "v1.2.0" +version_fallback = "v1.6.0" pooling = "cls" query_prefix = "Represent this sentence: " n_inputs = 3 @@ -92,7 +92,7 @@ ram_mb = 450 init_secs = 47 binary_name = "travsr-embed" github_repo = "Travsr-com/travsr-embed" -version_fallback = "v1.2.0" +version_fallback = "v1.6.0" pooling = "cls" query_prefix = "Represent this sentence: " n_inputs = 3 @@ -118,7 +118,7 @@ ram_mb = 1400 init_secs = 150 binary_name = "travsr-embed" github_repo = "Travsr-com/travsr-embed" -version_fallback = "v1.2.0" +version_fallback = "v1.6.0" pooling = "cls" query_prefix = "Represent this sentence: " n_inputs = 3 diff --git a/docs/adrs/ADR-019-embed-reembed-contract.md b/docs/adrs/ADR-019-embed-reembed-contract.md new file mode 100644 index 00000000..f9e36da2 --- /dev/null +++ b/docs/adrs/ADR-019-embed-reembed-contract.md @@ -0,0 +1,96 @@ +# ADR-019: Embed Re-embed Contract (`--reembed` + version floor) + +**Date:** 2026-08-23 +**Status:** Accepted +**Phase:** N/A (cross-repo contract change) +**Author:** Solution Architect +**Related:** RFC-025 (sidecar version floor / honesty tests), travsr-embed #6 (engine provenance), EMBED_UX_AUDIT.md findings F3 + F8 + +--- + +## Context + +A `travsr embed` index stores one vector per node in `embed.db`, tagged with the +engine that produced it (`meta.embed_backend`). GPU/ORT fp32 and tract fp32 +matmul are not bit-identical, so when the resolved engine changes between runs +(for example travsr-embed v1.5.0 reversed the default macOS engine to tract), +an incremental reindex leaves `embed.db` with vectors from two engines. The +sidecar detected this and printed: + +> WARNING: embedding backend changed (...). Existing vectors were produced by a +> different engine; run `travsr embed reindex --rebuild` for consistent +> embeddings. + +Two defects made that remedy dishonest (EMBED_UX_AUDIT.md F3): + +1. **`travsr embed reindex --rebuild` did not exist** — the flag errored with + "unexpected argument". +2. **No re-embed-all path existed anywhere.** `reindex` skips nodes that already + have a vector (`NOT EXISTS`); `switch` only rewrites config; `calibrate` is + explicitly "without re-embedding"; `gc` reclaims. A user could not get a + consistent index without manually deleting `embed.db`. + +The warning also fired **mid-reindex** (F8), so it read as "reindex while +reindexing". + +## Decision + +**Add a first-class full-rebuild path, spanning the sidecar and the CLI, gated by +the RFC-025 version floor. The three pieces move together and the sidecar +release ships first.** + +### 1. Sidecar: `--reindex --reembed` (travsr-embed v1.6.0) + +`--reembed` clears every stored vector for the active model **and** the recorded +engine, then the ordinary reindex re-embeds every node with the current engine +and rebuilds the HNSW index. Because the clear happens once, up front, the +existing chunked-streaming reindex loop (which drains via `NOT EXISTS`) is +reused unchanged and terminates normally. + +**Why clear-then-reindex rather than "bypass `NOT EXISTS`".** The reindex loop +depends on committed chunks dropping out of the pending set to make progress; +removing the filter would loop forever. Deleting the model's rows up front makes +every node pending again with no other change to the loop. + +**Crash-safety.** The old vectors are gone before any new one is written, so at +every moment the rows present are all from the current engine (never a torn or +mixed file). A killed run simply leaves fewer rows; a later ordinary `--reindex` +fills the rest with the same engine. Forgetting the recorded engine up front +means a mid-run crash leaves provenance honest (single engine), and a completed +run records the current engine with no mixed-engine warning. + +### 2. Sidecar: honest, end-of-run notice (F8) + +The mixed-engine notice is emitted **once at the end of a run** instead of +mid-progress, and points at the command that now exists. It is plain-language: +no engine names, no internal terms. + +### 3. CLI: `travsr embed reindex --rebuild` + version floor (RFC-025) + +`--rebuild` invokes the sidecar in `--reembed` mode through the existing +banner/progress/cancel plumbing. Because `--reembed` is a **required** sidecar +behavior the moment the CLI depends on it, the same change bumps +`EMBED_MIN_VERSION` to **v1.6.0** and every embed `version_fallback` to v1.6.0 +(RFC-025 decision 3). A pre-v1.6.0 sidecar then hits the actionable floor +refusal at the reindex entry point instead of failing on an unknown argument. + +## Release ordering (load-bearing) + +The RFC-025 honesty test `declared_floor <= latest_released` (network) refuses a +floor above what users can install. Therefore: + +1. Publish **travsr-embed v1.6.0** (carrying `--reembed`) first. +2. Once published, `latest_released >= min_version`, so honesty test (b) passes. +3. Merge the main CLI change (`--rebuild` + the v1.6.0 floor bump) after. + +Until step 1 lands, honesty test (b) is red **by design** on a networked run +(offline it skips); the non-network honesty test (a), +`version_fallback >= min_version`, stays green because both are v1.6.0. + +## Consequences + +- The engine-change remedy is now reachable and consistent end-to-end. +- The daemon's automatic reindex paths are untouched: `--reembed` is CLI-only + (every daemon spawn passes `reembed = false`). +- A future sidecar-behavior dependency repeats this pattern: add the behavior, + release the sidecar, floor to it in the same commit that first relies on it. diff --git a/plugin-hashes.lock b/plugin-hashes.lock index e4596079..24971455 100644 --- a/plugin-hashes.lock +++ b/plugin-hashes.lock @@ -1,6 +1,6 @@ # Auto-generated by update-plugin-hashes.sh — do not edit manually # Format: crate_name = sha256_of_src_tree -travsr-plugin-host = a780c268d22c4c4a38bb0bf586c4d128bdc1902153cc50994b011008a41a8b84 +travsr-plugin-host = 1e19ef90398f1e8f73d0cd3431e0c0fae2b56c5bf6debc5fd22f75ef275b6597 travsr-plugin-protocol = d6f8d6949b3c684aa21d4742a8c9ba04ccec793bea9551d8264f4c7be901d427 travsr-plugin-sdk = 6f20b7313d11d8fa0dfafd5203ef5870be443ee30b2141687c4624b7cc5f9a2b From 8745239c97dae96b6db60cb134364f1bf32ff046 Mon Sep 17 00:00:00 2001 From: Abhishek Date: Sun, 23 Aug 2026 19:08:34 +0530 Subject: [PATCH 3/3] [travsr-cli] Address PR #772 review: --rebuild/--phase1 conflict, honest fallbacks, ADR Add clap conflicts_with between --rebuild and --phase1 on embed reindex. The sidecar clears the whole model before re-embedding, so --rebuild --phase1 N deleted every vector and refilled only the phase 1 tier, silently dropping the phase 2 index from a command that reads as safe. clap now rejects the combination; a test covers it. This pairs with the sidecar-side guard in travsr-embed #24. Fix the two CPU-budget fallback messages in prompt_cpu_budget. After F1, parse_cpu_choice returns None to mean fall through to config/env/default, not Full. Both error arms printed using Full while returning None, so a user with embed.capacity = auto was told Full and got auto, the exact F1 bug on the error paths. Reword them to say the configured budget is used, and correct the stale falls through to Full comment on the test. Document the version-floor blast radius in ADR-019 Consequences. The v1.6.0 floor is a single host constant on resolve_backend, so it refuses all embedding for anyone at or below v1.5.0, including background reindexing, until they reinstall, even though --reembed is opt-in. Record this as an accepted trade-off for RFC-025 consistency, with the per-operation split noted as the escape hatch. Also extend the ADR crash-safety note to cover the HNSW index removal added in #24. --- crates/travsr-cli/src/embed.rs | 36 ++++++++++++++++++--- docs/adrs/ADR-019-embed-reembed-contract.md | 32 +++++++++++++++--- 2 files changed, 59 insertions(+), 9 deletions(-) diff --git a/crates/travsr-cli/src/embed.rs b/crates/travsr-cli/src/embed.rs index d10f14ad..9671383e 100644 --- a/crates/travsr-cli/src/embed.rs +++ b/crates/travsr-cli/src/embed.rs @@ -125,7 +125,11 @@ pub enum EmbedCommand { /// Use this after switching the embedding model, when older embeddings /// may be inconsistent with newer ones. Slower than a normal reindex, /// which only fills in what is missing. - #[arg(long)] + /// + /// Cannot be combined with `--phase1`: the sidecar clears the whole + /// model before re-embedding, so a phase-restricted rebuild would delete + /// the phase 2 tier and never refill it. + #[arg(long, conflicts_with = "phase1")] rebuild: bool, /// Only embed symbol nodes with shell_number >= N (Phase 1 high-centrality pass). /// Omit to embed all pending nodes. @@ -641,9 +645,12 @@ fn prompt_cpu_budget() -> Result> { let parsed = parse_cpu_choice(&choice, &custom); match choice.as_str() { "" | "1" | "2" | "3" | "4" => {} - "5" if parsed.is_none() => println!(" (not a valid percent, using Full)"), + // parsed is None on these two arms, which means "no explicit choice, + // fall through to config/env/default" (F1) rather than Full, so the + // message must not claim Full or it repeats the very bug F1 fixed. + "5" if parsed.is_none() => println!(" (not a valid percent, using the configured budget)"), "5" => {} - _ => println!(" (unrecognised, using Full)"), + _ => println!(" (unrecognised, using the configured budget)"), } Ok(parsed) } @@ -2808,6 +2815,27 @@ mod embed_ux_tests { assert_eq!(parse_cpu_choice("1", ""), Some(Capacity::Percent(100))); } + /// `--rebuild` and `--phase1` must be mutually exclusive: the sidecar clears + /// the whole model before re-embedding, so a phase-restricted rebuild would + /// delete the phase 2 tier and never refill it. clap must reject the combo. + #[test] + fn rebuild_and_phase1_conflict() { + use clap::Parser; + #[derive(Parser)] + struct Wrap { + #[command(subcommand)] + cmd: EmbedCommand, + } + // Each alone parses. + assert!(Wrap::try_parse_from(["x", "reindex", "--rebuild"]).is_ok()); + assert!(Wrap::try_parse_from(["x", "reindex", "--phase1", "5"]).is_ok()); + // Together they are rejected. + let err = Wrap::try_parse_from(["x", "reindex", "--rebuild", "--phase1", "5"]) + .err() + .expect("--rebuild --phase1 must conflict"); + assert_eq!(err.kind(), clap::error::ErrorKind::ArgumentConflict); + } + #[test] fn preset_choices_map_to_their_budgets() { assert_eq!(parse_cpu_choice("2", ""), Some(Capacity::Percent(50))); @@ -2818,7 +2846,7 @@ mod embed_ux_tests { #[test] fn custom_choice_parses_its_percent() { assert_eq!(parse_cpu_choice("5", "73"), Some(Capacity::Percent(73))); - // An unparseable custom percent yields no override (falls through to Full). + // An unparseable custom percent yields no override (falls through to config). assert_eq!(parse_cpu_choice("5", "abc"), None); } diff --git a/docs/adrs/ADR-019-embed-reembed-contract.md b/docs/adrs/ADR-019-embed-reembed-contract.md index f9e36da2..b3717ad8 100644 --- a/docs/adrs/ADR-019-embed-reembed-contract.md +++ b/docs/adrs/ADR-019-embed-reembed-contract.md @@ -53,11 +53,16 @@ removing the filter would loop forever. Deleting the model's rows up front makes every node pending again with no other change to the loop. **Crash-safety.** The old vectors are gone before any new one is written, so at -every moment the rows present are all from the current engine (never a torn or -mixed file). A killed run simply leaves fewer rows; a later ordinary `--reindex` -fills the rest with the same engine. Forgetting the recorded engine up front -means a mid-run crash leaves provenance honest (single engine), and a completed -run records the current engine with no mixed-engine warning. +every moment the rows present in `embed.db` are all from the current engine +(never a torn or mixed file). The stale HNSW index files are removed in the same +up-front step, because search reads the index directly and `rebuild_index` only +rewrites it after the full re-embed; leaving them would let a killed run keep +serving old-engine vectors for rows that no longer exist. A killed run therefore +degrades to "no index" (visible, recoverable by a plain `--reindex`) and fewer +rows, all current-engine, rather than a stale index that looks healthy. +Forgetting the recorded engine up front means a mid-run crash leaves provenance +honest (single engine), and a completed run records the current engine with no +mixed-engine warning. ### 2. Sidecar: honest, end-of-run notice (F8) @@ -94,3 +99,20 @@ Until step 1 lands, honesty test (b) is red **by design** on a networked run (every daemon spawn passes `reembed = false`). - A future sidecar-behavior dependency repeats this pattern: add the behavior, release the sidecar, floor to it in the same commit that first relies on it. +- **Upgrade wall (accepted trade-off).** `EMBED_MIN_VERSION` is a single host + constant consumed by `resolve_backend`, the chokepoint every embed spawn funnels + through (foreground `--rebuild`, the daemon's background phase-1/phase-2/all + passes, and `run_parallel_reindex_blocking`). Raising it to v1.6.0 therefore + refuses **all** embedding for any user still on a sidecar at or below v1.5.0 - + including ordinary background reindexing - until they run `travsr embed init + --reinstall`, even though `--reembed` is strictly opt-in. This differs from the + v1.2.0 floor, which guarded the #376 content-hash CDC path the host relied on + unconditionally, so a blunt floor was the only correct answer there. Here a + narrower gate (refuse only on the `--rebuild` path, leave `PhaseFilter`-only + spawns on the v1.2.0 floor) would protect the same case without the wall. We + keep the single blunt floor deliberately, for RFC-025 consistency: one + honesty-tested constant, one refusal message, no per-operation floor threaded + through the trust-boundary chokepoint. The cost is that the reinstall prompt + reaches the whole installed base on first upgrade rather than only users of the + new flag. If that cost proves too high in practice, splitting the floor by + operation is the escape hatch and does not change the contract above.