test(coverage): address PR review — tighten assertions, fix flaky test, add env-var RAII guard

Inline review fixes:
- migration/ops.rs: migrate_openclaw_returns_error_for_missing_source_workspace now requires Err() (the underlying helper bails when the source dir doesn't exist) and asserts a non-empty error message.
- text_input/ops.rs: replace tautological `!inserted || inserted` in insert_text_surfaces_accessibility_failure_as_inserted_false with the real contract: require Ok(..) and pin `inserted`↔`error` mutual exclusion. Headless runs legitimately see inserted=false, so the assertion still holds while catching regressions in either branch.
- update/scheduler.rs: remove tick_runs_without_panicking_when_event_bus_is_uninitialised — it hit real api.github.com HTTPS and was flaky under offline CI / rate limits. Replace with a comment documenting the decision and the integration-test path for exercising tick().

Nitpick fixes:
- security/ops.rs: extend security_policy_info_matches_default_policy_values with assertions for autonomy and allowed_commands so the full default shape is pinned.
- voice/postprocess.rs: tighten ready_llm_with_whitespace_only_context_never_embeds_header from `assert_eq!(result.trim(), "raw text")` to exact equality (cleanup_transcription trims internally). The pre-existing doc comment on with_ready_llm plus the in-module block comment at lines 255-268 already document the LOCAL_AI_TEST_MUTEX contract and the deliberate non-use in permissive tests — no new docs needed.
- webhooks/ops.rs: list_tunnels_hits_webhooks_core_endpoint_and_returns_payload now asserts the inbound Authorization header equals `Bearer test-session-token`; get_tunnel_encodes_id_in_path uses an id full of reserved URL chars so the test actually verifies percent-encoding rather than just trimming.
- workspace/ops.rs: introduce a WorkspaceEnvGuard RAII helper; replace all six unsafe set_var/remove_var pairs in the init_workspace tests so OPENHUMAN_WORKSPACE is cleared on panic too. Contract is documented inline: guard requires the caller to hold ENV_LOCK.

Refs tinyhumansai#530.

Author: CodeGhost21

src/openhuman/migration/ops.rs

@@ -57,26 +57,18 @@ mod tests {
     #[tokio::test]
     async fn migrate_openclaw_returns_error_for_missing_source_workspace() {
         // Pointing at a non-existent source directory must surface as
-        // an Err from the wrapper (not a panic / ok), so the JSON-RPC
-        // adapter can return the error to the caller.
+        // an Err from the wrapper (the underlying `migrate_openclaw_memory`
+        // bails with "OpenClaw workspace not found at ..."), so the
+        // JSON-RPC adapter can return the error to the caller.
         let tmp = TempDir::new().unwrap();
         let config = test_config(&tmp);
         let missing = tmp.path().join("does-not-exist").join("nested");
-        let result = migrate_openclaw(&config, Some(missing), false).await;
-        // Either an Err OR an Ok with a non-success report is
-        // acceptable here — we just pin the no-panic, deterministic-
-        // shape contract.
-        match result {
-            Ok(outcome) => {
-                // If the migration helper decides "nothing to do" for
-                // a missing source and returns Ok, we still expect the
-                // canonical log line.
-                assert!(outcome
-                    .logs
-                    .iter()
-                    .any(|l| l.contains("migration completed")));
-            }
-            Err(e) => assert!(!e.is_empty()),
-        }
+        let err = migrate_openclaw(&config, Some(missing), false)
+            .await
+            .expect_err("missing source workspace must surface as Err");
+        assert!(
+            !err.is_empty(),
+            "error string must be non-empty so the RPC caller sees a reason"
+        );
     }
 }

src/openhuman/security/ops.rs

@@ -51,6 +51,11 @@ mod tests {
     fn security_policy_info_matches_default_policy_values() {
         let outcome = security_policy_info();
         let default = SecurityPolicy::default();
+        assert_eq!(outcome.value["autonomy"], json!(default.autonomy));
+        assert_eq!(
+            outcome.value["allowed_commands"],
+            json!(default.allowed_commands)
+        );
         assert_eq!(
             outcome.value["max_actions_per_hour"],
             json!(default.max_actions_per_hour)

src/openhuman/text_input/ops.rs

@@ -273,30 +273,43 @@ mod tests {
     #[tokio::test]
     async fn insert_text_surfaces_accessibility_failure_as_inserted_false() {
         // A non-empty payload bypasses the guard and reaches the
-        // `accessibility::apply_text_to_focused_field` call. In a
-        // headless test there's no focused field, so the OS-side call
-        // errors and the wrapper packs the message into the result
-        // struct rather than propagating it as Err.
-        let result = insert_text(InsertTextParams {
+        // `accessibility::apply_text_to_focused_field` call. The contract
+        // of `insert_text` is: any platform failure is wrapped in
+        // `InsertTextResult { inserted: false, error: Some(..) }` and
+        // returned as `Ok` — never propagated as `Err` — so the JSON-RPC
+        // caller always gets a structured result. We pin that contract.
+        //
+        // On a host with a focused text field `inserted` can legitimately
+        // be `true`; in a headless CI runner it will be `false`. Either
+        // way, `inserted` and `error` must be mutually exclusive.
+        let r = insert_text(InsertTextParams {
             text: "hello".into(),
             // Keep validation flags off so the test only exercises the
-            // `apply_text_to_focused_field` error path; turning them on
-            // would route through `validate_focused_target` first which
-            // has its own OS-specific behaviour.
+            // `apply_text_to_focused_field` path; turning them on would
+            // route through `validate_focused_target` first which has its
+            // own OS-specific behaviour.
             validate_focus: None,
             expected_app: None,
             expected_role: None,
         })
-        .await;
-        // Either Ok(..) (error wrapped in result) or Err(..) — both
-        // shapes are acceptable proof we reached the platform call
-        // without panicking.
-        match result {
-            Ok(r) => assert!(
-                !r.value.inserted || r.value.inserted,
-                "insertion result must be a deterministic bool"
-            ),
-            Err(e) => assert!(!e.is_empty()),
+        .await
+        .expect("insert_text must wrap platform failures as Ok(inserted=false)");
+
+        if r.value.inserted {
+            assert!(
+                r.value.error.is_none(),
+                "inserted=true must not carry an error: {:?}",
+                r.value.error
+            );
+            assert!(r.logs.iter().any(|l| l.contains("insert_text: ok")));
+        } else {
+            let err = r
+                .value
+                .error
+                .as_deref()
+                .expect("inserted=false must carry an error message");
+            assert!(!err.is_empty(), "error message must be non-empty");
+            assert!(r.logs.iter().any(|l| l.contains("insert_text: failed")));
         }
     }
 }

src/openhuman/update/scheduler.rs

@@ -107,14 +107,14 @@ mod tests {
         run(cfg).await;
     }
 
-    #[tokio::test]
-    async fn tick_runs_without_panicking_when_event_bus_is_uninitialised() {
-        // `tick` calls `update_core::check_available` (which may hit
-        // GitHub) and then publishes a HealthChanged event. With no
-        // event bus initialised in the test process, publish_global
-        // must no-op. The HTTP call may succeed or fail depending on
-        // network availability — either branch is acceptable as long
-        // as `tick` returns cleanly.
-        tick().await;
-    }
+    // NOTE: We deliberately do NOT unit-test `tick()` directly. It calls
+    // `update_core::check_available()` which performs a real HTTPS request
+    // to api.github.com — running that from the unit suite makes the test
+    // flaky (offline CI runners, rate limits, DNS hiccups). Coverage of
+    // the HTTP + JSON-parse path is better handled via an integration test
+    // that uses an HTTP mock (e.g. `httpmock`) around a refactored
+    // `check_available_with_url(base_url)`. For now the surrounding
+    // properties are locked down by:
+    //   - `min_interval_is_at_least_ten_minutes` (rate-limit floor)
+    //   - `run_returns_immediately_when_disabled` (disabled short-circuit)
 }

src/openhuman/voice/postprocess.rs

@@ -412,6 +412,9 @@ mod tests {
             !result.contains("Conversation context:"),
             "whitespace-only context must NOT be forwarded, got: {result}"
         );
-        assert_eq!(result.trim(), "raw text");
+        // Exact equality: `cleanup_transcription` trims the LLM response,
+        // so either branch (LLM echo of the raw-only prompt, or the
+        // short-circuit fallback) must return exactly "raw text".
+        assert_eq!(result, "raw text");
     }
 }

src/openhuman/webhooks/ops.rs

@@ -220,6 +220,7 @@ mod tests {
     };
     use axum::{
         extract::Path,
+        http::HeaderMap,
         routing::{delete, get, patch, post},
         Json, Router,
     };
@@ -441,9 +442,24 @@ mod tests {
 
     #[tokio::test]
     async fn list_tunnels_hits_webhooks_core_endpoint_and_returns_payload() {
+        // Inspect the inbound Authorization header so we catch regressions
+        // where the JWT stops being forwarded (or is sent with the wrong
+        // scheme). `config_with_backend` stores `test-session-token`, so
+        // the header must be `Bearer test-session-token`.
         let app = Router::new().route(
             "/webhooks/core",
-            get(|| async { Json(json!({"tunnels": [{"id": "t-1"}]})) }),
+            get(|headers: HeaderMap| async move {
+                let auth = headers
+                    .get("authorization")
+                    .and_then(|v| v.to_str().ok())
+                    .unwrap_or("")
+                    .to_string();
+                assert_eq!(
+                    auth, "Bearer test-session-token",
+                    "authorization header must forward the stored session token"
+                );
+                Json(json!({"tunnels": [{"id": "t-1"}]}))
+            }),
         );
         let base = spawn_mock_backend(app).await;
         let tmp = TempDir::new().unwrap();
@@ -490,16 +506,26 @@ mod tests {
 
     #[tokio::test]
     async fn get_tunnel_encodes_id_in_path() {
+        // Use an id full of reserved URL characters so we actually verify
+        // percent-encoding on the outbound path. axum's `Path` extractor
+        // decodes before handing us the string, so the server must see
+        // the trimmed, *decoded* form of the id.
         let app = Router::new().route(
             "/webhooks/core/{id}",
             get(|Path(id): Path<String>| async move { Json(json!({ "id": id })) }),
         );
         let base = spawn_mock_backend(app).await;
         let tmp = TempDir::new().unwrap();
         let config = config_with_backend(&tmp, base);
-        let out = get_tunnel(&config, "  abc-123  ").await.unwrap();
-        // Server should see the trimmed id.
-        assert_eq!(out.value["id"], json!("abc-123"));
+        let raw_id = "  abc:/?#[ ]@!$&'()*+,;=%  ";
+        let trimmed = raw_id.trim();
+        let out = get_tunnel(&config, raw_id).await.unwrap();
+        assert_eq!(
+            out.value["id"],
+            json!(trimmed),
+            "server should receive the trimmed, decoded id — proves the client \
+             percent-encoded reserved chars instead of sending them raw"
+        );
     }
 
     #[tokio::test]

src/openhuman/workspace/ops.rs

@@ -103,6 +103,34 @@ mod tests {
     use crate::openhuman::config::TEST_ENV_LOCK as ENV_LOCK;
     use tempfile::tempdir;
 
+    /// RAII guard for `OPENHUMAN_WORKSPACE`. Sets the env var on
+    /// construction and clears it on drop so a panicking test doesn't
+    /// leak the override into sibling tests. Must be constructed while
+    /// holding `ENV_LOCK` — mutating process env vars concurrently is
+    /// unsafe and the lock serialises every test in this module.
+    struct WorkspaceEnvGuard;
+
+    impl WorkspaceEnvGuard {
+        fn set(path: &std::path::Path) -> Self {
+            // SAFETY: Caller holds `ENV_LOCK`, so no other thread in
+            // this process is reading or mutating this env var.
+            unsafe {
+                std::env::set_var("OPENHUMAN_WORKSPACE", path);
+            }
+            Self
+        }
+    }
+
+    impl Drop for WorkspaceEnvGuard {
+        fn drop(&mut self) {
+            // SAFETY: Same contract as `set()` — `ENV_LOCK` is held for
+            // the whole test, so no concurrent env access is possible.
+            unsafe {
+                std::env::remove_var("OPENHUMAN_WORKSPACE");
+            }
+        }
+    }
+
     // ── ensure_workspace_file ──────────────────────────────────────
 
     #[test]
@@ -175,16 +203,11 @@ mod tests {
     async fn init_workspace_creates_dirs_and_files_in_fresh_workspace() {
         let _g = ENV_LOCK.lock().unwrap_or_else(|p| p.into_inner());
         let tmp = tempdir().unwrap();
-        unsafe {
-            std::env::set_var("OPENHUMAN_WORKSPACE", tmp.path());
-        }
+        let _env = WorkspaceEnvGuard::set(tmp.path());
 
-        let result = init_workspace(false).await;
-
-        unsafe {
-            std::env::remove_var("OPENHUMAN_WORKSPACE");
-        }
-        let value = result.expect("init_workspace on empty temp should succeed");
+        let value = init_workspace(false)
+            .await
+            .expect("init_workspace on empty temp should succeed");
 
         let workspace_dir = value["result"]["workspace_dir"]
             .as_str()
@@ -221,20 +244,14 @@ mod tests {
     async fn init_workspace_reports_existing_entries_on_second_call_without_force() {
         let _g = ENV_LOCK.lock().unwrap_or_else(|p| p.into_inner());
         let tmp = tempdir().unwrap();
-        unsafe {
-            std::env::set_var("OPENHUMAN_WORKSPACE", tmp.path());
-        }
+        let _env = WorkspaceEnvGuard::set(tmp.path());
 
         // First call populates the workspace.
         init_workspace(false).await.expect("first init ok");
         // Second call without force should report everything as existing
         // and nothing as created / overwritten.
         let value = init_workspace(false).await.expect("second init ok");
 
-        unsafe {
-            std::env::remove_var("OPENHUMAN_WORKSPACE");
-        }
-
         let created = value["result"]["files"]["created"].as_array().unwrap();
         let overwritten = value["result"]["files"]["overwritten"].as_array().unwrap();
         let existing = value["result"]["files"]["existing"].as_array().unwrap();
@@ -261,9 +278,7 @@ mod tests {
     async fn init_workspace_with_force_overwrites_existing_bootstrap_files() {
         let _g = ENV_LOCK.lock().unwrap_or_else(|p| p.into_inner());
         let tmp = tempdir().unwrap();
-        unsafe {
-            std::env::set_var("OPENHUMAN_WORKSPACE", tmp.path());
-        }
+        let _env = WorkspaceEnvGuard::set(tmp.path());
 
         let first = init_workspace(false).await.expect("initial init");
         // The config loader may place the workspace at a subpath of the
@@ -280,10 +295,6 @@ mod tests {
 
         let value = init_workspace(true).await.expect("forced init");
 
-        unsafe {
-            std::env::remove_var("OPENHUMAN_WORKSPACE");
-        }
-
         let overwritten: Vec<&str> = value["result"]["files"]["overwritten"]
             .as_array()
             .unwrap()