From 32d6be7ea56ebac71b6e5afdab208332c4274f03 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 25 Aug 2026 14:49:34 -0400 Subject: [PATCH] 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 Claude-Session: https://claude.ai/code/session_01TDdjG18d2AHh7mFWXFkBLn --- Cargo.lock | 1 + docs/public/reference/cli.mdx | 2 +- lib/apps/fabro-cli/Cargo.toml | 2 +- lib/apps/fabro-cli/src/args.rs | 4 +- lib/apps/fabro-cli/src/commands/model.rs | 68 +++++++------------ lib/apps/fabro-cli/tests/it/cmd/model_test.rs | 62 +++++------------ .../fabro-server/src/server/handler/models.rs | 43 ++++++------ lib/components/fabro-llm/src/model_test.rs | 28 ++++++-- lib/foundation/fabro-model/Cargo.toml | 4 ++ lib/foundation/fabro-model/src/reasoning.rs | 2 + 10 files changed, 92 insertions(+), 124 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5a229fbad..f7969922c 100644 --- a/Cargo.lock +++ b/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", diff --git a/docs/public/reference/cli.mdx b/docs/public/reference/cli.mdx index 8df61ae5b..fa3b97212 100644 --- a/docs/public/reference/cli.mdx +++ b/docs/public/reference/cli.mdx @@ -696,7 +696,7 @@ fabro model test [OPTIONS] | `-j, --jobs ` | Number of model tests to run concurrently in bulk mode
Default: `4` | | `-m, --model ` | Test a specific model | | `-p, --provider ` | Filter by provider | -| `--reasoning-effort ` | Request a reasoning-effort level (`low`, `medium`, `high`, `xhigh`, or `max`) | +| `--reasoning-effort ` | Request a reasoning-effort level
Values: `low`, `medium`, `high`, `xhigh`, `max` | | `--server ` | Fabro server target: http(s) URL or absolute Unix socket path | | `--tools` | Run a multi-turn tool-use test | diff --git a/lib/apps/fabro-cli/Cargo.toml b/lib/apps/fabro-cli/Cargo.toml index 44c9982bc..fa16a7375 100644 --- a/lib/apps/fabro-cli/Cargo.toml +++ b/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" } diff --git a/lib/apps/fabro-cli/src/args.rs b/lib/apps/fabro-cli/src/args.rs index b94293295..ede9e8eb9 100644 --- a/lib/apps/fabro-cli/src/args.rs +++ b/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, } diff --git a/lib/apps/fabro-cli/src/commands/model.rs b/lib/apps/fabro-cli/src/commands/model.rs index 2f90e1f46..425fcd175 100644 --- a/lib/apps/fabro-cli/src/commands/model.rs +++ b/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, - 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, ) diff --git a/lib/apps/fabro-cli/tests/it/cmd/model_test.rs b/lib/apps/fabro-cli/tests/it/cmd/model_test.rs index 1cec2e7b4..01c57c32a 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/model_test.rs +++ b/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 - 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] diff --git a/lib/apps/fabro-server/src/server/handler/models.rs b/lib/apps/fabro-server/src/server/handler/models.rs index 6a5934274..b66180f9f 100644 --- a/lib/apps/fabro-server/src/server/handler/models.rs +++ b/lib/apps/fabro-server/src/server/handler/models.rs @@ -190,37 +190,32 @@ async fn test_providers(_auth: RequiredUser, State(state): State>) } } +fn parse_query_enum(value: Option<&str>, label: &str) -> Result, 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>, Path(id): Path, Query(params): Query, ) -> 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::( + 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, diff --git a/lib/components/fabro-llm/src/model_test.rs b/lib/components/fabro-llm/src/model_test.rs index c0e1208e0..7498e5e64 100644 --- a/lib/components/fabro-llm/src/model_test.rs +++ b/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, client: Arc, ) -> 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, 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, + client: Arc, + 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, client: Arc, ) -> 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); diff --git a/lib/foundation/fabro-model/Cargo.toml b/lib/foundation/fabro-model/Cargo.toml index 92e869f81..9c586d27e 100644 --- a/lib/foundation/fabro-model/Cargo.toml +++ b/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 diff --git a/lib/foundation/fabro-model/src/reasoning.rs b/lib/foundation/fabro-model/src/reasoning.rs index 44587df4a..56e07ad26 100644 --- a/lib/foundation/fabro-model/src/reasoning.rs +++ b/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 {