diff --git a/crates/biorouter-server/src/routes/session_meta.rs b/crates/biorouter-server/src/routes/session_meta.rs index caec804c2..11d6faee7 100644 --- a/crates/biorouter-server/src/routes/session_meta.rs +++ b/crates/biorouter-server/src/routes/session_meta.rs @@ -21,11 +21,18 @@ //! //! # The poll does the reading //! -//! There is no background watcher and no registry of watched ids. The request -//! carries the ids its client has open, and while parked it re-reads exactly -//! those rows on a short interval, hands them to -//! [`SessionMetaEvents::observe`], and returns as soon as the revision moves. -//! An idle app with no chats open therefore reads nothing at all. +//! There is no background watcher. The request carries the ids its client has +//! open, and while parked it re-reads exactly those rows on a short interval, +//! hands them to [`SessionMetaEvents::observe`], and returns as soon as the +//! revision moves. An idle app with no chats open therefore reads nothing at +//! all. +//! +//! ⚠ **A poll does register its ids, and that registry is not bookkeeping.** +//! The row map behind `observe` is process-global while an id list is one +//! window's, so a poll that pruned that map against its OWN list would evict a +//! second window's chats — and a re-adopted id is adopted SILENTLY, so the two +//! windows would thrash and neither would ever be told a row moved. The claim +//! taken below lives for exactly this request; see [`SessionMetaEvents::watch`]. //! //! ⚠ **`since=0` is a baseline request, not a replay.** A client establishing //! itself gets the current revision and no changes; asking for a replay from @@ -126,6 +133,15 @@ pub async fn session_changes( .unwrap_or(MAX_WAIT) .min(MAX_WAIT); + // Claimed for the life of this poll and released when it answers, so the + // row map is pruned against the union of every live watcher rather than + // against whichever list arrived last. + // + // ⚠ It must be a NAMED binding. `let _ = events.watch(&ids)` drops the + // guard on the spot and reinstates the defect in a shape that reads as a + // fix. + let _claim = events.watch(&ids); + // Adopt this caller's ids before parking. A chat opened a moment ago has not // changed, and reporting its whole row as new would wake every window on // connect — the first observation of an id is silent by construction @@ -142,7 +158,6 @@ pub async fn session_changes( { events.observe(rows); } - events.retain_watched(&ids); let delta = events.since(query.since); if !delta.changes.is_empty() || delta.truncated { @@ -186,4 +201,53 @@ mod tests { .join(","); assert_eq!(parse_ids(Some(&many)).len(), MAX_IDS); } + + /// The claim must be BOUND, and nothing that RUNS can see whether it is. + /// + /// ⚠ This is the one line carrying the two-window fix, and every + /// behavioural test in the workspace passes without it: the row map lives + /// in `biorouter`, the poll's claim is taken here, and a route test drives + /// neither. Deleting `let _claim = …` restores the defect, and so does + /// writing `let _ = …`, which drops the guard on the spot while reading as + /// a fix. A source scan is the only instrument that can see either. + #[test] + fn the_poll_binds_its_watch_claim_for_the_life_of_the_request() { + // Production only. This module names the shape it forbids, so a scan + // that read its own tests would report itself as its first offender. + let (production, _) = include_str!("session_meta.rs") + .split_once("#[cfg(test)]") + .expect("this route's tests sit at the end, behind one `#[cfg(test)]`"); + assert!( + production.len() > 5000, + "the production slice is {} bytes, far too short to be this route — the slice is wrong and a clean result would mean nothing", + production.len() + ); + + // Comments are stripped for the same reason, and it is not theoretical + // here: the call site carries a warning that SPELLS the forbidden + // `let _ = …` form, so an unstripped scan sees two claims and fails on + // a correct tree. Truncating early can only lose a match, and a lost + // match fails the count below rather than passing quietly. + let claims: Vec<&str> = production + .lines() + .filter_map(|line| line.split("//").next()) + .filter(|code| code.contains(".watch(")) + .map(str::trim) + .collect(); + assert_eq!( + claims.len(), + 1, + "the poll claims its ids exactly once, for the life of the request. Found: {claims:?}" + ); + + // Whitespace-insensitive, so `let _=` cannot slip past a spelling. + let squashed: String = claims[0].chars().filter(|c| !c.is_whitespace()).collect(); + assert!( + squashed.starts_with("let") && !squashed.starts_with("let_="), + "the claim must be bound to a NAMED local. `let _ = …` drops the guard \ + immediately, which prunes the process-global row map against this one caller's \ + ids again — the defect this endpoint had, wearing a fix. Found: {}", + claims[0] + ); + } } diff --git a/crates/biorouter/src/model.rs b/crates/biorouter/src/model.rs index 01a821860..f8b73dc61 100644 --- a/crates/biorouter/src/model.rs +++ b/crates/biorouter/src/model.rs @@ -1172,8 +1172,8 @@ mod tests { struct Watched { key: &'static str, verdict: Verdict, - /// How many files under `crates/*/src` must still name this key **in - /// code**. + /// How many files under `crates/*/src` or `crates/*/tests` must still + /// name this key **in code**. /// /// ⚠ A **mention** count, not a match count, and deliberately so: a /// `Presence` row is healthy at zero matches, so a floor on matches @@ -1364,10 +1364,27 @@ mod tests { /// watchable set is the whole config surface, not this table. This table /// is a list of measured hazards, not a closed set. Green here is not a /// proof of hermeticity. - /// 3. **`crates/*/tests/`, which is skipped on purpose.** Each file there - /// compiles to its own binary, so a write cannot reach the lib test - /// binary these readers live in. It can still reach the other tests in - /// its own file; that is a smaller, separate hazard the audit records. + /// 3. **`crates/*/examples/` and `build.rs`.** Neither is compiled into a + /// test binary, so a write there cannot reach one at all. Eight files, + /// and the only `.rs` in the workspace this walk does not read. + /// + /// # What it walks + /// + /// `crates/*/src/**` **and** `crates/*/tests/**`. + /// + /// ⚠ **The second half was missing, and the omission read as deliberate.** + /// The two guards this table replaced walked all of `crates/**`; the table + /// walked `crates/*/src/**` and its own documentation explained why + /// `tests/` was skipped — so consolidating three instruments into one + /// narrowed the coverage by 129 of the workspace's 762 `.rs` files while + /// looking like a strict improvement. The reasoning was half right: a + /// crate's top-level `tests/` file compiles to its OWN binary, so a write + /// there cannot race the lib tests most of these readers live in. It races + /// every other test in that binary, which is a smaller hazard and not a + /// different one — and it is invisible from anywhere else, which is the + /// part that matters. Two unrestored writes of + /// `BIOROUTER_ALLOW_PROJECT_HOOKS` sat there while this guard, the audit's + /// ledger and the audit's recipe all reported the key as handled. #[test] fn no_test_parks_a_shared_setting_in_the_process_environment() { // CARGO_MANIFEST_DIR is /crates/biorouter; go up twice. @@ -1410,7 +1427,8 @@ mod tests { }) .collect(); - let mut scanned = 0usize; + let mut scanned_src = 0usize; + let mut scanned_tests = 0usize; let mut mentions = vec![0usize; WATCHED.len()]; let mut offenders: Vec = Vec::new(); @@ -1427,18 +1445,26 @@ mod tests { if path.extension().and_then(|e| e.to_str()) != Some("rs") { continue; } - // In scope: `crates//src/**`. A `tests/` directory nested - // INSIDE `src` is an ordinary module of that crate's lib and stays - // in scope; only a crate's top-level `tests/` is its own binary. + // In scope: `crates//src/**` and `crates//tests/**`. + // A `tests/` directory nested INSIDE `src` is an ordinary module of + // that crate's lib and is already covered by the first arm. + // + // Counted per scope rather than in one total, because one number + // cannot tell "the tests half was walked" from "the src half grew" — + // and narrowing back to `src` alone is exactly the regression the + // widening exists to prevent. let Ok(relative) = path.strip_prefix(&crates) else { continue; }; let mut parts = relative.components(); let _crate_name = parts.next(); - if parts.next().map(|c| c.as_os_str()) != Some(std::ffi::OsStr::new("src")) { - continue; + match parts.next().and_then(|c| c.as_os_str().to_str()) { + Some("src") => scanned_src += 1, + Some("tests") => scanned_tests += 1, + // `examples/` and `build.rs`: not compiled into a test binary, + // so a write there cannot reach one. + _ => continue, } - scanned += 1; let Ok(source) = std::fs::read_to_string(path) else { continue; }; @@ -1496,11 +1522,19 @@ mod tests { } } - // A walk that reads nothing agrees with a walk that finds nothing. + // A walk that reads nothing agrees with a walk that finds nothing. One + // floor per scope: a single total is satisfied by the `src` half alone, + // so it would pass on the very narrowing this widening undid. + assert!( + scanned_src > 400, + "the audit only scanned {scanned_src} files under crates/*/src, which is too few \ + to have walked the workspace" + ); assert!( - scanned > 400, - "the audit only scanned {scanned} files under crates/*/src, which is too few to \ - have walked the workspace" + scanned_tests > 90, + "the audit only scanned {scanned_tests} files under crates/*/tests, which is too \ + few to have walked them. A write parked there is invisible to every other test in \ + its own binary, and to every other instrument in this repository" ); // PER-KEY non-vacuity. One global floor lets most rows rot silently diff --git a/crates/biorouter/src/session_meta.rs b/crates/biorouter/src/session_meta.rs index 8e8865dd4..ba18ce9c4 100644 --- a/crates/biorouter/src/session_meta.rs +++ b/crates/biorouter/src/session_meta.rs @@ -58,10 +58,11 @@ //! believes a change's fields and never refetches drifts the first time two //! changes race. Every consumer here re-reads the row it was told about. -use std::collections::HashMap; +use std::collections::hash_map::Entry; +use std::collections::{HashMap, HashSet}; use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::{Arc, Mutex, PoisonError}; -use std::time::Duration; +use std::time::{Duration, Instant}; use serde::{Deserialize, Serialize}; use tokio::sync::Notify; @@ -75,6 +76,19 @@ use tracing::debug; /// client that has been away long enough to want a fresh read anyway. const BUFFER: usize = 32; +/// How long a chat's row survives in [`SessionMetaEvents::last`] after the last +/// watcher holding its id has gone away. +/// +/// ⚠ **A grace period, and it is what makes pruning SAFE rather than tidy.** A +/// client's claim lives for exactly one parked GET, so between two consecutive +/// polls of the only open window there is an instant with no live watcher at +/// all. Pruning strictly to the live union at that instant would evict the very +/// ids that window is about to ask about again — and [`SessionMetaEvents::observe`] +/// adopts a first-seen id SILENTLY, so a row rewritten in that gap would be +/// swallowed for good. Five minutes is far longer than any request boundary and +/// far shorter than a daemon's life. +const RETENTION: Duration = Duration::from_secs(300); + /// What changed about one chat's row. /// /// ⚠ **Advisory, exactly like the frame in `/reply`.** It is not a session @@ -121,6 +135,15 @@ pub struct SessionMetaRow { pub privacy_reason: Option, } +/// One watched chat's row, and when this process last read it. +/// +/// The timestamp is the second half of the retention rule: an id survives while +/// a live watcher holds it, and for [`RETENTION`] after the last one lets go. +struct Tracked { + row: SessionMetaRow, + seen: Instant, +} + #[derive(Default)] struct Buffer { changes: Vec, @@ -140,17 +163,31 @@ pub struct SessionMetaEvents { /// does at startup: a client that has just opened a chat has not changed it, /// and reporting the whole row as new would make every client refetch on /// connect for nothing. - last: Mutex>, + last: Mutex>, + /// The open-chat ids of every LIVE watcher, keyed by its token. + /// + /// ⚠ **This map is process-global; one caller's id list is not.** Two + /// Biorouter windows (tab tear-off is a shipped feature) each mount one + /// subscription and poll with DIFFERENT open-chat sets, so pruning + /// [`Self::last`] against the ids of whichever poll happened to arrive last + /// makes the two windows evict each other in turn: B's prune drops A's + /// chats, A's next `observe` re-adopts them SILENTLY and publishes nothing, + /// then A's prune drops B's. Neither window ever learns that a row moved. + /// The union of every live watcher is the only list that is safe to prune + /// against, which is why the registry exists at all. + watchers: Mutex>>, + /// Hands out the tokens above. Monotonic and never reused, so a guard can + /// only ever retire its own claim. + next_watcher: AtomicU64, + /// How long an id survives after its last watcher goes away — [`RETENTION`] + /// in production, and settable so a test can observe an eviction without + /// sleeping through it. + retention: Duration, } impl Default for SessionMetaEvents { fn default() -> Self { - Self { - revision: AtomicU64::new(0), - buffer: Mutex::new(Buffer::default()), - notify: Notify::new(), - last: Mutex::new(HashMap::new()), - } + Self::with_retention(RETENTION) } } @@ -161,6 +198,22 @@ impl SessionMetaEvents { &INSTANCE } + /// A feed whose unwatched ids survive `retention` rather than [`RETENTION`]. + /// + /// `Duration::ZERO` prunes strictly to the live union, which is how a test + /// asserts that dropping a watcher really does release its ids. + pub fn with_retention(retention: Duration) -> Self { + Self { + revision: AtomicU64::new(0), + buffer: Mutex::new(Buffer::default()), + notify: Notify::new(), + last: Mutex::new(HashMap::new()), + watchers: Mutex::new(HashMap::new()), + next_watcher: AtomicU64::new(0), + retention, + } + } + fn buffer(&self) -> std::sync::MutexGuard<'_, Buffer> { self.buffer.lock().unwrap_or_else(PoisonError::into_inner) } @@ -243,16 +296,26 @@ impl SessionMetaEvents { pub fn observe(&self, rows: Vec) -> usize { let mut moved = Vec::new(); { + let now = Instant::now(); let mut last = self.last.lock().unwrap_or_else(PoisonError::into_inner); for row in rows { - match last.get(&row.session_id) { - Some(before) if before == &row => {} - Some(_) => { - last.insert(row.session_id.clone(), row.clone()); - moved.push(row); + // `entry` rather than `get` + `insert`: the vacant arm writes + // through the same borrow, which a `match last.get(..)` cannot + // do on stable. + match last.entry(row.session_id.clone()) { + Entry::Occupied(mut slot) => { + let tracked = slot.get_mut(); + // The stamp is refreshed whether or not the row moved: + // it records that somebody READ this chat, which is what + // `retain_union` prunes on. + tracked.seen = now; + if tracked.row != row { + tracked.row = row.clone(); + moved.push(row); + } } - None => { - last.insert(row.session_id.clone(), row); + Entry::Vacant(slot) => { + slot.insert(Tracked { row, seen: now }); } } } @@ -264,14 +327,46 @@ impl SessionMetaEvents { count } - /// Stop tracking chats nobody is watching any more, so a long-lived daemon's - /// map does not grow with every chat ever opened. - pub fn retain_watched(&self, watched: &[String]) { - let mut last = self.last.lock().unwrap_or_else(PoisonError::into_inner); - if last.len() <= watched.len() { - return; + /// Claim `ids` as watched for as long as the returned guard lives. + /// + /// One poll, one guard. The claim is what keeps those chats in + /// [`Self::last`] while some OTHER caller's poll prunes, and retiring it is + /// the only thing that can make them eligible for eviction. + /// + /// ⚠ **Registering can only ever GROW the union, so it prunes nothing.** + /// Pruning belongs on the drop, where a claim disappears. + #[must_use = "the ids are watched only for as long as the guard is alive"] + pub fn watch(self: &Arc, ids: &[String]) -> WatchGuard { + let token = self.next_watcher.fetch_add(1, Ordering::SeqCst); + self.watchers + .lock() + .unwrap_or_else(PoisonError::into_inner) + .insert(token, ids.to_vec()); + WatchGuard { + events: Arc::clone(self), + token, } - last.retain(|id, _| watched.iter().any(|w| w == id)); + } + + /// Forget the chats that no live watcher holds and nothing has read within + /// [`Self::retention`], so a long-lived daemon's map does not grow with + /// every chat ever opened. + /// + /// ⚠ **The union of every live watcher, never one caller's list.** The + /// per-caller form this replaced is the two-window defect recorded on + /// [`Self::watchers`]; keeping the ids of every watcher is what makes the + /// prune agree with the map it prunes. + /// + /// The watchers lock is released before the rows lock is taken, so there is + /// no ordering between the two to remember. + fn retain_union(&self) { + let union: HashSet = { + let watchers = self.watchers.lock().unwrap_or_else(PoisonError::into_inner); + watchers.values().flatten().cloned().collect() + }; + let retention = self.retention; + let mut last = self.last.lock().unwrap_or_else(PoisonError::into_inner); + last.retain(|id, tracked| union.contains(id) || tracked.seen.elapsed() < retention); } /// Park until the revision moves past `since`, or `timeout` elapses. @@ -296,6 +391,26 @@ impl SessionMetaEvents { } } +/// One client's claim on the chats it has open, held for the life of its poll. +/// +/// Dropping it retires the claim and prunes the row map to what is left — see +/// [`SessionMetaEvents::watch`]. +pub struct WatchGuard { + events: Arc, + token: u64, +} + +impl Drop for WatchGuard { + fn drop(&mut self) { + self.events + .watchers + .lock() + .unwrap_or_else(PoisonError::into_inner) + .remove(&self.token); + self.events.retain_union(); + } +} + #[cfg(test)] mod tests { use super::*; @@ -314,8 +429,11 @@ mod tests { } } - fn events() -> SessionMetaEvents { - SessionMetaEvents::default() + /// An `Arc`, because a claim is taken through one — see + /// [`SessionMetaEvents::watch`]. Every existing test reaches the inner + /// methods through `Deref` and is unchanged by it. + fn events() -> Arc { + Arc::new(SessionMetaEvents::default()) } #[test] @@ -422,22 +540,114 @@ mod tests { #[test] fn a_chat_nobody_watches_stops_being_tracked() { - let e = events(); + // `Duration::ZERO` so the release is observable without sleeping through + // the grace period; that the grace exists at all is + // `a_row_rewritten_between_two_polls_of_one_window_is_still_reported`. + let e = Arc::new(SessionMetaEvents::with_retention(Duration::ZERO)); + let window_a = e.watch(&["s1".to_string()]); + let window_b = e.watch(&["s2".to_string()]); e.observe(vec![ row("s1", "versa_azure", "gpt-5.5", "public"), row("s2", "codex", "gpt-6-astra", "public"), ]); - e.retain_watched(&["s1".to_string()]); - // s2 is a stranger again, so re-observing it adopts silently rather than - // announcing a change nobody asked about. + + drop(window_b); + // s2 was nobody's the moment B retired, so it is a stranger again and + // re-observing it adopts silently rather than announcing a change + // nobody asked about. assert_eq!(e.observe(vec![row("s2", "ollama", "qwen3.6", "public")]), 0); - // s1 is still tracked. + // s1 is still claimed by A, and B retiring must not have touched it. assert_eq!(e.observe(vec![row("s1", "ollama", "qwen3.6", "public")]), 1); + + drop(window_a); + assert_eq!( + e.observe(vec![row("s1", "codex", "gpt-6-astra", "public")]), + 0, + "the last claim on s1 retired, so s1 is a stranger too" + ); + } + + #[test] + fn two_windows_with_different_open_chats_do_not_evict_each_other() { + // Two Biorouter windows, each with one subscription and its own open + // chats. The per-caller prune this replaced made them thrash: B's prune + // dropped s1, A's next `observe` re-adopted s1 as first-seen and + // published nothing, then A's prune dropped s2 — so neither window ever + // learned that a row had moved. Measured before the fix as + // "window A must learn that s1 moved; got 0". + let e = events(); + let window_a = e.watch(&["s1".to_string()]); + let window_b = e.watch(&["s2".to_string()]); + + // Each window's first sight of its own chat is silent. + assert_eq!( + e.observe(vec![row("s1", "versa_azure", "gpt-5.5", "public")]), + 0 + ); + assert_eq!( + e.observe(vec![row("s2", "codex", "gpt-6-astra", "public")]), + 0 + ); + // Interleaved rounds: the defect needed one poll from each window to + // show, and a single round would have passed against it. + for _ in 0..3 { + assert_eq!( + e.observe(vec![row("s1", "versa_azure", "gpt-5.5", "public")]), + 0 + ); + assert_eq!( + e.observe(vec![row("s2", "codex", "gpt-6-astra", "public")]), + 0 + ); + } + + // `biorouter session --resume s1 --provider claude_code` from another + // process: only window A's chat moved. + assert_eq!( + e.observe(vec![row("s1", "claude_code", "claude-opus-5", "public")]), + 1, + "window A must learn that s1 moved" + ); + assert_eq!( + e.observe(vec![row("s2", "codex", "gpt-6-astra", "public")]), + 0, + "and window B must not be told about a chat that did not move" + ); + + let delta = e.since(0); + assert_eq!(delta.changes.len(), 1); + assert_eq!(delta.changes[0].session_id, "s1"); + drop((window_a, window_b)); + } + + #[test] + fn a_row_rewritten_between_two_polls_of_one_window_is_still_reported() { + // A claim lives for exactly one parked GET, so the ONLY window's ids are + // unclaimed for an instant at every request boundary. Pruning strictly + // to the live union there would evict them, and `observe` re-adopts a + // first-seen id SILENTLY — so a row rewritten in that instant would be + // swallowed for good. `RETENTION` is what closes it, and the default + // feed is the one production uses. + let e = events(); + let poll_one = e.watch(&["s1".to_string()]); + assert_eq!( + e.observe(vec![row("s1", "versa_azure", "gpt-5.5", "public")]), + 0 + ); + drop(poll_one); + + let poll_two = e.watch(&["s1".to_string()]); + assert_eq!( + e.observe(vec![row("s1", "claude_code", "claude-opus-5", "public")]), + 1, + "a row rewritten between two polls of one window must still be reported" + ); + drop(poll_two); } #[tokio::test] async fn a_parked_poll_wakes_on_the_change_it_was_waiting_for() { - let e = Arc::new(events()); + let e = events(); e.observe(vec![row("s1", "versa_azure", "gpt-5.5", "public")]); let waiter = Arc::clone(&e); let parked = tokio::spawn(async move { diff --git a/crates/biorouter/tests/global_memory_consent_agent_loop.rs b/crates/biorouter/tests/global_memory_consent_agent_loop.rs index 82e43d55c..551bf1d4b 100644 --- a/crates/biorouter/tests/global_memory_consent_agent_loop.rs +++ b/crates/biorouter/tests/global_memory_consent_agent_loop.rs @@ -136,9 +136,12 @@ async fn agent_with_hooks( provider: Arc, mode: BioRouterMode, ) -> (Agent, String, TempDir) { - // SAFETY: only flips the project-hook opt-in the HooksManager reads when it - // is constructed, a few lines below. - std::env::set_var("BIOROUTER_ALLOW_PROJECT_HOOKS", "1"); + // The project-hooks decision is STATED on the agent's config below rather + // than parked in the process environment. `HooksManager::new_with_managed` + // reads `BIOROUTER_ALLOW_PROJECT_HOOKS` with a bare `std::env::var` that no + // lock and no task-local override can reach, and this file never removed the + // variable — so it stood for every agent built in this binary afterwards. + // See `docs/testing/process-global-state.md`. let work_dir = TempDir::new().unwrap(); std::fs::create_dir_all(work_dir.path().join(".biorouter")).unwrap(); @@ -155,7 +158,8 @@ async fn agent_with_hooks( Arc::new(PermissionManager::new(permission_dir.path().to_path_buf())), None, mode, - ); + ) + .with_project_hooks(true); let agent = Agent::with_config(config); let session = session_manager diff --git a/crates/biorouter/tests/hooks_agent_loop_tests.rs b/crates/biorouter/tests/hooks_agent_loop_tests.rs index 3ba4940d3..9ab173552 100644 --- a/crates/biorouter/tests/hooks_agent_loop_tests.rs +++ b/crates/biorouter/tests/hooks_agent_loop_tests.rs @@ -127,9 +127,12 @@ async fn agent_with_project_hooks_in_mode( provider: Arc, mode: BioRouterMode, ) -> (Agent, String, TempDir) { - // SAFETY: tests run single-threaded per process for this env var; it only - // flips on the project-hook opt-in the HooksManager reads at construction. - std::env::set_var("BIOROUTER_ALLOW_PROJECT_HOOKS", "1"); + // The project-hooks decision is STATED on the agent's config below rather + // than parked in the process environment. `HooksManager::new_with_managed` + // reads `BIOROUTER_ALLOW_PROJECT_HOOKS` with a bare `std::env::var` that no + // lock and no task-local override can reach, and this file never removed the + // variable — so it stood for every agent built in this binary afterwards. + // See `docs/testing/process-global-state.md`. let work_dir = TempDir::new().unwrap(); std::fs::create_dir_all(work_dir.path().join(".biorouter")).unwrap(); @@ -149,7 +152,8 @@ async fn agent_with_project_hooks_in_mode( Arc::new(PermissionManager::new(permission_dir.path().to_path_buf())), None, mode, - ); + ) + .with_project_hooks(true); let agent = Agent::with_config(config); let session = session_manager diff --git a/docs/testing/process-global-state.md b/docs/testing/process-global-state.md index bc746320c..1338ed11a 100644 --- a/docs/testing/process-global-state.md +++ b/docs/testing/process-global-state.md @@ -27,7 +27,7 @@ Two remedies exist and **they are not interchangeable**: - **No new locks.** `env-lock` is a single global mutex over the whole environment. Adding a holder serialises writers against writers, which was never the failure mode, and does nothing about the unlocked readers that are. - **No new `#[serial]`.** An *unkeyed* `#[serial]` is worth even less than it looks: it excludes a test from the 31 other unkeyed ones and leaves it concurrent with the rest. -- **A source-scan guard beats a stress run.** The race needs an interleaving CI produces and a loaded laptop may never show, so twenty green runs are weak evidence. A source scan cannot flake. Give every scan a non-vacuity floor and prove it against a deliberately poisoned probe before trusting it. The standing guard is `model::tests::no_test_parks_a_shared_setting_in_the_process_environment` ([#209](https://github.com/BaranziniLab/biorouter/pull/209)), one table covering nine keys on both principles. +- **A source-scan guard beats a stress run.** The race needs an interleaving CI produces and a loaded laptop may never show, so twenty green runs are weak evidence. A source scan cannot flake. Give every scan a non-vacuity floor and prove it against a deliberately poisoned probe before trusting it. The standing guard is `model::tests::no_test_parks_a_shared_setting_in_the_process_environment` ([#209](https://github.com/BaranziniLab/biorouter/pull/209)), one table covering nine keys on both principles. It walks `crates/*/src/**` **and** `crates/*/tests/**`. ⚠ The `tests/` half was added later: the two guards that table replaced had walked all of `crates/**`, so consolidating three instruments into one silently dropped 129 of the workspace's 762 `.rs` files, and a consolidation is exactly the kind of change nobody re-measures the coverage of. - **Writing the environment is not banned outright.** A key that is *test-private* — namespaced so no production reader can resolve it — and restored on drop is safe without any lock, because there is no reader to race. ## Why this document exists rather than a fifth per-key guard @@ -119,8 +119,9 @@ Verdicts: **fixed**, **live** (a reader can observe another test's write today), | `TEST_KEY`, `API_KEY`, `PROVIDER`, `PORT`, `ENABLED`, `CONFIG`, `TEST_PRECEDENCE` | none in production today; `get_param` upper-cases, so any `get_param("provider")` would resolve one | 11 unguarded bare `set_var` in 3 `config/base.rs` tests; `TEST_KEY` was never removed at all | **fixed** — [#199](https://github.com/BaranziniLab/biorouter/pull/199): namespaced to `BIOROUTER_TEST_CONFIG_*` and restored on drop | | `OSV_ENDPOINT` | `OsvChecker::new` (`agents/extension_malware_check.rs:18`), reached in production from `extension_manager.rs:972` on every Stdio extension install | 3 tests, RAII-restored but unkeyed `#[serial]` | **live** — fixed by [#205](https://github.com/BaranziniLab/biorouter/pull/205) | | `BIOROUTER_TOOL_CALL_BATCHING` | `providers/base.rs:1236`, bare `env::var`, once per streamed turn | `formats/anthropic.rs:1745` under `#[serial(tool_call_batching_env)]`, a key only 3 tests hold | **live** — 5 flag-sensitive tests hold no key; fixed by [#205](https://github.com/BaranziniLab/biorouter/pull/205) | -| `BIOROUTER_ALLOW_PROJECT_HOOKS` | `hooks/mod.rs:214`, bare `env::var` | `providers/bedrock.rs:1147`, **never removed** | **latent** — no `.biorouter/hooks.yaml` in-tree, so no reader is sensitive today; fixed by [#205](https://github.com/BaranziniLab/biorouter/pull/205) | +| `BIOROUTER_ALLOW_PROJECT_HOOKS` | `hooks/mod.rs:214`, bare `env::var` | three writers, **none of them ever removed**: `providers/bedrock.rs:1147` in the lib, and `tests/hooks_agent_loop_tests.rs:132` + `tests/global_memory_consent_agent_loop.rs:141` in two separate test binaries | **fixed** — the lib writer by [#205](https://github.com/BaranziniLab/biorouter/pull/205), the two `tests/` writers by the row below. ⚠ This row said "no `.biorouter/hooks.yaml` in-tree, so no reader is sensitive today", and that was true only of the lib: **both** test files write one into their own working directory, so each binary's reader is sensitive from its first agent onward. Neither file ever removed the variable, so the opt-in stands for every agent that binary builds afterwards — including `agent_with_memory`, which asks for `hooks: {}` and never wanted the unlock. | | `BIOROUTER_ALLOW_PROJECT_HOOKS` override | `hooks/mod.rs:214` | `agents/subagent_tool.rs:5663` via `with_config_overrides` | **live defect, not a race** — the override is a no-op, so that arm does not test its own unlock; fixed by [#205](https://github.com/BaranziniLab/biorouter/pull/205) | +| `BIOROUTER_ALLOW_PROJECT_HOOKS` in `crates/*/tests` | `hooks/mod.rs:214` | `tests/hooks_agent_loop_tests.rs:132`, `tests/global_memory_consent_agent_loop.rs:141` | **fixed** — both now state the decision on `AgentConfig::with_project_hooks`, the seam [#205](https://github.com/BaranziniLab/biorouter/pull/205) added for exactly this. ⚠ **The interesting part is not the defect, it is that three instruments reported it clean.** The standing guard walked `crates/*/src` only, the row above named one writer, and the writer recipe under [Re-measuring](#re-measuring) grepped `crates/biorouter/src` — so the key read as handled from every angle while both writes stood. Measured: restoring one of the two lines leaves the pre-widening guard green. | | `HOME` | `security/policy/command.rs:846`, `policy/target.rs:125` | `knowledge/conversation_ingest.rs:806` | **latent** — `global_memory.rs` is already mitigated by `pinned_store_root()` | | `HOME` via `dirs::home_dir()` | `config/search_path.rs:72` and `:109`, both production, inside `SearchPaths::builder()` | the same `conversation_ingest.rs:806` `env_lock` writer | **live** — `a_coding_agent_path_offers_the_node_version_managers` reads `HOME` itself at `:214` to build its expected paths, then the builder reads it again; two instants, and the test holds no lock | | `BIOROUTER_PATH_ROOT` (the general case) | ~46 live `Paths::config_dir()` readers | 33 `lock_env` writers | **open** — deferred, see below | @@ -171,8 +172,15 @@ grep -rn 'Paths::[a-z_]*(' crates/biorouter/src --include='*.rs' grep -rn 'env::var\(_os\)\?(\s*"' crates/biorouter/src --include='*.rs' ``` +⚠ **This one scans all of `crates/`, and the scope is the point.** It read +`crates/biorouter/src` until 2026-09-09, matching the standing guard's walk — which +is how two unrestored writers of `BIOROUTER_ALLOW_PROJECT_HOOKS` sat in +`crates/biorouter/tests/` unreported. A writer in a crate's top-level `tests/` +cannot reach the lib test binary, but it reaches every other test in its own +binary, and nothing else in this repository looks there. + ```bash -grep -rn 'std::env::\(set_var\|remove_var\)' crates/biorouter/src --include='*.rs' +grep -rn 'std::env::\(set_var\|remove_var\)' crates/ --include='*.rs' ``` ```bash