From b007e9ef42c251585e7b58567a9cc91911cba4b3 Mon Sep 17 00:00:00 2001 From: lr90 Date: Sat, 10 Oct 2026 12:20:49 +0800 Subject: [PATCH 1/2] fix(cli): propagate benchmark failures and isolate benchmark runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three benchmark defects, all of which let a run report success it did not earn: Report I/O (#270): cmd_benchmark wrote --out with `unwrap_or_else(|e| eprintln!(..))` and then printed "Saved" unconditionally, returning unit, so a failed write still exited 0. Scripts that archive reports by exit status silently kept nothing. Propagate the error and only print "Saved" after the write succeeds. Error accounting (#271): store() parsed the body without error_for_status and turned a missing memory_id into a successful empty string, so a 400/500 seed or store step counted as a success — a scenario could score 100/S with its setup entirely broken. retrieve() collapsed transport, HTTP and parse failures into an empty vector, indistinguishable from a valid empty answer, and the retrieve step could never fail. Both now return errors; the retrieve step propagates them; run_assertion records them; and an errored assertion scores zero instead of passing on empty expectations (recall and noise default to 100 when a scenario declares no expected/excluded contents). Failures are also printed as diagnostics instead of being discarded into unread fields. Run isolation (#273): run_id came from epoch *seconds* with no random component, so independent runs started in the same second shared a user namespace and could purge or correct each other's memories. It now mixes in random bits. Separately, validation checked scenario_id uniqueness case-sensitively while the executor lowercases it to build the namespace, so `CaseA` and `casea` validated as distinct and then collided; uniqueness is now checked on the same normalized form. Fixes #270 Fixes #271 Fixes #273 Co-Authored-By: Claude Opus 5 --- memoria/crates/memoria-cli/src/benchmark.rs | 98 +++++++++++++++++-- .../memoria-cli/src/benchmark_executor.rs | 77 ++++++++++----- .../memoria-cli/src/benchmark_schema.rs | 8 +- .../memoria-cli/src/benchmark_scoring.rs | 17 ++-- memoria/crates/memoria-cli/src/main.rs | 30 +++++- 5 files changed, 182 insertions(+), 48 deletions(-) diff --git a/memoria/crates/memoria-cli/src/benchmark.rs b/memoria/crates/memoria-cli/src/benchmark.rs index e255af12..e1b765ac 100644 --- a/memoria/crates/memoria-cli/src/benchmark.rs +++ b/memoria/crates/memoria-cli/src/benchmark.rs @@ -31,10 +31,16 @@ pub fn validate_dataset(content: &str) -> Vec { return errors; } }; + // The executor lowercases scenario_id to build each scenario's user/session + // namespace, so uniqueness has to be checked on that same normalized form — + // otherwise `CaseA` and `casea` validate as distinct but collide at run time. let mut ids = HashSet::new(); for s in &dataset.scenarios { - if !ids.insert(&s.scenario_id) { - errors.push(format!("duplicate scenario_id: {}", s.scenario_id)); + if !ids.insert(s.scenario_id.to_lowercase()) { + errors.push(format!( + "duplicate scenario_id (case-insensitive): {}", + s.scenario_id + )); } if s.seed_memories.is_empty() { errors.push(format!("{}: no seed_memories", s.scenario_id)); @@ -127,9 +133,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 +160,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 +204,84 @@ 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()); + } + + // ── 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..a541309f 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,45 @@ 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() }; - items - .map(|arr| { - arr.iter() - .filter_map(|i| i["content"].as_str().map(String::from)) - .collect() - }) - .unwrap_or_default() + let items = items.ok_or_else(|| { + anyhow::anyhow!("retrieve response was neither an array nor {{\"results\": [...]}}") + })?; + Ok(items + .iter() + .filter_map(|i| i["content"].as_str().map(String::from)) + .collect()) } fn run_step(&self, client: &Client, step: &ScenarioStep, session_id: &str) -> StepResult { @@ -180,7 +197,7 @@ impl BenchmarkExecutor { step.query.as_deref().unwrap_or(""), session_id, step.top_k.unwrap_or(5), - ); + )?; } "search" => { client @@ -214,14 +231,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 +249,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..d9549a1a 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| { @@ -3308,6 +3308,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 +3368,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 +3498,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() From 9ef4d5b87e20d4fecc04a08c554ec75f2fd3d33d Mon Sep 17 00:00:00 2001 From: lr90 Date: Sat, 10 Oct 2026 13:23:09 +0800 Subject: [PATCH 2/2] fix(cli): enforce scenario isolation on every run and reject malformed items Follow-up to review on this PR; both findings reproduced before fixing. The case-insensitive scenario_id check only ran under --validate-only, so an ordinary `memoria benchmark` still executed a dataset containing both `CaseA` and `casea` under one namespace (verified: two scenarios, one X-Impersonate-User, exit 0). The invariant is now a reusable `colliding_scenario_namespaces` helper that cmd_benchmark enforces before executing anything, so the isolation fix cannot be bypassed by omitting the flag. Only this invariant is enforced pre-run, not all of validate_dataset, to avoid failing datasets over unrelated warnings that run today. retrieve() checked the top-level response shape but still used filter_map over each item's `content`, so an entry without a string content was silently discarded: a 200 returning `[{"content":"memory"},{"memory_id":"broken"}]` scored Overall 100.0 (S) with no diagnostics. Every item is now required to carry a string `content`, and a malformed one propagates through the failure path added in this PR (same case now scores 17.5 (D) with a diagnostic). Co-Authored-By: Claude Opus 5 --- memoria/crates/memoria-cli/src/benchmark.rs | 44 ++++++++++++++----- .../memoria-cli/src/benchmark_executor.rs | 13 ++++-- memoria/crates/memoria-cli/src/main.rs | 14 ++++++ 3 files changed, 58 insertions(+), 13 deletions(-) diff --git a/memoria/crates/memoria-cli/src/benchmark.rs b/memoria/crates/memoria-cli/src/benchmark.rs index e1b765ac..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,17 +52,8 @@ pub fn validate_dataset(content: &str) -> Vec { return errors; } }; - // The executor lowercases scenario_id to build each scenario's user/session - // namespace, so uniqueness has to be checked on that same normalized form — - // otherwise `CaseA` and `casea` validate as distinct but collide at run time. - let mut ids = HashSet::new(); + errors.extend(colliding_scenario_namespaces(&dataset.scenarios)); for s in &dataset.scenarios { - if !ids.insert(s.scenario_id.to_lowercase()) { - errors.push(format!( - "duplicate scenario_id (case-insensitive): {}", - s.scenario_id - )); - } if s.seed_memories.is_empty() { errors.push(format!("{}: no seed_memories", s.scenario_id)); } @@ -239,6 +251,18 @@ mod tests { 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] diff --git a/memoria/crates/memoria-cli/src/benchmark_executor.rs b/memoria/crates/memoria-cli/src/benchmark_executor.rs index a541309f..3919aafd 100644 --- a/memoria/crates/memoria-cli/src/benchmark_executor.rs +++ b/memoria/crates/memoria-cli/src/benchmark_executor.rs @@ -170,10 +170,17 @@ impl BenchmarkExecutor { let items = items.ok_or_else(|| { anyhow::anyhow!("retrieve response was neither an array nor {{\"results\": [...]}}") })?; - Ok(items + // 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 .iter() - .filter_map(|i| i["content"].as_str().map(String::from)) - .collect()) + .enumerate() + .map(|(i, item)| { + item["content"].as_str().map(String::from).ok_or_else(|| { + anyhow::anyhow!("retrieve result [{i}] has no string \"content\" field") + }) + }) + .collect() } fn run_step(&self, client: &Client, step: &ScenarioStep, session_id: &str) -> StepResult { diff --git a/memoria/crates/memoria-cli/src/main.rs b/memoria/crates/memoria-cli/src/main.rs index d9549a1a..9c0f231f 100644 --- a/memoria/crates/memoria-cli/src/main.rs +++ b/memoria/crates/memoria-cli/src/main.rs @@ -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();