Improve Whitaker test-helper quality

.github/workflows/code_style.yml

@@ -50,7 +50,7 @@ jobs:
       CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_LINKER: clang
       CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUSTFLAGS: >-
         -C link-arg=-fuse-ld=mold
-      WHITAKER_INSTALLER_VERSION: '0.2.6'
+      WHITAKER_INSTALLER_VERSION: '0.2.7'
     strategy:
       fail-fast: false
       matrix:

src/agent/dispatcher/tests/auth.rs

@@ -2,6 +2,7 @@
 
 use super::super::{check_auth_required, parse_auth_result};
 use super::*;
+use crate::test_support::ExpectValid;
 
 /// Serialize `json` as a successful `Result<String, Error>` and call
 /// `check_auth_required`. Eliminates the repeated two-line setup in every
@@ -20,7 +21,7 @@ fn assert_auth_detected(
 ) {
     assert!(detected.is_some(), "expected auth detection to fire");
     let (name, instructions) =
-        detected.expect("expected auth detection to fire and return (name, instructions)");
+        detected.expect_valid("expected auth detection to fire and return (name, instructions)");
     assert_eq!(name, expected_name);
     assert!(
         instructions.contains(expected_instructions_fragment),
@@ -95,7 +96,7 @@ fn test_detect_auth_awaiting_default_instructions() {
     .to_string());
 
     let (_, instructions) = check_auth_required("tool_auth", &result)
-        .expect("expected auth detection to fire for tool_auth with awaiting_token");
+        .expect_valid("expected auth detection to fire for tool_auth with awaiting_token");
     assert_eq!(instructions, "Please provide your API token/key.");
 }
 
@@ -108,7 +109,7 @@ fn test_detect_auth_awaiting_type_field_without_name() {
     .to_string());
 
     let (name, instructions) = check_auth_required("tool_auth", &result)
-        .expect("expected auth detection to fire for type=awaiting_token");
+        .expect_valid("expected auth detection to fire for type=awaiting_token");
 
     assert_eq!(name, "tool_auth");
     assert_eq!(instructions, "Visit the auth flow.");

src/agent/dispatcher/tests/image_sentinel.rs

@@ -5,6 +5,7 @@
 //! `ChannelManager` with a `StubChannel`, verifying that SSE status events
 //! are emitted or skipped correctly.
 
+use crate::test_support::ExpectValid;
 use std::sync::Arc;
 
 use rstest::{fixture, rstest};
@@ -158,7 +159,7 @@ async fn run_image_generate_and_count_statuses(data_url: Option<&str>) -> (bool,
     let result = delegate
         .maybe_emit_image_sentinel("image_generate", &output)
         .await;
-    let count = statuses.lock().expect("statuses lock poisoned").len();
+    let count = statuses.lock().expect_valid("statuses lock poisoned").len();
     (result, count)
 }
 
@@ -210,7 +211,10 @@ async fn delegate_emits_image_generated_for_valid_data_url(
         "should return true for {tool_name} with valid sentinel"
     );
 
-    let captured = harness.statuses.lock().expect("statuses lock poisoned");
+    let captured = harness
+        .statuses
+        .lock()
+        .expect_valid("statuses lock poisoned");
     assert_eq!(captured.len(), 1, "should have emitted exactly one status");
     match &captured[0] {
         StatusUpdate::ImageGenerated { data_url, path } => {
@@ -255,7 +259,10 @@ async fn delegate_returns_false_for_non_image_tool(
 
     assert!(!result, "should return false for non-image tool");
 
-    let captured = harness.statuses.lock().expect("statuses lock poisoned");
+    let captured = harness
+        .statuses
+        .lock()
+        .expect_valid("statuses lock poisoned");
     assert!(
         captured.is_empty(),
         "should NOT emit any status for non-image tool"

src/agent/dispatcher/tests/loop_guard.rs

@@ -1,5 +1,6 @@
 //! Loop guard and termination tests.
 
+use crate::test_support::ExpectValid;
 use proptest::prelude::*;
 
 use super::*;
@@ -84,7 +85,7 @@ async fn force_text_prevents_infinite_tool_call_loop() {
     let output = reasoning
         .respond_with_tools(&ctx_normal)
         .await
-        .expect("respond_with_tools failed for normal context");
+        .expect_valid("respond_with_tools failed for normal context");
     assert!(
         matches!(output.result, RespondResult::ToolCalls { .. }),
         "Without force_text, should get tool calls"
@@ -98,7 +99,7 @@ async fn force_text_prevents_infinite_tool_call_loop() {
     let output = reasoning
         .respond_with_tools(&ctx_forced)
         .await
-        .expect("respond_with_tools failed for forced-text context");
+        .expect_valid("respond_with_tools failed for forced-text context");
     assert!(
         matches!(output.result, RespondResult::Text(_)),
         "With force_text, should get text response, got: {:?}",
@@ -149,7 +150,7 @@ async fn test_dispatcher_terminates_with_all_tool_calls_failing() {
 
     // The loop should complete (either with a text response from force_text,
     // or an error from the hard ceiling). Both are acceptable termination.
-    let inner = result.expect("test timed out or dispatcher context lost");
+    let inner = result.expect_valid("test timed out or dispatcher context lost");
     match inner {
         Ok(super::super::AgenticLoopResult::Response(text)) => {
             assert_eq!(text, "forced text");
@@ -188,16 +189,16 @@ fn build_test_agent_config(max_tool_iterations: usize) -> AgentConfig {
 }
 
 /// Assert that the timeout-wrapped agentic loop result is a text response.
-fn assert_agentic_loop_text_response<E: std::fmt::Debug>(
+fn assert_agentic_loop_text_response<E: std::fmt::Debug + std::fmt::Display>(
     result: Result<Result<super::super::AgenticLoopResult, E>, tokio::time::error::Elapsed>,
     expected_text: &str,
 ) {
     assert!(
         result.is_ok(),
         "Dispatcher timed out -- max_iterations guard failed to terminate the loop"
     );
-    let inner = result.expect("test timed out or dispatcher context lost");
-    match inner.expect("Expected Ok(AgenticLoopResult) but dispatcher returned Err") {
+    let inner = result.expect_valid("test timed out or dispatcher context lost");
+    match inner.expect_valid("Expected Ok(AgenticLoopResult) but dispatcher returned Err") {
         super::super::AgenticLoopResult::Response(text) => {
             assert_eq!(text, expected_text);
         }

src/agent/dispatcher/tests/pipeline.rs

@@ -4,11 +4,11 @@ use std::sync::{Arc, Mutex as StdMutex};
 use std::time::Instant;
 
 use crate::agent::session::PendingApproval;
-use crate::channels::StatusUpdate;
 use crate::context::JobContext;
 use crate::llm::{ChatMessage, CompletionResponse, FinishReason, NativeLlmProvider, Role};
 use crate::testing::StubChannel;
 use crate::tools::{ApprovalRequirement, NativeTool, ToolError, ToolOutput};
+use crate::{channels::StatusUpdate, test_support::ExpectValid};
 
 use super::*;
 
@@ -95,7 +95,7 @@ impl NativeLlmProvider for PipelineProvider {
             .count();
         self.observed_tool_message_counts
             .lock()
-            .expect("tool message count lock poisoned")
+            .expect_valid("tool message count lock poisoned")
             .push(tool_message_count);
 
         if tool_message_count >= self.tool_calls.len().max(1) {
@@ -243,13 +243,13 @@ async fn pipeline_runs_inline_for_single_tool() {
     let tools: Vec<Arc<dyn crate::tools::Tool>> = Vec::new();
     let (agent, statuses) = make_pipeline_agent(provider, tools, 6, false)
         .await
-        .expect("make_pipeline_agent should build");
+        .expect_valid("make_pipeline_agent should build");
     let (_session, _thread_id, message, ctx) = build_run_loop_ctx("run echo").await;
 
     let result = agent
         .run_agentic_loop(&message, ctx)
         .await
-        .expect("inline pipeline should succeed");
+        .expect_valid("inline pipeline should succeed");
 
     match result {
         super::super::AgenticLoopResult::Response(text) => assert_eq!(text, "inline done"),
@@ -258,14 +258,14 @@ async fn pipeline_runs_inline_for_single_tool() {
         }
     }
 
-    let captured = statuses.lock().expect("statuses lock poisoned");
+    let captured = statuses.lock().expect_valid("statuses lock poisoned");
     assert_tool_started_status(&captured, "echo");
     assert_tool_completed_status(&captured, "echo");
     assert_tool_result_status(&captured, "echo");
 
     let observed = observed_tool_message_counts
         .lock()
-        .expect("tool message count lock poisoned")
+        .expect_valid("tool message count lock poisoned")
         .clone();
     assert_eq!(
         observed,
@@ -302,13 +302,13 @@ async fn pipeline_runs_parallel_for_multiple_tools() {
     })];
     let (agent, statuses) = make_pipeline_agent(provider, tools, 6, false)
         .await
-        .expect("make_pipeline_agent should build");
+        .expect_valid("make_pipeline_agent should build");
     let (session, thread_id, message, ctx) = build_run_loop_ctx("run both tools").await;
 
     let result = agent
         .run_agentic_loop(&message, ctx)
         .await
-        .expect("parallel pipeline should succeed");
+        .expect_valid("parallel pipeline should succeed");
 
     match result {
         super::super::AgenticLoopResult::Response(text) => assert_eq!(text, "parallel done"),
@@ -318,15 +318,15 @@ async fn pipeline_runs_parallel_for_multiple_tools() {
     }
 
     {
-        let captured = statuses.lock().expect("statuses lock poisoned");
+        let captured = statuses.lock().expect_valid("statuses lock poisoned");
         assert_thinking_status(&captured, "Executing 2 tool(s)...");
         assert_tool_completed_status(&captured, "echo");
         assert_tool_completed_status(&captured, "second_tool");
     }
 
     let observed = observed_tool_message_counts
         .lock()
-        .expect("tool message count lock poisoned")
+        .expect_valid("tool message count lock poisoned")
         .clone();
     assert_eq!(
         observed,
@@ -338,8 +338,8 @@ async fn pipeline_runs_parallel_for_multiple_tools() {
     let thread = sess
         .threads
         .get(&thread_id)
-        .expect("thread should still exist");
-    let turn = thread.last_turn().expect("turn should exist");
+        .expect_valid("thread should still exist");
+    let turn = thread.last_turn().expect_valid("turn should exist");
     assert_eq!(
         turn.tool_calls.len(),
         2,
@@ -374,13 +374,13 @@ async fn pipeline_blocks_on_approval() {
     })];
     let (agent, _statuses) = make_pipeline_agent(provider, tools, 6, false)
         .await
-        .expect("make_pipeline_agent should build");
+        .expect_valid("make_pipeline_agent should build");
     let (_session, _thread_id, message, ctx) = build_run_loop_ctx("run approval tool").await;
 
     let result = agent
         .run_agentic_loop(&message, ctx)
         .await
-        .expect("approval pipeline should return NeedApproval");
+        .expect_valid("approval pipeline should return NeedApproval");
 
     match result {
         super::super::AgenticLoopResult::NeedApproval { pending } => {

src/agent/dispatcher/tests/skill_bundle_context_bdd.rs

@@ -1,5 +1,6 @@
 //! Behaviour tests for model-facing active skill bundle metadata.
 
+use crate::test_support::ExpectValid;
 use std::path::PathBuf;
 
 use rstest::fixture;
@@ -55,14 +56,14 @@ fn make_loaded_bundle_skill(
             PathBuf::from("SKILL.md"),
             SkillPackageKind::Bundle,
         )
-        .expect("test entrypoint is bundle-relative"),
+        .expect_valid("test entrypoint is bundle-relative"),
         content_hash: format!("sha256:{skill}"),
         compiled_patterns: Vec::new(),
         lowercased_keywords: vec!["deploy".to_string(), "docs".to_string()],
         lowercased_exclude_keywords: Vec::new(),
         lowercased_tags: Vec::new(),
     })
-    .expect("BDD skill location should match manifest")
+    .expect_valid("BDD skill location should match manifest")
 }
 
 #[given("an installed bundled skill with supporting files")]
@@ -78,20 +79,21 @@ fn installed_bundled_skill(skill_context_world: &mut SkillContextWorld) {
 
 #[given("an installed bundled skill with a references file and an assets file")]
 fn installed_bundled_skill_with_ancillary_files(skill_context_world: &mut SkillContextWorld) {
-    let installed_dir = tempfile::tempdir().expect("installed bundle tempdir should be created");
+    let installed_dir =
+        tempfile::tempdir().expect_valid("installed bundle tempdir should be created");
     ambient_fs::create_dir_all(installed_dir.path().join("references"))
-        .expect("references directory should be created");
+        .expect_valid("references directory should be created");
     ambient_fs::create_dir_all(installed_dir.path().join("assets"))
-        .expect("assets directory should be created");
+        .expect_valid("assets directory should be created");
     ambient_fs::write(installed_dir.path().join("SKILL.md"), PROMPT_MARKER)
-        .expect("SKILL.md should be written");
+        .expect_valid("SKILL.md should be written");
     ambient_fs::write(
         installed_dir.path().join("references/usage.md"),
         REFERENCES_MARKER,
     )
-    .expect("reference file should be written");
+    .expect_valid("reference file should be written");
     ambient_fs::write(installed_dir.path().join("assets/note.txt"), ASSETS_MARKER)
-        .expect("asset file should be written");
+        .expect_valid("asset file should be written");
 
     let filesystem_root = installed_dir.path().to_path_buf();
     skill_context_world.filesystem_root = Some(filesystem_root.clone());
@@ -109,10 +111,10 @@ fn selected_for_agent_turn(skill_context_world: &mut SkillContextWorld) {
     let skill = skill_context_world
         .active_skill
         .clone()
-        .expect("Given step should install an active skill");
+        .expect_valid("Given step should install an active skill");
     let rendered = agent
         .build_skill_context_block(&[skill])
-        .expect("installed bundle skill should produce context");
+        .expect_valid("installed bundle skill should produce context");
     skill_context_world.rendered_context = Some(rendered);
 }
 
@@ -145,7 +147,7 @@ fn context_hides_filesystem_root(skill_context_world: &SkillContextWorld) {
     let filesystem_root = skill_context_world
         .filesystem_root
         .as_ref()
-        .expect("Given step should record the runtime root");
+        .expect_valid("Given step should record the runtime root");
     assert!(
         !rendered.contains(&filesystem_root.to_string_lossy().to_string()),
         "active skill context must not expose the private runtime root"
@@ -182,7 +184,7 @@ fn assets_content_is_absent(skill_context_world: &SkillContextWorld) {
 fn assert_rendered_snapshot(skill_context_world: SkillContextWorld, snapshot_name: &str) {
     let rendered = skill_context_world
         .rendered_context
-        .expect("When step should render active skill context");
+        .expect_valid("When step should render active skill context");
     insta::assert_snapshot!(snapshot_name, rendered);
 }