refactor(model): simplify model test plumbing

Review cleanups for the tools/reasoning-effort model test change:

- Extract a shared parse_query_enum helper in the models handler in
  place of two copy-pasted parse-or-400 match blocks.
- Collapse the duplicated basic-probe pipeline in fabro-llm behind a
  single basic_probe core; name the shared EXPANDED_MAX_TOKENS budget.
- Pass &ModelTestArgs to test_models_via_server instead of threading
  five of its fields positionally.
- Dedupe the two forwarding CLI integration tests behind a helper.
- Derive clap::ValueEnum for ReasoningEffort behind a feature-gated
  clap dep (same pattern as MergeStrategy in fabro-types) so --help,
  cli.mdx, and error output list effort values from the enum instead
  of a hand-written list that drifts.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TDdjG18d2AHh7mFWXFkBLn

Cargo.lock

@@ -2904,6 +2904,7 @@ dependencies = [
 name = "fabro-model"
 version = "0.336.0-nightly.1"
 dependencies = [
+ "clap",
  "fabro-static",
  "http 1.4.0",
  "insta",

docs/public/reference/cli.mdx

@@ -696,7 +696,7 @@ fabro model test [OPTIONS]
 | `-j, --jobs <jobs>` | Number of model tests to run concurrently in bulk mode<br />Default: `4` |
 | `-m, --model <model>` | Test a specific model |
 | `-p, --provider <provider>` | Filter by provider |
-| `--reasoning-effort <reasoning-effort>` | Request a reasoning-effort level (`low`, `medium`, `high`, `xhigh`, or `max`) |
+| `--reasoning-effort <reasoning_effort>` | Request a reasoning-effort level<br />Values: `low`, `medium`, `high`, `xhigh`, `max` |
 | `--server <server>` | Fabro server target: http(s) URL or absolute Unix socket path |
 | `--tools` | Run a multi-turn tool-use test |
 

lib/apps/fabro-cli/Cargo.toml

@@ -22,7 +22,7 @@ fabro-auth = { path = "../../foundation/fabro-auth" }
 fabro-config = { path = "../../foundation/fabro-config" }
 fabro-environment = { path = "../../components/fabro-environment" }
 fabro-llm = { path = "../../components/fabro-llm" }
-fabro-model = { path = "../../foundation/fabro-model" }
+fabro-model = { path = "../../foundation/fabro-model", features = ["clap"] }
 fabro-oauth = { path = "../../foundation/fabro-oauth" }
 fabro-github = { path = "../../components/fabro-github" }
 fabro-agent = { path = "../../components/fabro-agent" }

lib/apps/fabro-cli/src/args.rs

@@ -1096,8 +1096,8 @@ pub(crate) struct ModelTestArgs {
     #[arg(long, alias = "deep")]
     pub(crate) tools: bool,
 
-    /// Request a reasoning-effort level (low, medium, high, xhigh, or max)
-    #[arg(long)]
+    /// Request a reasoning-effort level
+    #[arg(long, value_enum)]
     pub(crate) reasoning_effort: Option<ReasoningEffort>,
 }
 

lib/apps/fabro-cli/src/commands/model.rs

@@ -2,7 +2,7 @@ use anyhow::{Context, Result, bail};
 use cli_table::format::{Border, Justify, Separator};
 use cli_table::{Cell, CellStruct, Color, Style, Table};
 use fabro_api::types as api_types;
-use fabro_model::{Model, ModelTestMode, ProviderId, ReasoningEffort};
+use fabro_model::{Model, ModelTestMode, ProviderId};
 use fabro_util::terminal::Styles;
 use futures::{StreamExt, stream};
 use serde::Serialize;
@@ -254,15 +254,15 @@ fn model_test_row_from_status(model: &Model, status: &str, result_color: Color)
 )]
 async fn test_models_via_server(
     client: &server_client::Client,
-    provider: Option<&str>,
-    model: Option<&str>,
-    tools: bool,
-    reasoning_effort: Option<ReasoningEffort>,
-    jobs: usize,
+    args: &ModelTestArgs,
     styles: &Styles,
     json_output: bool,
 ) -> Result<()> {
-    let request_mode = tools.then_some(ModelTestMode::Deep);
+    let provider = args.provider.as_deref();
+    let model = args.model.as_deref();
+    let jobs = args.jobs;
+    let reasoning_effort = args.reasoning_effort;
+    let request_mode = args.tools.then_some(ModelTestMode::Deep);
 
     let use_color = styles.use_color;
     let mut title = models_title(use_color);
@@ -497,25 +497,8 @@ async fn run_models(
                 print_models_table(&models, &styles);
             }
         }
