From 802aa5bd7523a4eb5fc8134d9dcabf1a7091f29d Mon Sep 17 00:00:00 2001 From: Joel Teply Date: Sun, 16 Aug 2026 18:46:28 -0500 Subject: [PATCH] fix(test): stop letting a busy CI runner adjudicate a performance claim (evaluator <10ms flake) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `test_all_gates_pass_normal_message` failed CI at 10.72971ms against a hardcoded `decision_time_ms < 10.0`. The gates did not regress — the runner was busy. It is red on PR #2331, whose diff is CI workflow config and cannot possibly have touched persona gating. A correctness test that a loaded machine can fail is not measuring the code, it is measuring the machine. Worse, it spends the reviewer's trust every time it flakes: the next red is assumed to be noise, and eventually a real one is. The performance claim is real and worth keeping, so it is NOT deleted — it MOVES to a `#[cfg(feature = "stress-tests")]` block (CLAUDE.md § test rules, item 2), where default `cargo test` does not run it and a quiet machine can. The stress version also takes the MEDIAN of 50 runs instead of trusting one sample, since a single timing of a live system is not a fact about it — one scheduler hiccup is not a regression. The correctness test keeps its correctness assertion (`should_respond`) and now asserts nothing about the clock. Same treatment as the grounding-cost flake in PR #2330 — second instance of this class, so it is a missing constraint, not two bugs. Surveyed the rest of the tree: 3 siblings exist (command_executor <500ms, rag/engine <250ms, sentinel/parallel <180ms). Deliberately NOT touched — none is failing and each has 20-50x more headroom than the 10ms budget that broke. Named in a comment at the fix site so the next person has the map instead of rediscovering it. cargo test persona::evaluator: 33 passed, 0 failed. cargo check --features stress-tests --profile test: clean (the gated block builds). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01LoTjvf5j3Ez13g6k8mRkFo --- .../src/persona/evaluator/mod.rs | 63 +++++++++++++++++-- 1 file changed, 58 insertions(+), 5 deletions(-) diff --git a/core/continuum-core/src/persona/evaluator/mod.rs b/core/continuum-core/src/persona/evaluator/mod.rs index aec85ec58..f78f7b20c 100644 --- a/core/continuum-core/src/persona/evaluator/mod.rs +++ b/core/continuum-core/src/persona/evaluator/mod.rs @@ -999,11 +999,18 @@ mod tests { now_ms(), ); assert!(result.should_respond); - assert!( - result.decision_time_ms < 10.0, - "Decision should be <10ms, was {}ms", - result.decision_time_ms - ); + // The wall-clock assertion that used to live here (`decision_time_ms < 10.0`) + // is GONE, and deliberately not replaced with a looser bound. It failed CI at + // 10.73ms — not because the gates regressed, but because a shared runner was + // busy. A correctness test that a loaded machine can fail is not measuring the + // code; it is measuring the machine, and it spends the reviewer's trust every + // time it flakes. The performance claim it was making is real and worth + // keeping, so it MOVED to the stress block below, where `cargo test` does not + // adjudicate it (CLAUDE.md § test rules, item 2). Same treatment as the + // grounding-cost flake in PR #2330. Siblings of this class still exist and are + // NOT touched here because they are not failing and have 20-50x more headroom: + // command_executor.rs (<500ms), rag/engine.rs (<250ms), sentinel/parallel.rs + // (<180ms). If any of them starts flaking, this is the fix. } #[test] @@ -1062,4 +1069,50 @@ mod tests { // moved to their respective submodules in continuum#1208: // - rate_limiter::tests // - adequacy::tests + + /// Performance claims about the gate path. Compile-time gated so a busy shared + /// runner never adjudicates them (CLAUDE.md § test rules, item 2): default + /// `cargo test` skips this block entirely, and it is run deliberately, on a quiet + /// machine, when the claim is what you actually want to check. + #[cfg(feature = "stress-tests")] + mod stress { + use super::*; + + // what this catches: the gate path taking a slow route — an added I/O call, + // a lock, an inference hop. Every gate is a pure function over in-memory + // state, so the decision is sub-millisecond work; a budget of 10ms is ~10x + // headroom over that and still catches a category change. Measured over + // repeated runs rather than one sample, because a single timing of a live + // system is not a fact about it — one scheduler hiccup is not a regression. + #[test] + fn gate_path_stays_off_the_slow_route() { + let (engine, persona_id) = test_engine("TestBot"); + let request = test_request(persona_id, "TestBot"); + let sleep = SleepState::default(); + let rate_limiter = RateLimiterState::default(); + + const RUNS: usize = 50; + let mut times: Vec = (0..RUNS) + .map(|_| { + full_evaluate( + &request, + &rate_limiter, + &sleep, + &engine, + &RecentMessageCache::new(), + now_ms(), + ) + .decision_time_ms + }) + .collect(); + times.sort_by(|a, b| a.partial_cmp(b).expect("decision times are finite")); + let median = times[RUNS / 2]; + assert!( + median < 10.0, + "median gate decision over {RUNS} runs should be <10ms, was {median}ms \ + (slowest {}ms) — something on the gate path is doing real work", + times[RUNS - 1] + ); + } + } }