diff --git a/memoria/crates/memoria-cli/src/benchmark.rs b/memoria/crates/memoria-cli/src/benchmark.rs index e255af12..9ed04186 100644 --- a/memoria/crates/memoria-cli/src/benchmark.rs +++ b/memoria/crates/memoria-cli/src/benchmark.rs @@ -22,6 +22,27 @@ pub use benchmark_taxonomy::{official_category, scenario_question_type, scenario use std::collections::HashSet; +/// Scenario IDs that collapse to the same execution namespace. +/// +/// The executor lowercases `scenario_id` to build each scenario's user and +/// session namespace, so `CaseA` and `casea` are distinct to a case-sensitive +/// uniqueness check yet share one namespace at run time — letting one +/// scenario's writes, purges and corrections hit the other. Uniqueness has to +/// be judged on the normalized form the executor actually uses. +pub fn colliding_scenario_namespaces(scenarios: &[Scenario]) -> Vec { + let mut seen = HashSet::new(); + let mut errors = Vec::new(); + for s in scenarios { + if !seen.insert(s.scenario_id.to_lowercase()) { + errors.push(format!( + "duplicate scenario_id (case-insensitive): {}", + s.scenario_id + )); + } + } + errors +} + pub fn validate_dataset(content: &str) -> Vec { let mut errors = vec![]; let dataset: ScenarioDataset = match serde_json::from_str(content) { @@ -31,11 +52,8 @@ pub fn validate_dataset(content: &str) -> Vec { return errors; } }; - let mut ids = HashSet::new(); + errors.extend(colliding_scenario_namespaces(&dataset.scenarios)); for s in &dataset.scenarios { - if !ids.insert(&s.scenario_id) { - errors.push(format!("duplicate scenario_id: {}", s.scenario_id)); - } if s.seed_memories.is_empty() { errors.push(format!("{}: no seed_memories", s.scenario_id)); } @@ -127,9 +145,9 @@ mod tests { _scenario_id: id.into(), step_results: vec![], assertion_results: vec![AssertionResult { - _query: "query".into(), + query: "query".into(), returned_contents: vec!["memory".into()], - _error: None, + error: None, }], error: None, } @@ -154,9 +172,9 @@ mod tests { _scenario_id: "lme-2".into(), step_results: vec![], assertion_results: vec![AssertionResult { - _query: "query".into(), + query: "query".into(), returned_contents: vec![], - _error: None, + error: None, }], error: None, }, @@ -198,4 +216,96 @@ mod tests { Some("Instruction Following") ); } + + // ── scenario_id uniqueness must match the execution namespace ──────────── + + fn dataset_json(ids: &[&str]) -> String { + let scenarios: Vec<_> = ids + .iter() + .map(|id| { + json!({ + "scenario_id": id, + "title": "t", + "difficulty": "L1", + "horizon": "short", + "tags": [], + "seed_memories": [{"content": "memory"}], + "assertions": [{"query": "q", "expected_contents": ["memory"]}], + }) + }) + .collect(); + json!({"dataset_id": "d", "version": "1", "scenarios": scenarios}).to_string() + } + + #[test] + fn validate_rejects_case_distinct_scenario_ids_that_share_a_namespace() { + let errors = validate_dataset(&dataset_json(&["CaseA", "casea"])); + assert!( + errors.iter().any(|e| e.contains("duplicate scenario_id")), + "case-distinct ids collide in the execution namespace: {errors:?}" + ); + } + + #[test] + fn validate_accepts_genuinely_distinct_scenario_ids() { + assert!(validate_dataset(&dataset_json(&["case-a", "case-b"])).is_empty()); + } + + #[test] + fn colliding_namespaces_are_detectable_without_full_validation() { + // cmd_benchmark enforces this on every run, not just --validate-only, + // so the check has to be reachable independently of validate_dataset. + let collide: ScenarioDataset = + serde_json::from_str(&dataset_json(&["CaseA", "casea"])).unwrap(); + let distinct: ScenarioDataset = + serde_json::from_str(&dataset_json(&["case-a", "case-b"])).unwrap(); + assert_eq!(colliding_scenario_namespaces(&collide.scenarios).len(), 1); + assert!(colliding_scenario_namespaces(&distinct.scenarios).is_empty()); + } + + // ── a failed retrieval is not an empty-but-successful one ──────────────── + + #[test] + fn errored_assertion_does_not_pass_on_empty_expectations() { + let mut scenario = scenario_with_metadata("err-1", "beam", "event_ordering"); + // No expected/excluded contents: scoring an errored retrieval like a valid + // empty answer would hand this a free pass (recall and noise both 100). + scenario.assertions = vec![MemoryAssertion { + query: "query".into(), + top_k: 3, + expected_contents: vec![], + excluded_contents: vec![], + }]; + + let errored_exec = ScenarioExecution { + _scenario_id: "err-1".into(), + step_results: vec![], + assertion_results: vec![AssertionResult { + query: "query".into(), + returned_contents: vec![], + error: Some("HTTP status server error (500)".into()), + }], + error: None, + }; + let empty_exec = ScenarioExecution { + _scenario_id: "err-1".into(), + step_results: vec![], + assertion_results: vec![AssertionResult { + query: "query".into(), + returned_contents: vec![], + error: None, + }], + error: None, + }; + + let errored = score_scenario(&scenario, &errored_exec); + let empty = score_scenario(&scenario, &empty_exec); + assert_eq!(errored.aus_assertion_pass, 0.0); + assert_eq!(errored.mqs_recall, 0.0); + assert_eq!(errored.mqs_noise_rejection, 0.0); + assert_eq!( + empty.aus_assertion_pass, 100.0, + "a genuinely empty answer still scores on its own merits" + ); + } } diff --git a/memoria/crates/memoria-cli/src/benchmark_executor.rs b/memoria/crates/memoria-cli/src/benchmark_executor.rs index 4246d399..3919aafd 100644 --- a/memoria/crates/memoria-cli/src/benchmark_executor.rs +++ b/memoria/crates/memoria-cli/src/benchmark_executor.rs @@ -13,10 +13,14 @@ pub struct BenchmarkExecutor { impl BenchmarkExecutor { pub fn new(api_url: &str, token: &str) -> Self { - let run_id = SystemTime::now() + // A second-granularity timestamp alone collides: independent runs started + // in the same second share a namespace and can purge/correct each other's + // memories. Mix in random bits so each run is isolated. + let secs = SystemTime::now() .duration_since(UNIX_EPOCH) - .map(|d| d.as_secs().to_string()) + .map(|d| d.as_secs()) .unwrap_or_default(); + let run_id = format!("{secs}-{:08x}", fastrand::u32(..)); Self { base_url: api_url.trim_end_matches('/').into(), token: token.into(), @@ -131,32 +135,52 @@ impl BenchmarkExecutor { let resp = client .post(format!("{}/v1/memories", self.base_url)) .json(&body) - .send()?; + .send()? + .error_for_status()?; let data: Value = resp.json()?; - Ok(data["memory_id"].as_str().unwrap_or("").to_string()) + match data["memory_id"].as_str() { + Some(id) if !id.is_empty() => Ok(id.to_string()), + // A 2xx without a memory_id is a response-shape failure, not a write + // that happened to return nothing. + _ => anyhow::bail!("store response carried no memory_id"), + } } - fn retrieve(&self, client: &Client, query: &str, session_id: &str, top_k: i64) -> Vec { + /// `Ok(vec![])` means the service answered with no matches; an `Err` means the + /// query never produced an answer. Collapsing both into an empty vector hid + /// service failures behind valid-looking empty retrievals. + fn retrieve( + &self, + client: &Client, + query: &str, + session_id: &str, + top_k: i64, + ) -> anyhow::Result> { let resp = client .post(format!("{}/v1/memories/retrieve", self.base_url)) .json(&json!({"query": query, "top_k": top_k, "session_id": session_id})) - .send(); - let data: Value = match resp.and_then(|r| r.json()) { - Ok(v) => v, - Err(_) => return vec![], - }; + .send()? + .error_for_status()?; + let data: Value = resp.json()?; let items = if data.is_array() { data.as_array() } else { data["results"].as_array() }; + let items = items.ok_or_else(|| { + anyhow::anyhow!("retrieve response was neither an array nor {{\"results\": [...]}}") + })?; + // filter_map would silently drop an entry whose content is missing or not + // a string, so a malformed backend response could still score full marks. items - .map(|arr| { - arr.iter() - .filter_map(|i| i["content"].as_str().map(String::from)) - .collect() + .iter() + .enumerate() + .map(|(i, item)| { + item["content"].as_str().map(String::from).ok_or_else(|| { + anyhow::anyhow!("retrieve result [{i}] has no string \"content\" field") + }) }) - .unwrap_or_default() + .collect() } fn run_step(&self, client: &Client, step: &ScenarioStep, session_id: &str) -> StepResult { @@ -180,7 +204,7 @@ impl BenchmarkExecutor { step.query.as_deref().unwrap_or(""), session_id, step.top_k.unwrap_or(5), - ); + )?; } "search" => { client @@ -214,14 +238,14 @@ impl BenchmarkExecutor { })(); match result { Ok(()) => StepResult { - _action: action, + action, success: true, - _error: None, + error: None, }, Err(e) => StepResult { - _action: action, + action, success: false, - _error: Some(e.to_string()), + error: Some(format!("{e:#}")), }, } } @@ -232,11 +256,17 @@ impl BenchmarkExecutor { assertion: &MemoryAssertion, session_id: &str, ) -> AssertionResult { - let contents = self.retrieve(client, &assertion.query, session_id, assertion.top_k); - AssertionResult { - _query: assertion.query.clone(), - returned_contents: contents, - _error: None, + match self.retrieve(client, &assertion.query, session_id, assertion.top_k) { + Ok(contents) => AssertionResult { + query: assertion.query.clone(), + returned_contents: contents, + error: None, + }, + Err(e) => AssertionResult { + query: assertion.query.clone(), + returned_contents: vec![], + error: Some(format!("{e:#}")), + }, } } diff --git a/memoria/crates/memoria-cli/src/benchmark_schema.rs b/memoria/crates/memoria-cli/src/benchmark_schema.rs index 334e20d8..ee899136 100644 --- a/memoria/crates/memoria-cli/src/benchmark_schema.rs +++ b/memoria/crates/memoria-cli/src/benchmark_schema.rs @@ -80,15 +80,15 @@ fn default_top_k() -> i64 { } pub struct StepResult { - pub _action: String, + pub action: String, pub success: bool, - pub _error: Option, + pub error: Option, } pub struct AssertionResult { - pub _query: String, + pub query: String, pub returned_contents: Vec, - pub _error: Option, + pub error: Option, } pub struct ScenarioExecution { diff --git a/memoria/crates/memoria-cli/src/benchmark_scoring.rs b/memoria/crates/memoria-cli/src/benchmark_scoring.rs index 8d36e231..8b3beb39 100644 --- a/memoria/crates/memoria-cli/src/benchmark_scoring.rs +++ b/memoria/crates/memoria-cli/src/benchmark_scoring.rs @@ -88,12 +88,17 @@ pub fn score_scenario(scenario: &Scenario, exec: &ScenarioExecution) -> Scenario let mut noises = vec![]; let mut passed_count = 0usize; for (i, assertion) in scenario.assertions.iter().enumerate() { - let returned = exec - .assertion_results - .get(i) - .map(|r| r.returned_contents.as_slice()) - .unwrap_or(&[]); - let (p, r, n, ok) = score_contents(assertion, returned); + let result = exec.assertion_results.get(i); + // A retrieval that failed tells us nothing about recall or noise. Scoring + // it like an empty-but-successful answer would let an assertion with no + // expected_contents pass on a service failure. + let (p, r, n, ok) = match result { + Some(result) if result.error.is_some() => (0.0, 0.0, 0.0, false), + _ => score_contents( + assertion, + result.map(|r| r.returned_contents.as_slice()).unwrap_or(&[]), + ), + }; precisions.push(p); recalls.push(r); noises.push(n); diff --git a/memoria/crates/memoria-cli/src/main.rs b/memoria/crates/memoria-cli/src/main.rs index c369b78f..9c0f231f 100644 --- a/memoria/crates/memoria-cli/src/main.rs +++ b/memoria/crates/memoria-cli/src/main.rs @@ -11,7 +11,7 @@ mod benchmark; -use anyhow::Result; +use anyhow::{Context, Result}; use clap::{Parser, Subcommand, ValueEnum}; use std::future::IntoFuture; use std::path::{Path, PathBuf}; @@ -3221,7 +3221,7 @@ fn cmd_benchmark( dataset: &str, out: Option<&str>, validate_only: bool, -) { +) -> Result<()> { fn print_category_breakdown( heading: &str, values: &std::collections::HashMap, @@ -3281,7 +3281,7 @@ fn cmd_benchmark( } std::process::exit(1); } - return; + return Ok(()); } let ds: benchmark::ScenarioDataset = serde_json::from_str(&content).unwrap_or_else(|e| { @@ -3295,6 +3295,20 @@ fn cmd_benchmark( ds.scenarios.len() ); + // --validate-only is not the only way in: scenario IDs that share an + // execution namespace would silently let one scenario's writes, purges and + // corrections hit another, so this invariant is enforced on every run. + let collisions = benchmark::colliding_scenario_namespaces(&ds.scenarios); + if !collisions.is_empty() { + for c in &collisions { + eprintln!(" ❌ {c}"); + } + anyhow::bail!( + "dataset has {} scenario_id collision(s); scenarios would share a memory namespace", + collisions.len() + ); + } + let executor = benchmark::BenchmarkExecutor::new(api_url, token); let mut executions = std::collections::HashMap::new(); @@ -3308,6 +3322,22 @@ fn cmd_benchmark( _ => "❌", }; println!(" {icon} {:.1} ({})", result.total_score, result.grade); + // Surface service failures: a scenario can legitimately score poorly on + // recall, but a failed seed/step/retrieval means the run never measured + // what the score claims to measure. + if let Some(error) = &exec.error { + eprintln!(" ✗ setup failed: {error}"); + } + for step in exec.step_results.iter().filter(|s| !s.success) { + if let Some(error) = &step.error { + eprintln!(" ✗ step '{}' failed: {error}", step.action); + } + } + for assertion in &exec.assertion_results { + if let Some(error) = &assertion.error { + eprintln!(" ✗ assertion {:?} failed: {error}", assertion.query); + } + } executions.insert(scenario.scenario_id.clone(), exec); } @@ -3352,9 +3382,13 @@ fn cmd_benchmark( if let Some(path) = out { let json = serde_json::to_string_pretty(&report).unwrap(); - std::fs::write(path, &json).unwrap_or_else(|e| eprintln!("Failed to write {path}: {e}")); + // Scripts archive reports based on the exit status, so a failed write has + // to fail the command — and "Saved" must only be printed once it is. + std::fs::write(path, &json) + .with_context(|| format!("failed to write benchmark report to {path}"))?; println!(" Saved: {path}"); } + Ok(()) } // ── Main ────────────────────────────────────────────────────────────────────── @@ -3478,7 +3512,7 @@ fn main() -> Result<()> { out, validate_only, } => { - cmd_benchmark(&api_url, &token, &dataset, out.as_deref(), validate_only); + cmd_benchmark(&api_url, &token, &dataset, out.as_deref(), validate_only)?; } Commands::Plugin { command } => { tokio::runtime::Builder::new_multi_thread()