-        ModelsCommand::Test(ModelTestArgs {
-            provider,
-            model,
-            tools,
-            reasoning_effort,
-            jobs,
-            ..
-        }) => {
-            test_models_via_server(
-                client,
-                provider.as_deref(),
-                model.as_deref(),
-                tools,
-                reasoning_effort,
-                jobs,
-                &styles,
-                json_output,
-            )
-            .await?;
+        ModelsCommand::Test(args) => {
+            test_models_via_server(client, &args, &styles, json_output).await?;
         }
     }
 
@@ -531,7 +514,8 @@ impl Default for ModelsCommand {
 #[cfg(test)]
 mod tests {
     use fabro_model::{
-        ModelControls, ModelCosts, ModelFeatures, ModelLimits, ReasoningEffortFeature,
+        ModelControls, ModelCosts, ModelFeatures, ModelLimits, ReasoningEffort,
+        ReasoningEffortFeature,
     };
 
     use super::*;
@@ -825,18 +809,14 @@ mod tests {
 
         let client = test_client(&server.url(""));
 
-        test_models_via_server(
-            &client,
-            None,
-            Some("venice-large"),
-            false,
-            None,
-            1,
-            &Styles::new(false),
-            true,
-        )
-        .await
-        .unwrap();
+        let args = ModelTestArgs {
+            model: Some("venice-large".to_string()),
+            jobs: 1,
+            ..ModelTestArgs::default()
+        };
+        test_models_via_server(&client, &args, &Styles::new(false), true)
+            .await
+            .unwrap();
     }
 
     #[tokio::test]
@@ -897,13 +877,13 @@ mod tests {
             })
             .await;
 
+        let args = ModelTestArgs {
+            jobs: 2,
+            ..ModelTestArgs::default()
+        };
         test_models_via_server(
             &test_client(&server.url("")),
-            None,
-            None,
-            false,
-            None,
-            2,
+            &args,
             &Styles::new(false),
             true,
         )

lib/apps/fabro-cli/tests/it/cmd/model_test.rs

@@ -107,24 +107,23 @@ fn help() {
           --verbose
               Enable verbose output [env: FABRO_VERBOSE=]
           --reasoning-effort <REASONING_EFFORT>
-              Request a reasoning-effort level (low, medium, high, xhigh, or max)
+              Request a reasoning-effort level [possible values: low, medium, high, xhigh, max]
       -h, --help
               Print help
     ----- stderr -----
     ");
 }
 
-#[test]
-fn model_test_tools_and_reasoning_effort_are_forwarded() {
+fn assert_model_test_forwards(cli_args: &[&str], expected_query: &[(&str, &str)]) {
     let context = test_context!();
     let server = MockServer::start();
     context.set_http_target(&server.base_url());
     let list = mock_model_list(&server, [model_json("test-model", "anthropic", true)]);
     let test = server.mock(|when, then| {
-        when.method("POST")
-            .path("/api/v1/models/test-model/test")
-            .query_param("mode", "deep")
-            .query_param("reasoning_effort", "low");
+        let mut when = when.method("POST").path("/api/v1/models/test-model/test");
+        for (name, value) in expected_query {
+            when = when.query_param(*name, *value);
+        }
         then.status(200)
             .header("Content-Type", "application/json")
             .json_body(serde_json::json!({
@@ -135,15 +134,8 @@ fn model_test_tools_and_reasoning_effort_are_forwarded() {
     });
 
     let mut cmd = context.command();
-    cmd.args([
-        "model",
-        "test",
-        "--model",
-        "test-model",
-        "--tools",
-        "--reasoning-effort",
-        "low",
-    ]);
+    cmd.args(["model", "test", "--model", "test-model"]);
+    cmd.args(cli_args);
     let output = cmd.output().expect("command should execute");
 
     assert!(
@@ -156,37 +148,17 @@ fn model_test_tools_and_reasoning_effort_are_forwarded() {
     test.assert();
 }
 
+#[test]
+fn model_test_tools_and_reasoning_effort_are_forwarded() {
+    assert_model_test_forwards(&["--tools", "--reasoning-effort", "low"], &[
+        ("mode", "deep"),
+        ("reasoning_effort", "low"),
+    ]);
+}
+
 #[test]
 fn model_test_deep_remains_an_alias_for_tools() {
-    let context = test_context!();
-    let server = MockServer::start();
-    context.set_http_target(&server.base_url());
-    let list = mock_model_list(&server, [model_json("test-model", "anthropic", true)]);
-    let test = server.mock(|when, then| {
-        when.method("POST")
-            .path("/api/v1/models/test-model/test")
-            .query_param("mode", "deep");
-        then.status(200)
-            .header("Content-Type", "application/json")
-            .json_body(serde_json::json!({
-                "model_id": "test-model",
-                "provider": "anthropic",
-                "status": "ok"
-            }));
-    });
-
-    let mut cmd = context.command();
-    cmd.args(["model", "test", "--model", "test-model", "--deep"]);
-    let output = cmd.output().expect("command should execute");
-
-    assert!(
-        output.status.success(),
-        "model test should succeed:\nstdout:\n{}\nstderr:\n{}",
-        String::from_utf8_lossy(&output.stdout),
-        String::from_utf8_lossy(&output.stderr)
-    );
-    list.assert();
-    test.assert();
+    assert_model_test_forwards(&["--deep"], &[("mode", "deep")]);
 }
 
 #[test]

lib/apps/fabro-server/src/server/handler/models.rs

@@ -190,37 +190,32 @@ async fn test_providers(_auth: RequiredUser, State(state): State<Arc<AppState>>)
     }
 }
 
+fn parse_query_enum<T: FromStr>(value: Option<&str>, label: &str) -> Result<Option<T>, ApiError> {
+    value
+        .map(|value| {
+            T::from_str(value).map_err(|_| {
+                ApiError::new(StatusCode::BAD_REQUEST, format!("invalid {label}: {value}"))
+            })
+        })
+        .transpose()
+}
+
 async fn test_model(
     _auth: RequiredUser,
     State(state): State<Arc<AppState>>,
     Path(id): Path<String>,
     Query(params): Query<ModelTestParams>,
 ) -> Response {
-    let mode = match params.mode.as_deref() {
-        Some(value) => match ModelTestMode::from_str(value) {
-            Ok(mode) => mode,
-            Err(_) => {
-                return ApiError::new(
-                    StatusCode::BAD_REQUEST,
-                    format!("invalid model test mode: {value}"),
-                )
-                .into_response();
-            }
-        },
-        None => ModelTestMode::Basic,
+    let mode = match parse_query_enum(params.mode.as_deref(), "model test mode") {
+        Ok(mode) => mode.unwrap_or(ModelTestMode::Basic),
+        Err(error) => return error.into_response(),
     };
-    let reasoning_effort = match params.reasoning_effort.as_deref() {
-        Some(value) => match ReasoningEffort::from_str(value) {
-            Ok(reasoning_effort) => Some(reasoning_effort),
-            Err(_) => {
-                return ApiError::new(
-                    StatusCode::BAD_REQUEST,
-                    format!("invalid reasoning effort: {value}"),
-                )
-                .into_response();
-            }
-        },
-        None => None,
+    let reasoning_effort = match parse_query_enum::<ReasoningEffort>(
+        params.reasoning_effort.as_deref(),
+        "reasoning effort",
+    ) {
+        Ok(reasoning_effort) => reasoning_effort,
+        Err(error) => return error.into_response(),
     };
     let llm_result = match state.resolve_llm_client().await {
         Ok(result) => result,

lib/components/fabro-llm/src/model_test.rs

@@ -55,19 +55,20 @@ pub async fn run_model_test(
     }
 }
 
+/// Output budget for tests where reasoning or tool rounds consume completion
+/// tokens before the final answer.
+const EXPANDED_MAX_TOKENS: i64 = 1024;
+
 async fn run_basic_test(
     info: &Model,
     reasoning_effort: Option<ReasoningEffort>,
     client: Arc<Client>,
 ) -> ModelTestOutcome {
-    let params = build_basic_test_params(
+    basic_probe(
         info.id.as_str(),
         info.provider.to_string(),
         reasoning_effort,
         client,
-    );
-    basic_model_probe_outcome(
-        generate::generate(params),
         Duration::from_secs(ModelTestMode::Basic.timeout_secs()),
     )
     .await
@@ -95,8 +96,17 @@ pub async fn run_basic_model_probe_with_timeout(
     client: Arc<Client>,
     probe_timeout: Duration,
 ) -> ModelTestOutcome {
-    let params = build_basic_test_params(model_id, provider.to_string(), None, client);
+    basic_probe(model_id, provider.to_string(), None, client, probe_timeout).await
+}
 
+async fn basic_probe(
+    model_id: &str,
+    provider: String,
+    reasoning_effort: Option<ReasoningEffort>,
+    client: Arc<Client>,
+    probe_timeout: Duration,
+) -> ModelTestOutcome {
+    let params = build_basic_test_params(model_id, provider, reasoning_effort, client);
     basic_model_probe_outcome(generate::generate(params), probe_timeout).await
 }
 
@@ -106,7 +116,11 @@ fn build_basic_test_params(
     reasoning_effort: Option<ReasoningEffort>,
     client: Arc<Client>,
 ) -> GenerateParams {
-    let max_tokens = if reasoning_effort.is_some() { 1024 } else { 16 };
+    let max_tokens = if reasoning_effort.is_some() {
+        EXPANDED_MAX_TOKENS
+    } else {
+        16
+    };
     let mut params = GenerateParams::new(model_id, client)
         .provider(provider)
         .prompt("Say OK")
@@ -196,7 +210,7 @@ fn build_tools_test_params(
         )
         .tools(vec![add_tool])
         .max_tool_rounds(5)
-        .max_tokens(1024);
+        .max_tokens(EXPANDED_MAX_TOKENS);
 
     if let Some(reasoning_effort) = reasoning_effort {
         params = params.reasoning_effort(reasoning_effort);

lib/foundation/fabro-model/Cargo.toml

@@ -12,7 +12,11 @@ doctest = false
 [lints]
 workspace = true
 
+[features]
+clap = ["dep:clap"]
+
 [dependencies]
+clap = { workspace = true, optional = true }
 fabro-static.workspace = true
 http = "1"
 rust-embed.workspace = true

lib/foundation/fabro-model/src/reasoning.rs

@@ -24,6 +24,8 @@ use serde::{Deserialize, Serialize};
     strum::IntoStaticStr,
     strum::VariantArray,
 )]
+#[cfg_attr(feature = "clap", derive(clap::ValueEnum))]
+#[cfg_attr(feature = "clap", clap(rename_all = "lowercase"))]
 #[serde(rename_all = "lowercase")]
 #[strum(serialize_all = "lowercase")]
 pub enum ReasoningEffort {