test: close three pre-existing CI intermittencies

crates/biorouter/src/providers/claude_code.rs

@@ -2636,8 +2636,32 @@ mod cancellation_tests {
         unsafe { libc::kill(pid, 0) == 0 }
     }
 
+    /// How long these tests wait for a child process to spawn or to exit.
+    ///
+    /// ⚠ Deliberately generous, and it used to be `100` — five seconds. Five
+    /// seconds is not a process-spawn budget on a CI runner. `test
+    /// (macos-latest)` compiles the whole workspace cold on three cores, and
+    /// this loop was measured failing on a developer machine at load ~20 with
+    /// three worktrees building:
+    ///
+    /// ```text
+    /// dropping_an_unread_stream_reaps_the_child
+    ///   panicked at claude_code.rs: "the child should have started and wrote its pid"
+    /// ```
+    ///
+    /// The failure reads as a defect in child reaping and is really the
+    /// scheduler not having got round to the child yet. It is also
+    /// self-perpetuating: CI saves its Rust cache only on a green run, so one
+    /// such red job keeps the next run cold, which makes the next timeout
+    /// MORE likely.
+    ///
+    /// Raising it costs nothing when the child behaves — every loop here exits
+    /// the moment it sees what it is waiting for, so the ceiling is only ever
+    /// paid by a genuine failure.
+    const CHILD_WAIT_TICKS: usize = 1_200; // 60 s at 50 ms per tick
+
     async fn wait_for_exit(pid: i32) -> bool {
-        for _ in 0..100 {
+        for _ in 0..CHILD_WAIT_TICKS {
             if !alive(pid) {
                 return true;
             }
@@ -2730,7 +2754,7 @@ mod cancellation_tests {
 
         // Give the child long enough to start and record its pid.
         let mut pid = None;
-        for _ in 0..100 {
+        for _ in 0..CHILD_WAIT_TICKS {
             if let Ok(text) = std::fs::read_to_string(&pid_file) {
                 if let Ok(parsed) = text.trim().parse::<i32>() {
                     pid = Some(parsed);

crates/biorouter/src/providers/codex.rs

@@ -2786,6 +2786,30 @@ for line in sys.stdin:
         unsafe { libc::kill(pid, 0) == 0 }
     }
 
+    /// How long these tests wait for a child process to spawn or to exit.
+    ///
+    /// ⚠ Deliberately generous, and it used to be `100` — five seconds. Five
+    /// seconds is not a process-spawn budget on a CI runner. `test
+    /// (macos-latest)` compiles the whole workspace cold on three cores, and
+    /// this loop was measured failing on a developer machine at load ~20 with
+    /// three worktrees building:
+    ///
+    /// ```text
+    /// dropping_an_unread_stream_reaps_the_child
+    ///   panicked at claude_code.rs: "the child should have started and wrote its pid"
+    /// ```
+    ///
+    /// The failure reads as a defect in child reaping and is really the
+    /// scheduler not having got round to the child yet. It is also
+    /// self-perpetuating: CI saves its Rust cache only on a green run, so one
+    /// such red job keeps the next run cold, which makes the next timeout
+    /// MORE likely.
+    ///
+    /// Raising it costs nothing when the child behaves — every loop here exits
+    /// the moment it sees what it is waiting for, so the ceiling is only ever
+    /// paid by a genuine failure.
+    const CHILD_WAIT_TICKS: usize = 1_200; // 60 s at 50 ms per tick
+
     /// Dropping the stream mid-turn reaps the app server.
     #[tokio::test]
     async fn dropping_a_live_stream_reaps_the_app_server() {
@@ -2833,7 +2857,7 @@ for line in sys.stdin:
         drop(stream);
 
         let mut reaped = false;
-        for _ in 0..100 {
+        for _ in 0..CHILD_WAIT_TICKS {
             if !alive(pid) {
                 reaped = true;
                 break;

crates/biorouter/src/security/global_memory.rs

@@ -1177,6 +1177,39 @@ record_result(all);"#;
 
     /// Every spelling a model might use, built independently of the production
     /// helper so the test does not agree with the code by construction.
+    /// Hold `BIOROUTER_PATH_ROOT` still for the duration of a test.
+    ///
+    /// ⚠ Every test below resolves the store **twice** — once here, via
+    /// [`store_path_spellings`], to build the path it expects to see refused,
+    /// and again inside [`global_memory_gate`] when the assertion runs. Both go
+    /// through `biorouter_mcp::global_memory_dir()` -> `config_dir()`, which
+    /// reads `BIOROUTER_PATH_ROOT` each time.
+    ///
+    /// Other tests in this binary legitimately point that variable at their own
+    /// temporary root and put it back — `logging.rs`, `managed/mod.rs`,
+    /// `providers/utils.rs` and `session/diagnostics.rs` all do, correctly,
+    /// under `env_lock`. This module took no lock at all, so one of those could
+    /// land between our two resolutions: the expected path was built against
+    /// one root and the gate matched against another, no refusal fired, and the
+    /// test failed claiming
+    ///
+    /// ```text
+    /// developer__shell {"command":"rm -rf …/config/memory"}
+    ///   points at the machine-wide store and must be refused
+    /// ```
+    ///
+    /// which reads as a hole in the #63 consent gate and is really a harness
+    /// race. Measured at 6 failures in 20 full-suite runs before this guard.
+    ///
+    /// The writers were never the problem and adding a lock to them would not
+    /// help — `env_lock` serialises only the tasks that ASK for it, and the
+    /// reader here never did. Pinning to the variable's *current* value is
+    /// deliberate: the point is to hold the lock, not to change the root.
+    fn pinned_store_root() -> env_lock::EnvGuard<'static> {
+        let current = std::env::var("BIOROUTER_PATH_ROOT").ok();
+        env_lock::lock_env([("BIOROUTER_PATH_ROOT", current.as_deref())])
+    }
+
     fn store_path_spellings() -> Vec<String> {
         let store = biorouter_mcp::global_memory_dir();
         let mut forms = vec![store.to_string_lossy().into_owned()];
@@ -1203,6 +1236,7 @@ record_result(all);"#;
     /// disclose a memory category, and there is a call that *is*.
     #[test]
     fn a_tool_that_names_the_store_path_is_refused() {
+        let _root = pinned_store_root();
         for store in store_path_spellings() {
             for (tool, arguments) in [
                 (
@@ -1299,6 +1333,7 @@ record_result(all);"#;
     /// which resolves paths and cannot be talked out of it.
     #[test]
     fn prose_that_merely_quotes_the_store_path_is_not_refused() {
+        let _root = pinned_store_root();
         for store in store_path_spellings() {
             for (tool, arguments) in [
                 // The regression as review found it: documentation about the
@@ -1377,6 +1412,7 @@ record_result(all);"#;
     /// fix that let these through would be worse than the regression.
     #[test]
     fn a_path_argument_naming_the_store_is_still_refused_after_the_narrowing() {
+        let _root = pinned_store_root();
         for store in store_path_spellings() {
             for (tool, arguments) in [
                 (
@@ -1427,6 +1463,7 @@ record_result(all);"#;
     /// store's path in its storage table, and rewriting it is an ordinary edit.
     #[test]
     fn this_repositorys_own_documentation_can_still_be_edited() {
+        let _root = pinned_store_root();
         let repo = std::path::Path::new(env!("CARGO_MANIFEST_DIR"))
             .parent()
             .and_then(|p| p.parent())

crates/biorouter/tests/skill_package_live.rs

@@ -70,6 +70,30 @@ async fn hyperframes_main_imports_as_one_package_with_every_declared_skill() {
         declared.len()
     );
 
+    // The version the repository itself declares, read here for the same reason
+    // the component list above is: a literal asserts upstream's RELEASE CADENCE
+    // rather than our importer. It was pinned to "0.8.12", HyperFrames shipped
+    // 0.8.16, and this test then failed *before* reaching the package-identity
+    // assertions it exists for — so a real change to `plan.rs` went unvalidated
+    // while the failure pointed somewhere else entirely.
+    //
+    // ⚠ Read BEFORE `plan_from_entries`, which takes `fetched.entries` BY VALUE,
+    // and own the string — a `&str` borrowed out of `plugin_manifest` would
+    // still be live across that move.
+    let plugin_manifest: serde_json::Value = serde_json::from_str(
+        &fetched
+            .entries
+            .iter()
+            .find(|entry| entry.name == ".codex-plugin/plugin.json")
+            .expect(".codex-plugin/plugin.json")
+            .text(),
+    )
+    .expect(".codex-plugin/plugin.json parses");
+    let declared_version = plugin_manifest["version"]
+        .as_str()
+        .expect("the plugin manifest declares a version")
+        .to_string();
+
     let plan = skill_package::plan_from_entries(fetched.entries, &fetched.id_hints, fetched.source)
         .expect("plan");
 
@@ -82,7 +106,7 @@ async fn hyperframes_main_imports_as_one_package_with_every_declared_skill() {
         plan.ambiguity.is_none(),
         "an explicitly declared package must not ask the user to choose"
     );
-    assert_eq!(plan.version.as_deref(), Some("0.8.12"));
+    assert_eq!(plan.version.as_deref(), Some(declared_version.as_str()));
 
     // Every skill the manifest declares, and only those.
     let mut components: Vec<String> = plan.components.iter().map(|c| c.name.clone()).collect();