From cd3f4080079ba401439d01edf237cd7359afbfc5 Mon Sep 17 00:00:00 2001 From: Karn Date: Fri, 17 Jul 2026 18:50:37 +0530 Subject: [PATCH] fix: surface global config writes and correct first-run guidance Make read and setup commands honest about what they touch and guide the new user to the right next step. - setup now prints a notice before writing when settings land in the global config file (AUTOPHAGY_CONFIG_DIR unset) while --database points elsewhere; interactive runs fold it into the summary, --yes runs send it to stderr so JSON stdout stays clean. The "Re-running setup" line now keys off an on-disk config file rather than a possibly-stale flag. - status reports the true on-disk footprint by summing the WAL/shm sidecars, so a brand-new WAL database no longer under-reports ~4 KiB when it actually holds the ~250 KiB migrated schema, and says "created new database" for a file it just made. - status's search line points a fresh, empty database at `autophagy setup` instead of a `reindex` that has nothing to rebuild; the reindex hint stays for imported-but-unindexed stores. - empty `sessions` and `search` print friendly guidance instead of a bare header / unqualified "no matches"; JSON shapes are unchanged (empty arrays stay empty arrays). Co-Authored-By: Claude Fable 5 --- crates/autophagy-cli/src/main.rs | 71 ++++++--- crates/autophagy-cli/src/setup.rs | 31 +++- crates/autophagy-cli/src/status.rs | 37 ++++- crates/autophagy-cli/tests/cli.rs | 235 +++++++++++++++++++++++++++++ 4 files changed, 353 insertions(+), 21 deletions(-) diff --git a/crates/autophagy-cli/src/main.rs b/crates/autophagy-cli/src/main.rs index aea590c..b1f1aa9 100644 --- a/crates/autophagy-cli/src/main.rs +++ b/crates/autophagy-cli/src/main.rs @@ -718,7 +718,7 @@ impl ChallengeCheck { enum CommandReport { Import(ImportReport), Sessions(Vec), - Search(Vec), + Search(SearchReport), Digest(DigestReport), Patterns(PatternsReport), #[serde(rename = "mutations_propose")] @@ -753,6 +753,25 @@ enum CommandReport { Config(config::ConfigReport), } +/// Retrieval hits paired with whether the database held any events at all. +/// +/// The flag lets the text renderer distinguish "nothing was ever imported" from +/// "imported, but this query matched nothing" — two very different situations +/// for a new user. It never reaches the JSON surface: [`Serialize`] emits only +/// the bare hit array, so `--output json` stays an array and machine consumers +/// see no prose (an empty result is still `[]`). +#[derive(Debug)] +struct SearchReport { + hits: Vec, + database_empty: bool, +} + +impl Serialize for SearchReport { + fn serialize(&self, serializer: S) -> Result { + self.hits.serialize(serializer) + } +} + #[derive(Debug, Serialize)] struct ReindexReport { index_tool_input: bool, @@ -1278,7 +1297,14 @@ fn execute( outcome: outcome.map(RetrievalOutcome::from), limit, }; - Ok(CommandReport::Search(store.retrieve(&retrieval)?)) + let hits = store.retrieve(&retrieval)?; + // A zero-event store means nothing was ever imported, which the text + // renderer surfaces differently from a query that simply missed. + let database_empty = store.stats()?.events == 0; + Ok(CommandReport::Search(SearchReport { + hits, + database_empty, + })) } Commands::Digest { project, @@ -2025,24 +2051,35 @@ fn write_report( } }, CommandReport::Sessions(sessions) => { - writeln!(writer, "SESSION\tSOURCE\tEVENTS\tLAST EVENT\tPROJECT")?; - for session in sessions { - writeln!( - writer, - "{}\t{}\t{}\t{}\t{}", - session.session_id, - session.adapter, - session.event_count, - session.last_event_at, - session.project_path.as_deref().unwrap_or("-") - )?; + if sessions.is_empty() { + writeln!(writer, "no sessions imported yet — run `autophagy setup`")?; + } else { + writeln!(writer, "SESSION\tSOURCE\tEVENTS\tLAST EVENT\tPROJECT")?; + for session in sessions { + writeln!( + writer, + "{}\t{}\t{}\t{}\t{}", + session.session_id, + session.adapter, + session.event_count, + session.last_event_at, + session.project_path.as_deref().unwrap_or("-") + )?; + } } } - CommandReport::Search(hits) => { - if hits.is_empty() { - writeln!(writer, "no retrieval matches")?; + CommandReport::Search(report) => { + if report.hits.is_empty() { + if report.database_empty { + writeln!( + writer, + "no events imported yet — run `autophagy setup` to import your sessions" + )?; + } else { + writeln!(writer, "no retrieval matches")?; + } } - for hit in hits { + for hit in &report.hits { writeln!( writer, "{}\t{}\t{} bps\t{}", diff --git a/crates/autophagy-cli/src/setup.rs b/crates/autophagy-cli/src/setup.rs index 8898fd5..d4a9f8a 100644 --- a/crates/autophagy-cli/src/setup.rs +++ b/crates/autophagy-cli/src/setup.rs @@ -192,6 +192,11 @@ pub fn run( verbose, }; + // Whether the user pointed the database somewhere explicit. Setup's config + // file, by contrast, never moves with `--database`, so an explicit database + // is the signal that the two locations have diverged (see the global-config + // notice below). + let database_explicit = database.is_some(); let database = resolve_database_path(database)?; let mut store = open_store(&database)?; @@ -202,7 +207,10 @@ pub fn run( let prior_interval = config.watch_interval_seconds; ui.say("Autophagy setup — local, offline, nothing leaves your machine."); - if config.present { + // Only claim this is a re-run when a config file is actually on disk; a + // first run against an empty (or freshly relocated) config directory has no + // prior settings to show as defaults. + if crate::config::config_path()?.exists() { ui.say("Re-running setup — current settings are shown as defaults."); } ui.say(""); @@ -428,6 +436,27 @@ pub fn run( } // 7. Persist the chosen settings so later runs and commands inherit them. + // Settings live in a single global config file that does NOT move with + // `--database`. When the user redirected the database to an explicit path + // but the config still lands in the shared application-support directory, + // say so plainly before writing — otherwise a throwaway `--database /tmp/…` + // run silently rewrites their real global config. AUTOPHAGY_CONFIG_DIR is + // the escape hatch that keeps a run fully isolated; surface it here. + let config_is_global = std::env::var_os(crate::config::CONFIG_DIR_ENV).is_none(); + if config_is_global && database_explicit { + let notice = format!( + "note: settings are saved globally to {} (set {} to keep this run isolated)", + crate::config::config_path()?.display(), + crate::config::CONFIG_DIR_ENV, + ); + if interactive { + ui.say(¬ice); + } else { + // Non-interactive (`--yes`, including JSON): stdout carries the + // report, so the advisory goes to stderr where it can't corrupt it. + eprintln!("{notice}"); + } + } let selected_names = adapter_names(&selected); let config_path = crate::config::write_setup(&crate::config::SetupValues { adapters: selected_names.clone(), diff --git a/crates/autophagy-cli/src/status.rs b/crates/autophagy-cli/src/status.rs index 202c0f2..b667e26 100644 --- a/crates/autophagy-cli/src/status.rs +++ b/crates/autophagy-cli/src/status.rs @@ -7,7 +7,11 @@ //! is a COUNT-style query plus one deterministic detection pass; it works //! against an empty database and with no config file. -use std::{collections::BTreeMap, io::Write, path::PathBuf}; +use std::{ + collections::BTreeMap, + io::Write, + path::{Path, PathBuf}, +}; use serde::Serialize; use time::{OffsetDateTime, format_description::well_known::Rfc3339}; @@ -122,7 +126,7 @@ pub fn run( let existed_before = db_path.exists(); let store = open_store(&db_path)?; - let size_bytes = std::fs::metadata(&db_path).ok().map(|meta| meta.len()); + let size_bytes = on_disk_size(&db_path); let stats = store.stats()?; let signatures = store.signature_count()?; // Every indexed signature is an `operation/|…` key. Rows that do not @@ -250,7 +254,7 @@ pub fn write_text(report: &StatusReport, writer: &mut impl Write) -> std::io::Re let freshness = if db.exists { String::new() } else { - " · new database (nothing imported yet)".to_owned() + " · created new database (nothing imported yet)".to_owned() }; writeln!( writer, @@ -279,7 +283,13 @@ pub fn write_text(report: &StatusReport, writer: &mut impl Write) -> std::io::Re "{} command signatures indexed · exact recall on", group(i64::try_from(report.index.signatures).unwrap_or(i64::MAX)) ) + } else if report.database.events == 0 { + // Nothing imported yet: reindex has nothing to rebuild, so point the new + // user at the first step that actually populates the database. + "commands not indexed — run `autophagy setup` to import your sessions".to_owned() } else { + // Events exist but were imported without indexing; reindex is the right + // and only way to make their commands searchable. "commands not indexed — run `autophagy reindex --index-tool-input`".to_owned() }; writeln!(writer, "{}", row("Search", &search))?; @@ -442,6 +452,27 @@ fn group(value: i64) -> String { } } +/// True on-disk footprint of the database: the main file plus its WAL and +/// shared-memory sidecars. +/// +/// The store opens in WAL journal mode, so the pages written by the migrations +/// that `open_store` just ran live in `-wal` until a checkpoint. Reading +/// only the main file's length right after opening therefore undercounts a +/// freshly created database — it reports the ~4 KiB header rather than the +/// migrated schema — which is exactly the number a new user sees and disbelieves. +/// Summing the sidecars makes the reported size match what the database occupies. +fn on_disk_size(db_path: &Path) -> Option { + let mut total = std::fs::metadata(db_path).ok()?.len(); + for suffix in ["-wal", "-shm"] { + let mut sidecar = db_path.as_os_str().to_owned(); + sidecar.push(suffix); + if let Ok(meta) = std::fs::metadata(PathBuf::from(sidecar)) { + total += meta.len(); + } + } + Some(total) +} + fn format_bytes(bytes: u64) -> String { const UNITS: [&str; 4] = ["B", "KiB", "MiB", "GiB"]; #[allow(clippy::cast_precision_loss)] diff --git a/crates/autophagy-cli/tests/cli.rs b/crates/autophagy-cli/tests/cli.rs index 30631bf..fb44b51 100644 --- a/crates/autophagy-cli/tests/cli.rs +++ b/crates/autophagy-cli/tests/cli.rs @@ -1734,6 +1734,241 @@ fn detection_findings_are_cached_and_report_progress() { ); } +/// A read-only command must report the database's real footprint — the store +/// opens in WAL mode, so a naive read of the main file right after migrations +/// undercounts a brand-new database (the ~4 KiB header, not the migrated +/// schema). `status` sums the WAL sidecars so the number matches disk, and marks +/// a database it just created as new rather than pre-existing. +#[test] +fn status_reports_post_migration_size_for_a_new_database() { + let directory = tempfile::tempdir().expect("temporary directory"); + + let db_json = directory.path().join("fresh-json.db"); + assert!(!db_json.exists(), "precondition: database absent"); + let status = run_json(&db_json, ["status"]); + let size = status["result"]["database"]["size_bytes"] + .as_u64() + .expect("size_bytes present"); + assert!( + size > 100_000, + "post-migration size must reflect the real footprint, got {size} bytes" + ); + assert_eq!( + status["result"]["database"]["exists"], false, + "a database created by this very command reports as new, not pre-existing" + ); + + let db_text = directory.path().join("fresh-text.db"); + let output = command(&db_text) + .arg("status") + .output() + .expect("run status"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("created new database"), + "text status must say the database was just created: {stdout}" + ); +} + +/// On an empty database the search line points at `setup` (there is nothing to +/// reindex yet); once events exist but are unindexed, the reindex hint is the +/// correct one and returns. +#[test] +fn status_search_hint_points_at_setup_before_import_and_reindex_after() { + let directory = tempfile::tempdir().expect("temporary directory"); + + let fresh = directory.path().join("fresh.db"); + let output = command(&fresh).arg("status").output().expect("run status"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("autophagy setup"), + "an empty database's search line must point at setup: {stdout}" + ); + assert!( + !stdout.contains("reindex"), + "an empty database must not suggest reindex — nothing to rebuild: {stdout}" + ); + + // Events imported without indexing: reindex is now the right and only fix. + let imported = directory.path().join("imported.db"); + let input = directory.path().join("events.jsonl"); + fs::write(&input, VALID_JSONL).expect("write fixture"); + run_json( + &imported, + [ + "import", + input.to_str().expect("UTF-8 path"), + "--instance-key", + "fixture:cli", + ], + ); + let output = command(&imported) + .arg("status") + .output() + .expect("run status"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("reindex --index-tool-input"), + "an imported-but-unindexed database keeps the reindex hint: {stdout}" + ); +} + +/// Empty `sessions` and `search` output guides the new user in text, while the +/// JSON shapes stay stable empty arrays so machine consumers get no prose. +#[test] +fn empty_sessions_and_search_guide_the_user_but_keep_json_arrays() { + let directory = tempfile::tempdir().expect("temporary directory"); + let database = directory.path().join("empty.db"); + + // sessions: guidance instead of a bare header row; JSON is an empty array. + let output = command(&database) + .arg("sessions") + .output() + .expect("run sessions"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("no sessions imported yet"), + "empty sessions prints guidance, not a bare header: {stdout}" + ); + assert!( + !stdout.contains("SESSION\tSOURCE"), + "no bare tab-separated header row on empty sessions: {stdout}" + ); + let sessions_json = run_json(&database, ["sessions"]); + assert_eq!( + sessions_json["result"] + .as_array() + .expect("sessions array") + .len(), + 0, + "sessions JSON stays an empty array" + ); + + // search on an empty database explains nothing was imported; JSON is []. + let output = command(&database) + .args(["search", "anything"]) + .output() + .expect("run search"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("no events imported yet"), + "search against an empty database explains why it is empty: {stdout}" + ); + let search_json = run_json(&database, ["search", "anything"]); + assert!( + search_json["result"] + .as_array() + .expect("search array") + .is_empty(), + "search JSON result stays an empty array" + ); + + // Once events exist, a genuine miss reads as a miss — not "nothing imported". + let input = directory.path().join("events.jsonl"); + fs::write(&input, VALID_JSONL).expect("write fixture"); + run_json( + &database, + [ + "import", + input.to_str().expect("UTF-8 path"), + "--instance-key", + "fixture:cli", + ], + ); + let output = command(&database) + .args(["search", "zzznomatch"]) + .output() + .expect("run search"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("no retrieval matches"), + "a real miss on a populated database still says so: {stdout}" + ); + assert!( + !stdout.contains("no events imported"), + "a populated database must not claim nothing was imported: {stdout}" + ); +} + +/// `setup` must warn, before writing, that it saves settings to the GLOBAL +/// config file when the database was pointed elsewhere and no +/// `AUTOPHAGY_CONFIG_DIR` isolation is in effect — otherwise a throwaway +/// `--database /tmp/…` run silently rewrites the user's real global config. +/// +/// The "global" location is faked by overriding `HOME` (and `XDG_DATA_HOME` for +/// Linux) to a throwaway directory, so the resolved config path lands under the +/// temp home and never the developer's real one, while `AUTOPHAGY_CONFIG_DIR` +/// stays unset so the notice condition (config is global) genuinely holds. +#[test] +fn setup_warns_before_writing_global_config_for_explicit_database() { + let home = tempfile::tempdir().expect("temporary home"); + let workspace = tempfile::tempdir().expect("temporary workspace"); + let database = workspace.path().join("throwaway.db"); + let empty_claude = workspace.path().join("empty-claude"); + fs::create_dir_all(&empty_claude).expect("mkdir"); + + let output = Command::new(env!("CARGO_BIN_EXE_autophagy")) + .args(["--database", database.to_str().expect("UTF-8 path")]) + .args(["setup", "--yes", "--adapter", "claude-code"]) + // Fake the global config location under a throwaway home and leave + // AUTOPHAGY_CONFIG_DIR unset so setup treats the config as global. + .env_remove("AUTOPHAGY_CONFIG_DIR") + .env("HOME", home.path()) + .env("XDG_DATA_HOME", home.path().join("xdg")) + // No real adapter history under the throwaway home: import nothing. + .env("CLAUDE_CONFIG_DIR", &empty_claude) + .output() + .expect("run setup"); + assert!( + output.status.success(), + "setup failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("settings are saved globally to"), + "the global-config notice must appear on stderr: {stderr}" + ); + assert!( + stderr.contains("AUTOPHAGY_CONFIG_DIR"), + "the notice must name the isolation escape hatch: {stderr}" + ); + assert!( + stderr.contains(home.path().to_str().expect("UTF-8 home")), + "the config path in the notice must resolve under the throwaway home, \ + never the real one: {stderr}" + ); +} + +/// The mirror of the above: when `AUTOPHAGY_CONFIG_DIR` isolates the run (as the +/// test harness always does), the global-config notice must stay silent. +#[test] +fn setup_omits_global_notice_when_config_dir_is_isolated() { + let directory = tempfile::tempdir().expect("temporary directory"); + let database = directory.path().join("autophagy.db"); + let empty_claude = directory.path().join("empty-claude"); + fs::create_dir_all(&empty_claude).expect("mkdir"); + + // command() sets AUTOPHAGY_CONFIG_DIR to the database's temporary directory, + // so the config is isolated and the notice must not fire. + let output = command(&database) + .args(["--output", "json"]) + .args(["setup", "--yes", "--adapter", "claude-code"]) + .env("CLAUDE_CONFIG_DIR", &empty_claude) + .output() + .expect("run setup"); + assert!( + output.status.success(), + "setup failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + !stderr.contains("saved globally"), + "isolated runs must not print the global-config notice: {stderr}" + ); +} + fn run_json(database: &Path, args: [&str; N]) -> Value { let output = command(database) .args(["--output", "json"])