From 8ea22ef7a9dfefac310da2f86c59cfbfa1bbdde6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Halber?= Date: Thu, 13 Aug 2026 18:34:00 -0700 Subject: [PATCH 01/12] feat(scorers): create LLM scorers and classifiers --- Cargo.lock | 20 ++ Cargo.toml | 1 + README.md | 22 ++ src/functions/create.rs | 516 +++++++++++++++++++++++++++++++++ src/functions/mod.rs | 43 ++- src/functions/prompt_config.rs | 354 ++++++++++++++++++++++ src/scorers.rs | 98 ++++++- src/utils/json_object.rs | 34 +++ src/utils/mod.rs | 6 +- src/utils/structured_source.rs | 39 +++ src/utils/text_source.rs | 107 +++++++ tests/cli.rs | 30 ++ 12 files changed, 1264 insertions(+), 6 deletions(-) create mode 100644 src/functions/create.rs create mode 100644 src/functions/prompt_config.rs create mode 100644 src/utils/structured_source.rs create mode 100644 src/utils/text_source.rs diff --git a/Cargo.lock b/Cargo.lock index 3af95ea7..07ef953a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -588,6 +588,7 @@ dependencies = [ "unicode-width 0.1.14", "urlencoding", "uuid", + "yaml_serde", ] [[package]] @@ -1888,6 +1889,12 @@ dependencies = [ "libc", ] +[[package]] +name = "libyaml-rs" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2e126dda6f34391ab7b444f9922055facc83c07a910da3eb16f1e4d9c45dc777" + [[package]] name = "lingua" version = "0.1.0" @@ -4084,6 +4091,19 @@ version = "0.6.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1ffae5123b2d3fc086436f8834ae3ab053a283cfac8fe0a0b8eaae044768a4c4" +[[package]] +name = "yaml_serde" +version = "0.10.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "08c7c1b1a6a7c8a6b2741a6c21a4f8918e51899b111cfa08d1288202656e3975" +dependencies = [ + "indexmap 2.14.0", + "itoa", + "libyaml-rs", + "ryu", + "serde", +] + [[package]] name = "yoke" version = "0.8.3" diff --git a/Cargo.toml b/Cargo.toml index e8cad8b3..265c8352 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,6 +30,7 @@ reqwest = { version = "0.12.7", default-features = false, features = ["json", "r serde = { version = "1.0.210", features = ["derive"] } serde_json = "1.0.128" serde_path_to_error = "0.1.20" +yaml_serde = "0.10" toml = "0.8" sha2 = "0.10.8" strip-ansi-escapes = "0.2.0" diff --git a/README.md b/README.md index 3a80617e..f4fe8e07 100644 --- a/README.md +++ b/README.md @@ -149,9 +149,31 @@ Remove-Item -Recurse -Force (Join-Path $env:APPDATA "bt") -ErrorAction SilentlyC | `bt projects` | Manage projects (list, create, view, delete) | | `bt datasets` | Manage remote datasets (list, create, update, view, delete) | | `bt prompts` | Manage prompts (list, view, delete) | +| `bt scorers` | Manage scorers (list, create, view, invoke, delete) | | `bt sync` | Synchronize project logs between Braintrust and local NDJSON files | | `bt update` | Update bt in-place | +## `bt scorers` + +Create prompt-based LLM scorers or classifiers in the current project: + +```bash +bt scorers create "Helpfulness" \ + --model gpt-5.4-nano \ + --messages @messages.json \ + --choice-scores '{"A":1,"B":0}' + +bt scorers create "Safety label" \ + --model gpt-5.4-nano \ + --messages @messages.json \ + --classifications '["safe","unsafe"]' \ + --allow-no-match +``` + +Use `--if-exists error|ignore|replace` to control slug conflicts. Text and structured input flags accept an inline value, `@PATH`, or `-` for stdin; only one flag per command may read stdin. Use `--template-format mustache|jinja|none`; `nunjucks` and `jinja2` are accepted aliases for Jinja. + +For TypeScript and Python code scorers, use the Braintrust SDK and `bt functions push`. + ## `bt eval` **File selection:** diff --git a/src/functions/create.rs b/src/functions/create.rs new file mode 100644 index 00000000..7bb76ced --- /dev/null +++ b/src/functions/create.rs @@ -0,0 +1,516 @@ +use anyhow::{bail, Context, Result}; +use clap::{builder::BoolishValueParser, ArgGroup, Args}; +use dialoguer::Input; +use serde_json::{json, Map, Value}; + +use crate::{ + ui::{is_interactive, print_command_status, with_spinner, CommandStatus}, + utils::{merge_json_objects, read_text_source, read_yaml_object_source}, +}; + +use super::{ + api, + prompt_config::{ + parse_choice_scores_source, parse_classifications_source, validate_unit_interval, + PromptConfigArgs, + }, + IfExistsMode, ResolvedContext, +}; + +/// Create an LLM scorer or classifier. +/// +/// The generated definition matches Braintrust's prompt-function schema with +/// an `llm_classifier` parser. `--choice-scores` produces numeric scores; +/// `--classifications` produces labels. +#[derive(Debug, Clone, Args)] +#[command(group( + ArgGroup::new("output") + .required(true) + .multiple(false) + .args(["choice_scores", "classifications"]) +))] +#[command(after_help = "\ +Examples: + bt scorers create \"Helpfulness\" --model gpt-5.4-nano --messages @messages.json \\ + --choice-scores '{\"A\":1,\"B\":0}' + bt scorers create \"Correctness\" --slug correctness --model gpt-5.4-nano \\ + --messages @messages.json \\ + --choice-scores '{\"correct\":1,\"incorrect\":0}' --use-cot=false + bt scorers create \"Tone\" --model gpt-5.4-nano \\ + --messages @messages.json --choice-scores @scores.json + bt scorers create \"Safety label\" --model gpt-5.4-nano --messages @messages.json \\ + --classifications '[\"safe\",\"unsafe\"]' --template-format jinja + +TypeScript and Python code scorers: + TypeScript: projects.create({ name: \"test-project\" }).scorers.create({...}) + bt functions push scorer.ts + Python: projects.create(\"test-project\").scorers.create(...) + bt functions push scorer.py +")] +pub(crate) struct CreateArgs { + /// Scorer name. + #[arg(value_name = "NAME", conflicts_with = "name")] + name_positional: Option, + + /// Scorer name (alternative to the positional name). + #[arg(long, value_name = "NAME")] + name: Option, + + /// Unique scorer slug. Defaults to a slug generated from the name. + #[arg(long, short = 's')] + slug: Option, + + /// Scorer description. + #[arg(long, short = 'd')] + description: Option, + + /// Chat messages source: inline JSON, @PATH to read from a file, or - for + /// stdin. + #[arg(long, value_name = "SOURCE")] + messages: String, + + /// Model used by the LLM judge. + #[arg(long, short = 'm', value_name = "MODEL")] + model: String, + + #[command(flatten)] + prompt_config: PromptConfigArgs, + + /// Choice-to-score mapping for score output: inline JSON, @PATH to read + /// from a file, or - for stdin. Scores must be between 0 and 1. + #[arg(long, value_name = "SOURCE")] + choice_scores: Option, + + /// Labels for classification output: an inline JSON array, @PATH to read + /// from a file, or - for stdin. This creates an LLM classifier, which is + /// shown alongside scorers in the Braintrust UI. + #[arg(long, value_name = "SOURCE")] + classifications: Option, + + /// Allow a classifier to return no matching classification. + #[arg(long, requires = "classifications")] + allow_no_match: bool, + + /// Whether the scorer should use chain-of-thought reasoning. Defaults to + /// true; pass --use-cot=false to disable it. + #[arg( + long, + num_args = 0..=1, + default_missing_value = "true", + default_value_t = true, + value_parser = BoolishValueParser::new() + )] + use_cot: bool, + + /// Score threshold for passing, between 0 and 1. + #[arg(long, value_name = "NUMBER", conflicts_with = "classifications")] + pass_threshold: Option, + + /// Metadata as inline YAML, @PATH to a YAML file, or - for stdin. + #[arg(long, value_name = "SOURCE")] + metadata: Option, + + /// Behavior when a scorer with the same slug already exists. + #[arg(long, value_enum, default_value = "error")] + if_exists: IfExistsMode, +} + +pub(crate) async fn run(ctx: &ResolvedContext, args: &CreateArgs, json_output: bool) -> Result<()> { + let name = resolve_name(args)?; + let slug = resolve_slug(args, &name)?; + let definition = build_scorer_definition(args, &ctx.project.id, &name, &slug)?; + + let result = match with_spinner( + "Creating scorer...", + api::insert_functions(&ctx.client, std::slice::from_ref(&definition)), + ) + .await + { + Ok(result) => result, + Err(error) => { + print_command_status(CommandStatus::Error, &format!("Failed to create '{name}'")); + return Err(error); + } + }; + + let ignored = result.ignored_entries.is_some_and(|count| count > 0); + + if json_output { + println!( + "{}", + serde_json::to_string(&json!({ + "scorer": definition, + "ignored": ignored, + }))? + ); + return Ok(()); + } + + if ignored { + print_command_status( + CommandStatus::Warning, + &format!("Scorer '{name}' already exists; left it unchanged"), + ); + } else if args.if_exists == IfExistsMode::Replace { + print_command_status(CommandStatus::Success, &format!("Saved '{name}'")); + } else { + print_command_status(CommandStatus::Success, &format!("Created '{name}'")); + } + + Ok(()) +} + +fn resolve_name(args: &CreateArgs) -> Result { + let name = match (&args.name_positional, &args.name) { + (Some(_), Some(_)) => bail!("use either a positional name or --name, not both"), + (Some(name), None) | (None, Some(name)) => name.trim().to_string(), + (None, None) if is_interactive() => Input::::new() + .with_prompt("Scorer name") + .interact_text()? + .trim() + .to_string(), + (None, None) => bail!("scorer name required. Use: bt scorers create ..."), + }; + + if name.is_empty() { + bail!("scorer name cannot be empty"); + } + Ok(name) +} + +fn resolve_slug(args: &CreateArgs, name: &str) -> Result { + let slug = args + .slug + .as_deref() + .map(str::trim) + .map(ToOwned::to_owned) + .unwrap_or_else(|| slugify(name)); + if slug.is_empty() { + bail!("could not generate a slug from the scorer name; pass --slug explicitly"); + } + Ok(slug) +} + +fn slugify(value: &str) -> String { + let mut slug = String::new(); + let mut pending_separator = false; + + for character in value.trim().chars() { + if character.is_alphanumeric() { + if pending_separator && !slug.is_empty() { + slug.push('-'); + } + slug.extend(character.to_lowercase()); + pending_separator = false; + } else if !slug.is_empty() { + pending_separator = true; + } + } + + slug +} + +fn build_scorer_definition( + args: &CreateArgs, + project_id: &str, + name: &str, + slug: &str, +) -> Result { + let prompt = resolve_prompt_block(args)?; + let (function_type, parser) = resolve_output_parser(args)?; + + let mut prompt_data = json!({ + "prompt": prompt, + "parser": parser, + }) + .as_object() + .expect("prompt data is an object") + .clone(); + let prompt_config = args + .prompt_config + .build_prompt_data_patch(Some(&args.model))?; + merge_json_objects(&mut prompt_data, &prompt_config); + + let mut definition = json!({ + "project_id": project_id, + "name": name, + "slug": slug, + "function_data": { + "type": "prompt", + }, + "prompt_data": prompt_data, + "if_exists": args.if_exists.as_str(), + "function_type": function_type, + }); + + if let Some(description) = args.description.as_deref() { + definition["description"] = Value::String(description.to_string()); + } + + let metadata = resolve_metadata(args)?; + if !metadata.is_empty() { + definition["metadata"] = Value::Object(metadata); + } + + Ok(definition) +} + +fn resolve_output_parser(args: &CreateArgs) -> Result<(&'static str, Value)> { + match ( + args.choice_scores.as_deref(), + args.classifications.as_deref(), + ) { + (Some(source), None) => Ok(( + "scorer", + json!({ + "type": "llm_classifier", + "use_cot": args.use_cot, + "choice_scores": parse_choice_scores_source(source)?, + }), + )), + (None, Some(source)) => Ok(( + "classifier", + json!({ + "type": "llm_classifier", + "use_cot": args.use_cot, + "choice": parse_classifications_source(source)?, + "allow_no_match": args.allow_no_match, + }), + )), + (Some(_), Some(_)) => bail!( + "use either --choice-scores for score output or --classifications for classification output, not both" + ), + (None, None) => bail!( + "output choices required. Pass --choice-scores or --classifications " + ), + } +} + +fn resolve_metadata(args: &CreateArgs) -> Result> { + let mut metadata = match args.metadata.as_deref() { + Some(source) => read_yaml_object_source(source, "scorer metadata")?, + None => Map::new(), + }; + if let Some(pass_threshold) = args.pass_threshold { + validate_unit_interval(pass_threshold, "--pass-threshold")?; + metadata.insert("__pass_threshold".to_string(), json!(pass_threshold)); + } + Ok(metadata) +} + +fn resolve_prompt_block(args: &CreateArgs) -> Result { + let raw = read_text_source(&args.messages, "messages")?; + parse_messages(&raw) +} + +fn parse_messages(raw: &str) -> Result { + let messages: Value = serde_json::from_str(raw).context("invalid JSON in scorer messages")?; + match messages { + Value::Array(_) => Ok(json!({ "type": "chat", "messages": messages })), + _ => bail!("scorer messages must be a JSON array"), + } +} + +#[cfg(test)] +mod tests { + use clap::Parser; + + use super::*; + + #[derive(Debug, Parser)] + struct CreateArgsHarness { + #[command(flatten)] + args: CreateArgs, + } + + fn args() -> CreateArgs { + CreateArgs { + name_positional: Some("Test Helpfulness".to_string()), + name: None, + slug: None, + description: Some("Synthetic test scorer".to_string()), + messages: r#"[{"role":"user","content":"Judge {{output}}."}]"#.to_string(), + model: "gpt-test".to_string(), + prompt_config: PromptConfigArgs::default(), + choice_scores: Some(r#"{"A":1,"B":0}"#.to_string()), + classifications: None, + allow_no_match: false, + use_cot: true, + pass_threshold: None, + metadata: None, + if_exists: IfExistsMode::Error, + } + } + + #[test] + fn use_cot_defaults_to_true() { + let parsed = CreateArgsHarness::try_parse_from([ + "bt-scorers-create", + "Test scorer", + "--model", + "gpt-test", + "--messages", + r#"[{"role":"user","content":"Judge {{output}}"}]"#, + "--choice-scores", + r#"{"yes":1,"no":0}"#, + ]) + .expect("parse create args"); + + assert!(parsed.args.use_cot); + } + + #[test] + fn builds_sdk_compatible_llm_scorer_definition() { + let args = args(); + let body = build_scorer_definition( + &args, + "00000000-0000-0000-0000-000000000001", + "Test Helpfulness", + "test-helpfulness", + ) + .expect("definition"); + + assert_eq!(body["function_data"], json!({ "type": "prompt" })); + assert_eq!(body["function_type"], "scorer"); + assert_eq!(body["prompt_data"]["prompt"]["type"], "chat"); + assert_eq!(body["prompt_data"]["options"]["model"], "gpt-test"); + assert_eq!( + body["prompt_data"]["parser"], + json!({ + "type": "llm_classifier", + "use_cot": true, + "choice_scores": { "A": 1, "B": 0 }, + }) + ); + assert_eq!(body["if_exists"], "error"); + assert_eq!(body["description"], "Synthetic test scorer"); + } + + #[test] + fn builds_chat_prompt_definition() { + let args = args(); + + let body = + build_scorer_definition(&args, "test-project", "Test", "test").expect("definition"); + + assert_eq!(body["prompt_data"]["prompt"]["type"], "chat"); + assert_eq!( + body["prompt_data"]["prompt"]["messages"], + json!([{ "role": "user", "content": "Judge {{output}}." }]) + ); + } + + #[test] + fn rejects_non_array_messages() { + let mut args = args(); + args.messages = r#"{"role":"user","content":"Judge {{output}}"}"#.to_string(); + + let error = build_scorer_definition(&args, "test-project", "Test", "test") + .expect_err("messages should be an array"); + assert!(error.to_string().contains("messages must be a JSON array")); + } + + #[test] + fn rejects_non_numeric_choice_score() { + let mut args = args(); + args.choice_scores = Some(r#"{"A":"one"}"#.to_string()); + + let error = build_scorer_definition(&args, "test-project", "Test", "test") + .expect_err("string score should fail"); + assert!(error.to_string().contains("must be a number")); + } + + #[test] + fn supports_disabling_use_cot() { + let mut args = args(); + args.use_cot = false; + + let body = + build_scorer_definition(&args, "test-project", "Test", "test").expect("definition"); + assert_eq!(body["prompt_data"]["parser"]["use_cot"], false); + } + + #[test] + fn builds_classification_output() { + let mut args = args(); + args.choice_scores = None; + args.classifications = Some(r#"["safe","unsafe"]"#.to_string()); + args.allow_no_match = true; + + let body = + build_scorer_definition(&args, "test-project", "Test", "test").expect("definition"); + + assert_eq!(body["function_type"], "classifier"); + assert_eq!( + body["prompt_data"]["parser"]["choice"], + json!(["safe", "unsafe"]) + ); + assert_eq!(body["prompt_data"]["parser"]["allow_no_match"], true); + assert!(body["prompt_data"]["parser"].get("choice_scores").is_none()); + } + + #[test] + fn builds_model_params_template_metadata_and_pass_threshold() { + let parsed = CreateArgsHarness::try_parse_from([ + "bt-scorers-create", + "Test scorer", + "--model", + "gpt-test", + "--messages", + r#"[{"role":"user","content":"Judge {{output}}"}]"#, + "--choice-scores", + r#"{"yes":1,"no":0}"#, + "--temperature", + "0.1", + "--max-tokens", + "256", + "--top-p", + "0.8", + "--frequency-penalty", + "-0.25", + "--presence-penalty", + "0.5", + "--stop-sequence", + "END", + "--tool-choice", + "required", + "--reasoning-effort", + "medium", + "--verbosity", + "high", + "--template-format", + "jinja", + "--pass-threshold", + "0.7", + "--metadata", + "owner: test-team", + ]) + .expect("parse create args"); + + let body = + build_scorer_definition(&parsed.args, "test-project", "Test scorer", "test-scorer") + .expect("definition"); + let params = &body["prompt_data"]["options"]["params"]; + assert_eq!(params["temperature"], 0.1); + assert_eq!(params["max_tokens"], 256); + assert_eq!(params["top_p"], 0.8); + assert_eq!(params["frequency_penalty"], -0.25); + assert_eq!(params["presence_penalty"], 0.5); + assert_eq!(params["stop"], json!(["END"])); + assert_eq!(params["tool_choice"], "required"); + assert_eq!(params["reasoning_effort"], "medium"); + assert_eq!(params["verbosity"], "high"); + assert_eq!(body["prompt_data"]["template_format"], "nunjucks"); + assert_eq!(body["metadata"]["owner"], "test-team"); + assert_eq!(body["metadata"]["__pass_threshold"], 0.7); + } + + #[test] + fn slugify_normalizes_name() { + assert_eq!( + slugify(" Test Helpfulness / Judge "), + "test-helpfulness-judge" + ); + assert_eq!(slugify("Already--Separated"), "already-separated"); + } +} diff --git a/src/functions/mod.rs b/src/functions/mod.rs index d055c231..a43e2618 100644 --- a/src/functions/mod.rs +++ b/src/functions/mod.rs @@ -13,9 +13,11 @@ use crate::{ }; pub(crate) mod api; +pub(crate) mod create; mod delete; mod invoke; mod list; +pub(crate) mod prompt_config; mod pull; mod push; pub(crate) mod report; @@ -114,6 +116,9 @@ fn build_web_path(function: &Function) -> String { match function.function_type.as_deref() { Some("tool") => format!("tools?pr={}", urlencoding::encode(id)), Some("scorer") => format!("scorers/{}", urlencoding::encode(id)), + Some("classifier") if function.prompt_data.is_some() => { + format!("scorers/{}", urlencoding::encode(id)) + } Some("classifier") => { let xact_id = function._xact_id.as_deref().unwrap_or(""); format!( @@ -177,7 +182,7 @@ pub struct FunctionArgs { } #[derive(Debug, Clone, Subcommand)] -enum FunctionCommands { +pub(crate) enum FunctionCommands { /// List all in the current project List, /// View a function's details @@ -608,8 +613,16 @@ pub(crate) async fn select_function_interactive( } pub async fn run_typed(base: BaseArgs, args: FunctionArgs, kind: FunctionTypeFilter) -> Result<()> { + run_typed_command(base, args.command, kind).await +} + +pub(crate) async fn run_typed_command( + base: BaseArgs, + command: Option, + kind: FunctionTypeFilter, +) -> Result<()> { let ft = Some(kind); - match args.command { + match command { Some(FunctionCommands::View(v)) => match v.selector()? { ViewSelector::Id(id) => { let auth_ctx = resolve_auth_context(&base).await?; @@ -652,6 +665,12 @@ pub async fn run_typed(base: BaseArgs, args: FunctionArgs, kind: FunctionTypeFil } } +pub(crate) async fn run_scorer_create(base: BaseArgs, args: create::CreateArgs) -> Result<()> { + let json_output = base.json; + let ctx = resolve_context(&base).await?; + create::run(&ctx, &args, json_output).await +} + pub async fn run(base: BaseArgs, args: FunctionsArgs) -> Result<()> { let function_type = args.function_type; match args.command { @@ -1085,6 +1104,26 @@ mod tests { assert!(err.to_string().contains("either --id or a slug")); } + #[test] + fn prompt_classifier_web_path_uses_scorers_page() { + let function = Function { + id: "fn_test_classifier".to_string(), + name: "Test classifier".to_string(), + slug: "test-classifier".to_string(), + project_id: "test-project".to_string(), + description: None, + function_type: Some("classifier".to_string()), + prompt_data: Some(serde_json::json!({"parser": {"choice": ["a", "b"]}})), + function_data: Some(serde_json::json!({"type": "prompt"})), + tags: None, + metadata: None, + created: None, + _xact_id: None, + }; + + assert_eq!(build_web_path(&function), "scorers/fn_test_classifier"); + } + #[test] fn function_selection_label_includes_slug_when_name_differs() { let function = Function { diff --git a/src/functions/prompt_config.rs b/src/functions/prompt_config.rs new file mode 100644 index 00000000..cc3ef6cc --- /dev/null +++ b/src/functions/prompt_config.rs @@ -0,0 +1,354 @@ +use std::collections::HashSet; + +use anyhow::{bail, Context, Result}; +use clap::{Args, ValueEnum}; +use serde_json::{json, Map, Number, Value}; + +use crate::utils::read_text_source; + +#[derive(Debug, Clone, Default, Args)] +pub(crate) struct PromptConfigArgs { + /// Sampling temperature. + #[arg(long, value_name = "NUMBER")] + temperature: Option, + + /// Maximum number of generated tokens. + #[arg(long, value_name = "N")] + max_tokens: Option, + + /// Nucleus sampling probability. + #[arg(long, value_name = "NUMBER")] + top_p: Option, + + /// Frequency penalty. + #[arg(long, value_name = "NUMBER", allow_hyphen_values = true)] + frequency_penalty: Option, + + /// Presence penalty. + #[arg(long, value_name = "NUMBER", allow_hyphen_values = true)] + presence_penalty: Option, + + /// Stop sequence. Repeat this flag to specify multiple sequences. + #[arg(long, value_name = "TEXT", action = clap::ArgAction::Append)] + stop_sequence: Vec, + + /// Tool choice: auto, none, required, or a specific function name. + #[arg(long, value_name = "CHOICE")] + tool_choice: Option, + + /// Reasoning effort for supported models. + #[arg(long, value_enum)] + reasoning_effort: Option, + + /// Response verbosity for supported models. + #[arg(long, value_enum)] + verbosity: Option, + + /// Prompt template syntax. Jinja is stored using Braintrust's `nunjucks` + /// format; `nunjucks` and `jinja2` are accepted aliases. + #[arg(long, value_enum, value_name = "FORMAT")] + template_format: Option, +} + +#[derive(Debug, Clone, Copy, ValueEnum)] +enum ReasoningEffort { + None, + Minimal, + Low, + Medium, + High, +} + +impl ReasoningEffort { + fn as_str(self) -> &'static str { + match self { + Self::None => "none", + Self::Minimal => "minimal", + Self::Low => "low", + Self::Medium => "medium", + Self::High => "high", + } + } +} + +#[derive(Debug, Clone, Copy, ValueEnum)] +enum Verbosity { + Low, + Medium, + High, +} + +impl Verbosity { + fn as_str(self) -> &'static str { + match self { + Self::Low => "low", + Self::Medium => "medium", + Self::High => "high", + } + } +} + +#[derive(Debug, Clone, Copy, ValueEnum)] +enum TemplateFormat { + Mustache, + #[value(name = "jinja", alias = "nunjucks", alias = "jinja2")] + Nunjucks, + None, +} + +impl TemplateFormat { + fn as_str(self) -> &'static str { + match self { + Self::Mustache => "mustache", + Self::Nunjucks => "nunjucks", + Self::None => "none", + } + } +} + +impl PromptConfigArgs { + /// Build a partial `prompt_data` object matching the app's prompt schema. + pub(crate) fn build_prompt_data_patch( + &self, + model: Option<&str>, + ) -> Result> { + let mut prompt_data = Map::new(); + let mut options = Map::new(); + let mut params = Map::new(); + + if let Some(model) = model { + let model = model.trim(); + if model.is_empty() { + bail!("--model cannot be empty"); + } + options.insert("model".to_string(), Value::String(model.to_string())); + } + + insert_optional_number(&mut params, "temperature", self.temperature)?; + if let Some(max_tokens) = self.max_tokens { + params.insert("max_tokens".to_string(), Value::Number(max_tokens.into())); + } + if let Some(top_p) = self.top_p { + validate_unit_interval(top_p, "--top-p")?; + insert_number(&mut params, "top_p", top_p, "--top-p")?; + } + insert_optional_number(&mut params, "frequency_penalty", self.frequency_penalty)?; + insert_optional_number(&mut params, "presence_penalty", self.presence_penalty)?; + + if !self.stop_sequence.is_empty() { + params.insert( + "stop".to_string(), + Value::Array( + self.stop_sequence + .iter() + .map(|value| Value::String(value.clone())) + .collect(), + ), + ); + } + + if let Some(tool_choice) = self.tool_choice.as_deref() { + let tool_choice = tool_choice.trim(); + if tool_choice.is_empty() { + bail!("--tool-choice cannot be empty"); + } + let value = match tool_choice { + "auto" | "none" | "required" => Value::String(tool_choice.to_string()), + function_name => json!({ + "type": "function", + "function": { "name": function_name }, + }), + }; + params.insert("tool_choice".to_string(), value); + } + + if let Some(reasoning_effort) = self.reasoning_effort { + params.insert( + "reasoning_effort".to_string(), + Value::String(reasoning_effort.as_str().to_string()), + ); + } + if let Some(verbosity) = self.verbosity { + params.insert( + "verbosity".to_string(), + Value::String(verbosity.as_str().to_string()), + ); + } + + if !params.is_empty() { + options.insert("params".to_string(), Value::Object(params)); + } + if !options.is_empty() { + prompt_data.insert("options".to_string(), Value::Object(options)); + } + if let Some(template_format) = self.template_format { + prompt_data.insert( + "template_format".to_string(), + Value::String(template_format.as_str().to_string()), + ); + } + + Ok(prompt_data) + } +} + +fn insert_optional_number( + target: &mut Map, + key: &str, + value: Option, +) -> Result<()> { + if let Some(value) = value { + insert_number(target, key, value, &format!("--{}", key.replace('_', "-")))?; + } + Ok(()) +} + +fn insert_number( + target: &mut Map, + key: &str, + value: f64, + label: &str, +) -> Result<()> { + let number = + Number::from_f64(value).ok_or_else(|| anyhow::anyhow!("{label} must be finite"))?; + target.insert(key.to_string(), Value::Number(number)); + Ok(()) +} + +pub(crate) fn validate_unit_interval(value: f64, label: &str) -> Result<()> { + if !value.is_finite() || !(0.0..=1.0).contains(&value) { + bail!("{label} must be between 0 and 1"); + } + Ok(()) +} + +pub(crate) fn parse_choice_scores_source(source: &str) -> Result> { + let raw = read_text_source(source, "choice scores")?; + let value: Value = serde_json::from_str(&raw).context("invalid JSON in choice scores")?; + let scores = match value { + Value::Object(scores) => scores, + _ => bail!("choice scores must be a JSON object mapping choices to numeric scores"), + }; + if scores.is_empty() { + bail!("choice scores cannot be empty"); + } + for (choice, score) in &scores { + if choice.trim().is_empty() { + bail!("choice score labels cannot be empty"); + } + let Some(score) = score.as_f64() else { + bail!("score for choice '{choice}' must be a number"); + }; + validate_unit_interval(score, &format!("score for choice '{choice}'"))?; + } + Ok(scores) +} + +pub(crate) fn parse_classifications_source(source: &str) -> Result> { + let raw = read_text_source(source, "classifications")?; + let value: Value = serde_json::from_str(&raw).context("invalid JSON in classifications")?; + let choices = match value { + Value::Array(choices) => choices, + _ => bail!("classifications must be a JSON array of strings"), + }; + if choices.is_empty() { + bail!("classifications cannot be empty"); + } + + let mut seen = HashSet::new(); + choices + .into_iter() + .map(|choice| { + let Value::String(choice) = choice else { + bail!("every classification must be a string"); + }; + let choice = choice.trim(); + if choice.is_empty() { + bail!("classifications cannot contain an empty label"); + } + if !seen.insert(choice.to_string()) { + bail!("classification labels must be unique; found '{choice}' more than once"); + } + Ok(Value::String(choice.to_string())) + }) + .collect() +} + +#[cfg(test)] +mod tests { + use clap::Parser; + + use super::*; + + #[derive(Debug, Parser)] + struct Harness { + #[command(flatten)] + config: PromptConfigArgs, + } + + #[test] + fn builds_web_ui_compatible_prompt_configuration() { + let args = Harness::try_parse_from([ + "test", + "--temperature", + "0.2", + "--max-tokens", + "512", + "--top-p", + "0.9", + "--frequency-penalty", + "-0.5", + "--presence-penalty", + "0.25", + "--stop-sequence", + "END", + "--stop-sequence", + "DONE", + "--tool-choice", + "test_tool", + "--reasoning-effort", + "high", + "--verbosity", + "low", + "--template-format", + "jinja", + ]) + .expect("parse arguments"); + + let patch = args + .config + .build_prompt_data_patch(Some("gpt-test")) + .expect("prompt data"); + assert_eq!(patch["options"]["model"], "gpt-test"); + assert_eq!(patch["options"]["params"]["temperature"], 0.2); + assert_eq!(patch["options"]["params"]["max_tokens"], 512); + assert_eq!(patch["options"]["params"]["top_p"], 0.9); + assert_eq!(patch["options"]["params"]["frequency_penalty"], -0.5); + assert_eq!(patch["options"]["params"]["presence_penalty"], 0.25); + assert_eq!(patch["options"]["params"]["stop"], json!(["END", "DONE"])); + assert_eq!( + patch["options"]["params"]["tool_choice"], + json!({"type": "function", "function": {"name": "test_tool"}}) + ); + assert_eq!(patch["options"]["params"]["reasoning_effort"], "high"); + assert_eq!(patch["options"]["params"]["verbosity"], "low"); + assert_eq!(patch["template_format"], "nunjucks"); + } + + #[test] + fn validates_scores_against_api_range() { + let error = parse_choice_scores_source(r#"{"bad":1.5}"#) + .expect_err("out-of-range score should fail"); + assert!(error.to_string().contains("between 0 and 1")); + } + + #[test] + fn parses_unique_classification_labels() { + let choices = + parse_classifications_source(r#"["safe","unsafe"]"#).expect("classifications"); + assert_eq!( + choices, + json!(["safe", "unsafe"]).as_array().unwrap().clone() + ); + } +} diff --git a/src/scorers.rs b/src/scorers.rs index 842240b3..58c0083c 100644 --- a/src/scorers.rs +++ b/src/scorers.rs @@ -1,10 +1,102 @@ use anyhow::Result; +use clap::{Args, Subcommand}; use crate::args::BaseArgs; -use crate::functions::{self, FunctionArgs, FunctionTypeFilter}; +use crate::functions::{self, FunctionCommands, FunctionTypeFilter}; -pub type ScorersArgs = FunctionArgs; +#[derive(Debug, Clone, Args)] +#[command(after_help = "\ +Examples: + bt scorers list + bt scorers view my-scorer + bt scorers create \"Helpfulness\" --model gpt-5.4-nano --messages @messages.json \\ + --choice-scores '{\"A\":1,\"B\":0}' + bt scorers delete my-scorer + +TypeScript and Python code scorers: + TypeScript: projects.create({ name: \"test-project\" }).scorers.create({...}) + bt functions push scorer.ts + Python: projects.create(\"test-project\").scorers.create(...) + bt functions push scorer.py +")] +pub struct ScorersArgs { + #[command(subcommand)] + command: Option, +} + +#[derive(Debug, Clone, Subcommand)] +enum ScorersCommands { + /// Create an LLM scorer or classifier + Create(Box), + #[command(flatten)] + Function(FunctionCommands), +} pub async fn run(base: BaseArgs, args: ScorersArgs) -> Result<()> { - functions::run_typed(base, args, FunctionTypeFilter::Scorer).await + match args.command { + Some(ScorersCommands::Create(create)) => functions::run_scorer_create(base, *create).await, + Some(ScorersCommands::Function(command)) => { + functions::run_typed_command(base, Some(command), FunctionTypeFilter::Scorer).await + } + None => functions::run_typed_command(base, None, FunctionTypeFilter::Scorer).await, + } +} + +#[cfg(test)] +mod tests { + use clap::Parser; + + use super::*; + + #[derive(Debug, Parser)] + struct ScorersArgsHarness { + #[command(flatten)] + args: ScorersArgs, + } + + #[test] + fn parses_create_scorer() { + let parsed = ScorersArgsHarness::try_parse_from([ + "bt-scorers", + "create", + "Test scorer", + "--model", + "gpt-test", + "--messages", + r#"[{"role":"user","content":"Judge {{output}}"}]"#, + "--choice-scores", + r#"{"yes":1,"no":0}"#, + "--use-cot=false", + "--if-exists", + "replace", + ]) + .expect("parse create"); + + assert!(matches!( + parsed.args.command, + Some(ScorersCommands::Create(_)) + )); + } + + #[test] + fn parses_create_classifier() { + let parsed = ScorersArgsHarness::try_parse_from([ + "bt-scorers", + "create", + "Test classifier", + "--model", + "gpt-test", + "--messages", + r#"[{"role":"user","content":"Classify {{output}}"}]"#, + "--classifications", + r#"["safe","unsafe"]"#, + "--allow-no-match", + ]) + .expect("parse create classifier"); + + assert!(matches!( + parsed.args.command, + Some(ScorersCommands::Create(_)) + )); + } } diff --git a/src/utils/json_object.rs b/src/utils/json_object.rs index 20bc850c..4f2ee3d8 100644 --- a/src/utils/json_object.rs +++ b/src/utils/json_object.rs @@ -1,5 +1,18 @@ use serde_json::{Map, Value}; +pub(crate) fn merge_json_objects(target: &mut Map, source: &Map) { + for (key, value) in source { + match (target.get_mut(key), value) { + (Some(Value::Object(target_inner)), Value::Object(source_inner)) => { + merge_json_objects(target_inner, source_inner); + } + _ => { + target.insert(key.clone(), value.clone()); + } + } + } +} + pub(crate) fn lookup_object_path<'a, P>( object: &'a Map, path: &[P], @@ -20,6 +33,27 @@ mod tests { use super::*; + #[test] + fn merge_json_objects_deep_merges_nested_maps() { + let mut target = json!({ + "prompt_data": { "options": { "model": "gpt-test" } } + }) + .as_object() + .expect("object") + .clone(); + let source = json!({ + "prompt_data": { "options": { "params": { "temperature": 0 } } } + }) + .as_object() + .expect("object") + .clone(); + + merge_json_objects(&mut target, &source); + + assert_eq!(target["prompt_data"]["options"]["model"], "gpt-test"); + assert_eq!(target["prompt_data"]["options"]["params"]["temperature"], 0); + } + #[test] fn lookup_object_path_finds_nested_values() { let object = json!({ diff --git a/src/utils/mod.rs b/src/utils/mod.rs index adb19676..1429bebe 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -6,6 +6,8 @@ mod ids; mod json_object; mod plurals; mod profile; +mod structured_source; +mod text_source; pub(crate) use app_url::{app_project_url, app_project_url_with_encoded_path}; pub use duration::parse_duration_to_seconds; @@ -14,6 +16,8 @@ pub use fs_atomic::{ }; pub use git::GitRepo; pub(crate) use ids::new_uuid_id; -pub(crate) use json_object::lookup_object_path; +pub(crate) use json_object::{lookup_object_path, merge_json_objects}; pub use plurals::pluralize; pub(crate) use profile::{profile_author_slug, resolve_profile_info, sanitize_name_segment}; +pub(crate) use structured_source::read_yaml_object_source; +pub(crate) use text_source::read_text_source; diff --git a/src/utils/structured_source.rs b/src/utils/structured_source.rs new file mode 100644 index 00000000..fd5e0818 --- /dev/null +++ b/src/utils/structured_source.rs @@ -0,0 +1,39 @@ +use anyhow::{bail, Context, Result}; +use serde_json::{Map, Value}; + +use super::read_text_source; + +pub(crate) fn read_yaml_object_source( + source: &str, + description: &str, +) -> Result> { + let raw = read_text_source(source, description)?; + let value: Value = + yaml_serde::from_str(&raw).with_context(|| format!("invalid YAML in {description}"))?; + match value { + Value::Object(object) => Ok(object), + _ => bail!("{description} must be a YAML mapping/object"), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parses_inline_yaml_object() { + let value = + read_yaml_object_source("owner: test-team\nsettings:\n enabled: true\n", "metadata") + .expect("metadata"); + + assert_eq!(value["owner"], "test-team"); + assert_eq!(value["settings"]["enabled"], true); + } + + #[test] + fn rejects_yaml_array() { + let error = + read_yaml_object_source("- one\n- two\n", "metadata").expect_err("array should fail"); + assert!(error.to_string().contains("mapping/object")); + } +} diff --git a/src/utils/text_source.rs b/src/utils/text_source.rs new file mode 100644 index 00000000..1f2ce754 --- /dev/null +++ b/src/utils/text_source.rs @@ -0,0 +1,107 @@ +use std::io::Read; +use std::sync::Mutex; + +use anyhow::{bail, Context, Result}; + +/// Which label drained stdin, so a second `-` fails loudly instead of reading "". +static STDIN_READER: Mutex> = Mutex::new(None); + +/// Resolve inline text, an `@PATH` file reference, or `-` for stdin. +/// +/// A leading literal `@` can be escaped as `@@`. Only one source per invocation +/// may read from stdin. +pub(crate) fn read_text_source(value: &str, label: &str) -> Result { + read_text_source_with_stdin_guard(value, label, &STDIN_READER) +} + +fn read_text_source_with_stdin_guard( + value: &str, + label: &str, + stdin_reader: &Mutex>, +) -> Result { + if value == "-" { + // The second reader would otherwise see "" and call it malformed input. + let mut reader = stdin_reader + .lock() + .map_err(|_| anyhow::anyhow!("stdin guard poisoned"))?; + if let Some(previous) = reader.as_deref() { + bail!("stdin was already read for {previous}; only one source can be '-'"); + } + *reader = Some(label.to_string()); + drop(reader); + + let mut content = String::new(); + std::io::stdin() + .read_to_string(&mut content) + .with_context(|| format!("failed to read {label} from stdin"))?; + return Ok(content); + } + + if let Some(literal) = value.strip_prefix("@@") { + return Ok(format!("@{literal}")); + } + + if let Some(path) = value.strip_prefix('@') { + if path.is_empty() { + bail!("{label} file path cannot be empty after '@'"); + } + return std::fs::read_to_string(path) + .with_context(|| format!("failed to read {label} file {path}")); + } + + Ok(value.to_string()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn returns_inline_text() { + assert_eq!( + read_text_source("Judge the answer.", "prompt").expect("inline prompt"), + "Judge the answer." + ); + } + + #[test] + fn reads_at_prefixed_file() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("judge.md"); + std::fs::write(&path, "Judge from a file.\n").expect("write prompt"); + + let source = format!("@{}", path.display()); + assert_eq!( + read_text_source(&source, "prompt").expect("file prompt"), + "Judge from a file.\n" + ); + } + + #[test] + fn double_at_escapes_literal_at() { + assert_eq!( + read_text_source("@@mention", "prompt").expect("literal prompt"), + "@mention" + ); + } + + #[test] + fn rejects_empty_file_reference() { + let error = read_text_source("@", "prompt").expect_err("empty path should fail"); + assert!(error.to_string().contains("cannot be empty")); + } + + #[test] + fn rejects_a_second_stdin_source() { + // A local guard avoids draining the suite's shared stdin; the rejection + // happens before any read. + let guard = Mutex::new(Some("metadata".to_string())); + + let error = read_text_source_with_stdin_guard("-", "patch", &guard) + .expect_err("second stdin source should fail"); + assert_eq!( + error.to_string(), + "stdin was already read for metadata; only one source can be '-'" + ); + } +} diff --git a/tests/cli.rs b/tests/cli.rs index 50f67c9e..89213adf 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -940,6 +940,36 @@ fn trace_setup_keeps_each_agents_persistent_selection_independent() { ); } +#[test] +fn scorers_create_help_includes_llm_judge_configuration() { + bt_command() + .args(["scorers", "create", "--help"]) + .assert() + .success() + .stdout(predicate::str::contains("--messages ")) + .stdout(predicate::str::contains("--model")) + .stdout(predicate::str::contains("--temperature")) + .stdout(predicate::str::contains("--max-tokens")) + .stdout(predicate::str::contains("--top-p")) + .stdout(predicate::str::contains("--frequency-penalty")) + .stdout(predicate::str::contains("--presence-penalty")) + .stdout(predicate::str::contains("--stop-sequence")) + .stdout(predicate::str::contains("--tool-choice")) + .stdout(predicate::str::contains("--reasoning-effort")) + .stdout(predicate::str::contains("--verbosity")) + .stdout(predicate::str::contains("--template-format")) + .stdout(predicate::str::contains("--choice-scores")) + .stdout(predicate::str::contains("--classifications")) + .stdout(predicate::str::contains("--use-cot")) + .stdout(predicate::str::contains("--pass-threshold")) + .stdout(predicate::str::contains("--metadata")) + .stdout(predicate::str::contains("--if-exists")) + .stdout(predicate::str::contains("TypeScript: projects.create")) + .stdout(predicate::str::contains("Python: projects.create")) + .stdout(predicate::str::contains("bt functions push scorer.ts")) + .stdout(predicate::str::contains("bt functions push scorer.py")); +} + #[test] fn topics_report_help_accepts_global_org_short_conflict_free() { bt_command() From 4b2b7b0d35a89c94cf0859af9c50bdb69721c4db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Halber?= Date: Fri, 14 Aug 2026 15:10:31 -0700 Subject: [PATCH 02/12] chore: AGENTS.md had imprecise guidelines about needing a timestamp in BTQL requests --- AGENTS.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 0c838320..4502de59 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -14,10 +14,10 @@ ## BTQL Safety -- Every BTQL query must include either: - - a timestamp filter (for example, `created >= NOW() - INTERVAL ...` or `created >= ""`), or - - a `root_span_id` filter. -- Do not run BTQL queries that lack both constraints. +- BTQL queries over `project_logs(...)` or the combined `project(...)` source must include a useful segment-elimination constraint: + - a selective range on `created`, `_xact_id`, or `_pagination_key`; or + - scoping to specific `root_span_id` or `id` values. +- This requirement does not apply to other object sources such as `project_functions(...)`, `project_prompts(...)`, `dataset(...)`, or `experiment(...)`. ## Tooling From 9d90b7025bceb327a46552f7180d1b1fdb31bf2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Halber?= Date: Fri, 14 Aug 2026 15:20:37 -0700 Subject: [PATCH 03/12] chore(scorer): show classification scorer with `bt scorers list` like in the web ui --- src/functions/api.rs | 40 +++++++++++++++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/src/functions/api.rs b/src/functions/api.rs index 57f8829c..63b765ae 100644 --- a/src/functions/api.rs +++ b/src/functions/api.rs @@ -68,17 +68,29 @@ pub async fn list_functions( project_id: &str, function_type: Option<&str>, ) -> Result> { + let query = list_functions_query(project_id, function_type); + let response = client.btql::(&query).await?; + + Ok(response.data) +} + +fn list_functions_query(project_id: &str, function_type: Option<&str>) -> String { let pid = escape_sql(project_id); - let query = match function_type { + let type_filter = match function_type { + // Match the web UI's Scorers tab: label-producing classifiers appear + // alongside score-producing scorers, while topic maps do not. + Some("scorer") => "function_type IN ('scorer', 'classifier') \ + AND COALESCE(function_data.type, '') != 'topic_map' \ + AND (origin IS NULL OR NOT COALESCE(origin.internal, FALSE))" + .to_string(), Some(ft) => { let ft = escape_sql(ft); - format!("SELECT * FROM project_functions('{pid}') WHERE function_type = '{ft}'") + format!("function_type = '{ft}'") } - None => format!("SELECT * FROM project_functions('{pid}')"), + None => return format!("SELECT * FROM project_functions('{pid}')"), }; - let response = client.btql::(&query).await?; - Ok(response.data) + format!("SELECT * FROM project_functions('{pid}') WHERE {type_filter}") } pub async fn get_function_by_slug( @@ -277,6 +289,24 @@ fn ignored_count(raw: &Value) -> Option { mod tests { use super::*; + #[test] + fn scorer_list_query_matches_web_ui_filter() { + let query = list_functions_query("test-project-id", Some("scorer")); + + assert!(query.contains("function_type IN ('scorer', 'classifier')")); + assert!(query.contains("COALESCE(function_data.type, '') != 'topic_map'")); + assert!(query.contains("origin IS NULL")); + assert!(query.contains("origin.internal")); + } + + #[test] + fn non_scorer_list_query_keeps_exact_type_filter() { + let query = list_functions_query("test-project-id", Some("tool")); + + assert!(query.contains("function_type = 'tool'")); + assert!(!query.contains("classifier")); + } + #[test] fn ignored_count_extracts_canonical_shape() { let first = serde_json::json!({ "ignored_count": 3 }); From 9c9670c65b8015711cca0326df532399216bcce4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Halber?= Date: Fri, 14 Aug 2026 15:28:16 -0700 Subject: [PATCH 04/12] fix(scorer): `--if-exists ignore --json` correctly shows the scorers has been ignored when it happens --- src/functions/api.rs | 58 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-) diff --git a/src/functions/api.rs b/src/functions/api.rs index 63b765ae..0eb6c772 100644 --- a/src/functions/api.rs +++ b/src/functions/api.rs @@ -271,7 +271,8 @@ pub async fn insert_functions( .context("failed to insert functions")?; Ok(InsertFunctionsResult { - ignored_entries: ignored_count(&raw), + ignored_entries: ignored_count(&raw) + .or_else(|| ignored_count_from_function_results(&raw, functions)), }) } @@ -285,6 +286,26 @@ fn ignored_count(raw: &Value) -> Option { .and_then(|count| usize::try_from(count).ok()) } +fn ignored_count_from_function_results(raw: &Value, requests: &[Value]) -> Option { + let results = raw.get("functions")?.as_array()?; + if results.len() != requests.len() { + return None; + } + + results + .iter() + .zip(requests) + .try_fold(0usize, |count, (result, request)| { + let should_ignore = request.get("if_exists").and_then(Value::as_str) == Some("ignore"); + if !should_ignore { + return Some(count); + } + + let found_existing = result.get("found_existing")?.as_bool()?; + Some(count + usize::from(found_existing)) + }) +} + #[cfg(test)] mod tests { use super::*; @@ -321,6 +342,41 @@ mod tests { assert_eq!(ignored_count(&serde_json::json!({})), None); } + #[test] + fn derives_ignored_count_from_found_existing_results() { + let requests = vec![ + serde_json::json!({ "slug": "first", "if_exists": "ignore" }), + serde_json::json!({ "slug": "second", "if_exists": "replace" }), + serde_json::json!({ "slug": "third", "if_exists": "ignore" }), + ]; + let response = serde_json::json!({ + "functions": [ + { "slug": "first", "found_existing": true }, + { "slug": "second", "found_existing": true }, + { "slug": "third", "found_existing": false }, + ] + }); + + assert_eq!( + ignored_count_from_function_results(&response, &requests), + Some(1) + ); + } + + #[test] + fn ignored_count_fallback_rejects_mismatched_response_length() { + let requests = vec![serde_json::json!({ + "slug": "first", + "if_exists": "ignore" + })]; + let response = serde_json::json!({ "functions": [] }); + + assert_eq!( + ignored_count_from_function_results(&response, &requests), + None + ); + } + #[test] fn insert_functions_body_wraps_functions_array() { let functions = vec![serde_json::json!({ "slug": "demo" })]; From 4fc4dfab02563bedf1eee527c495539aa997cc0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Halber?= Date: Fri, 14 Aug 2026 15:39:35 -0700 Subject: [PATCH 05/12] chore(scorer): scorer creation returns the json of the backend response instead of what bt asked to be created --- src/functions/api.rs | 43 +++++++++++++++++++++++++++++++++++++++++ src/functions/create.rs | 10 +++++++++- 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/src/functions/api.rs b/src/functions/api.rs index 0eb6c772..ea5d6bd9 100644 --- a/src/functions/api.rs +++ b/src/functions/api.rs @@ -58,9 +58,27 @@ pub struct CodeUploadSlot { pub bundle_id: String, } +#[derive(Debug, Clone, Deserialize)] +pub struct InsertedFunctionResult { + pub id: String, + pub project_id: String, + pub slug: String, + pub found_existing: bool, +} + #[derive(Debug, Clone)] pub struct InsertFunctionsResult { pub ignored_entries: Option, + pub xact_id: Option, + pub functions: Vec, +} + +#[derive(Debug, Deserialize)] +struct InsertFunctionsResponse { + #[serde(default)] + xact_id: Option, + #[serde(default)] + functions: Vec, } pub async fn list_functions( @@ -270,9 +288,14 @@ pub async fn insert_functions( .await .context("failed to insert functions")?; + let response: InsertFunctionsResponse = serde_json::from_value(raw.clone()) + .context("unexpected insert-functions response shape")?; + Ok(InsertFunctionsResult { ignored_entries: ignored_count(&raw) .or_else(|| ignored_count_from_function_results(&raw, functions)), + xact_id: response.xact_id, + functions: response.functions, }) } @@ -377,6 +400,26 @@ mod tests { ); } + #[test] + fn parses_insert_function_operation_fields() { + let response: InsertFunctionsResponse = serde_json::from_value(serde_json::json!({ + "xact_id": "1000000000000000001", + "functions": [{ + "id": "fn_test_scorer", + "project_id": "test-project", + "slug": "test-scorer", + "found_existing": true + }] + })) + .expect("insert response"); + + assert_eq!(response.xact_id.as_deref(), Some("1000000000000000001")); + assert_eq!(response.functions[0].id, "fn_test_scorer"); + assert_eq!(response.functions[0].project_id, "test-project"); + assert_eq!(response.functions[0].slug, "test-scorer"); + assert!(response.functions[0].found_existing); + } + #[test] fn insert_functions_body_wraps_functions_array() { let functions = vec![serde_json::json!({ "slug": "demo" })]; diff --git a/src/functions/create.rs b/src/functions/create.rs index 7bb76ced..c9192cbf 100644 --- a/src/functions/create.rs +++ b/src/functions/create.rs @@ -136,10 +136,18 @@ pub(crate) async fn run(ctx: &ResolvedContext, args: &CreateArgs, json_output: b let ignored = result.ignored_entries.is_some_and(|count| count > 0); if json_output { + let function = result + .functions + .first() + .context("insert-functions response did not include the scorer identity")?; println!( "{}", serde_json::to_string(&json!({ - "scorer": definition, + "id": function.id, + "project_id": function.project_id, + "slug": function.slug, + "version": result.xact_id, + "found_existing": function.found_existing, "ignored": ignored, }))? ); From cad9569426cc33b9b9e0f9913a617ea458f57be0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Halber?= Date: Fri, 14 Aug 2026 16:11:56 -0700 Subject: [PATCH 06/12] fix(scorer): invoking a scorer respects `--json` and provider invalid_api_key responses are classified as user/provider errors, not Braintrust authentication failures --- src/functions/invoke.rs | 26 ++++++++++++++- src/main.rs | 74 +++++++++++++++++++++++++++++++++++++++-- src/scorers.rs | 19 +++++++++++ 3 files changed, 115 insertions(+), 4 deletions(-) diff --git a/src/functions/invoke.rs b/src/functions/invoke.rs index 2815ca0e..e7b294a1 100644 --- a/src/functions/invoke.rs +++ b/src/functions/invoke.rs @@ -42,6 +42,10 @@ impl InvokeArgs { } } +fn resolve_mode(mode: Option<&str>, json_output: bool) -> Option<&str> { + mode.or(json_output.then_some("json")) +} + fn resolve_input(input_arg: &Option) -> Result> { if let Some(raw) = input_arg { let parsed: Value = serde_json::from_str(raw).context("invalid JSON in --input")?; @@ -97,7 +101,7 @@ pub async fn run( .collect(); body["messages"] = json!(messages); } - if let Some(mode) = &args.mode { + if let Some(mode) = resolve_mode(args.mode.as_deref(), json_output) { body["mode"] = json!(mode); } if let Some(version) = &args.version { @@ -118,3 +122,23 @@ pub async fn run( Ok(()) } + +#[cfg(test)] +mod tests { + use super::resolve_mode; + + #[test] + fn json_output_requests_json_invoke_mode() { + assert_eq!(resolve_mode(None, true), Some("json")); + } + + #[test] + fn explicit_invoke_mode_takes_precedence_over_json_output() { + assert_eq!(resolve_mode(Some("text"), true), Some("text")); + } + + #[test] + fn default_output_does_not_set_invoke_mode() { + assert_eq!(resolve_mode(None, false), None); + } +} diff --git a/src/main.rs b/src/main.rs index 69d49a49..12cf43b1 100644 --- a/src/main.rs +++ b/src/main.rs @@ -250,13 +250,21 @@ enum ExitCode { User = 4, } +static JSON_OUTPUT_REQUESTED: std::sync::atomic::AtomicBool = + std::sync::atomic::AtomicBool::new(false); + fn main() { let exit_code = match try_main() { Ok(()) => ExitCode::Success, Err(err) => { let missing_credential = crate::auth::is_missing_credential_error(&err); let code = classify_error(&err, missing_credential); - print_error(&err, code, missing_credential); + print_error( + &err, + code, + missing_credential, + JSON_OUTPUT_REQUESTED.load(std::sync::atomic::Ordering::Relaxed), + ); code } }; @@ -309,6 +317,10 @@ fn try_main() -> Result<()> { apply_base_arg_sources(&matches, cli.command.base_mut()); cli.command.base_mut().profile_explicit = has_explicit_profile_arg(&argv); apply_base_output_defaults(&mut cli.command); + JSON_OUTPUT_REQUESTED.store( + cli.command.base().json, + std::sync::atomic::Ordering::Relaxed, + ); configure_output(cli.command.base()); apply_runtime_env_overrides(cli.command.base()); let runtime = tokio::runtime::Builder::new_multi_thread() @@ -426,7 +438,7 @@ fn classify_error(err: &anyhow::Error, missing_credential: bool) -> ExitCode { if let Some(http_error) = find_http_error(err) { let status = http_error.status.as_u16(); - if status == 401 || status == 403 { + if (status == 401 || status == 403) && !is_upstream_provider_auth_error(http_error) { return ExitCode::Auth; } if (400..=499).contains(&status) { @@ -461,6 +473,22 @@ fn find_http_error(err: &anyhow::Error) -> Option<&crate::http::HttpError> { .find_map(|source| source.downcast_ref::()) } +fn is_upstream_provider_auth_error(error: &crate::http::HttpError) -> bool { + let Ok(body) = serde_json::from_str::(&error.body) else { + return false; + }; + let provider_error = body.get("error").unwrap_or(&body); + provider_error.get("code").and_then(|value| value.as_str()) == Some("invalid_api_key") + || provider_error + .get("message") + .and_then(|value| value.as_str()) + .is_some_and(|message| { + let message = message.to_ascii_lowercase(); + message.contains("incorrect api key provided") + || message.contains("llm provider") && message.contains("credential") + }) +} + fn classify_sdk_error(err: &anyhow::Error) -> Option { let sdk_err = err .chain() @@ -512,7 +540,18 @@ fn looks_like_user_error(err: &anyhow::Error) -> bool { || message.contains("invalid") } -fn print_error(err: &anyhow::Error, code: ExitCode, missing_credential: bool) { +fn json_error_payload(err: &anyhow::Error) -> serde_json::Value { + find_http_error(err) + .and_then(|error| serde_json::from_str(&error.body).ok()) + .unwrap_or_else(|| serde_json::json!({ "error": { "message": err.to_string() } })) +} + +fn print_error(err: &anyhow::Error, code: ExitCode, missing_credential: bool, json_output: bool) { + if json_output { + println!("{}", json_error_payload(err)); + return; + } + eprintln!("error: {err}"); if code == ExitCode::Auth && !missing_credential { eprintln!("Your credentials may be expired or invalid. For OAuth profiles, try `bt login --refresh --profile `; if refresh fails, re-run `bt login --oauth --profile `. Run `bt status --all` to inspect profile status."); @@ -680,6 +719,35 @@ mod tests { parts.iter().map(OsString::from).collect() } + #[test] + fn provider_credential_errors_are_not_classified_as_bt_auth_errors() { + let err = anyhow::Error::new(crate::http::HttpError { + status: reqwest::StatusCode::UNAUTHORIZED, + body: serde_json::json!({ + "error": { + "message": "Incorrect API key provided: synthetic-key", + "type": "invalid_request_error", + "code": "invalid_api_key" + }, + "status": 401 + }) + .to_string(), + }); + + assert_eq!(classify_error(&err, false), ExitCode::User); + assert_eq!(json_error_payload(&err)["error"]["code"], "invalid_api_key"); + } + + #[test] + fn bt_unauthorized_errors_remain_auth_errors() { + let err = anyhow::Error::new(crate::http::HttpError { + status: reqwest::StatusCode::UNAUTHORIZED, + body: r#"{"error":"Unauthorized"}"#.to_string(), + }); + + assert_eq!(classify_error(&err, false), ExitCode::Auth); + } + #[test] fn handle_version_json_detects_long_form() { assert!(handle_version_json(&argv(&["bt", "--version", "--json"])).unwrap()); diff --git a/src/scorers.rs b/src/scorers.rs index 58c0083c..be8a5c47 100644 --- a/src/scorers.rs +++ b/src/scorers.rs @@ -47,6 +47,7 @@ mod tests { use clap::Parser; use super::*; + use crate::args::CLIArgs; #[derive(Debug, Parser)] struct ScorersArgsHarness { @@ -54,6 +55,24 @@ mod tests { args: ScorersArgs, } + #[test] + fn invoke_accepts_global_json_flag() { + #[derive(Debug, Parser)] + struct Harness { + #[command(flatten)] + command: CLIArgs, + } + + let parsed = Harness::try_parse_from(["bt-scorers", "invoke", "test-scorer", "--json"]) + .expect("parse scorer invoke with global JSON output"); + + assert!(parsed.command.base.json); + assert!(matches!( + parsed.command.args.command, + Some(ScorersCommands::Function(FunctionCommands::Invoke(_))) + )); + } + #[test] fn parses_create_scorer() { let parsed = ScorersArgsHarness::try_parse_from([ From e737d2b2780079fe8d4f1f906c2e2d6cc1449b73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Halber?= Date: Mon, 17 Aug 2026 17:24:21 -0700 Subject: [PATCH 07/12] chore(scorers): use_cache can only be used when temperature is 0 --- README.md | 2 +- src/functions/create.rs | 63 +++----------- src/functions/prompt_config.rs | 147 ++++++++++++++++++++++++++++++++- tests/cli.rs | 2 + 4 files changed, 159 insertions(+), 55 deletions(-) diff --git a/README.md b/README.md index f4fe8e07..ae5354d3 100644 --- a/README.md +++ b/README.md @@ -170,7 +170,7 @@ bt scorers create "Safety label" \ --allow-no-match ``` -Use `--if-exists error|ignore|replace` to control slug conflicts. Text and structured input flags accept an inline value, `@PATH`, or `-` for stdin; only one flag per command may read stdin. Use `--template-format mustache|jinja|none`; `nunjucks` and `jinja2` are accepted aliases for Jinja. +Use `--if-exists error|ignore|replace` to control slug conflicts. Text and structured input flags accept an inline value, `@PATH`, or `-` for stdin; only one flag per command may read stdin. Use `--template-format mustache|jinja|none`; `nunjucks` and `jinja2` are accepted aliases for Jinja. Model options include `--use-cache[=true|false]` and `--response-format text|json-object|`; structured output accepts a full `response_format` JSON object inline or from `@PATH`. For TypeScript and Python code scorers, use the Braintrust SDK and `bt functions push`. diff --git a/src/functions/create.rs b/src/functions/create.rs index c9192cbf..2a6f4cb5 100644 --- a/src/functions/create.rs +++ b/src/functions/create.rs @@ -458,59 +458,18 @@ mod tests { } #[test] - fn builds_model_params_template_metadata_and_pass_threshold() { - let parsed = CreateArgsHarness::try_parse_from([ - "bt-scorers-create", - "Test scorer", - "--model", - "gpt-test", - "--messages", - r#"[{"role":"user","content":"Judge {{output}}"}]"#, - "--choice-scores", - r#"{"yes":1,"no":0}"#, - "--temperature", - "0.1", - "--max-tokens", - "256", - "--top-p", - "0.8", - "--frequency-penalty", - "-0.25", - "--presence-penalty", - "0.5", - "--stop-sequence", - "END", - "--tool-choice", - "required", - "--reasoning-effort", - "medium", - "--verbosity", - "high", - "--template-format", - "jinja", - "--pass-threshold", - "0.7", - "--metadata", - "owner: test-team", - ]) - .expect("parse create args"); + fn builds_metadata_and_pass_threshold() { + let mut args = args(); + args.metadata = Some("owner: test-team".to_string()); + args.pass_threshold = Some(0.7); - let body = - build_scorer_definition(&parsed.args, "test-project", "Test scorer", "test-scorer") - .expect("definition"); - let params = &body["prompt_data"]["options"]["params"]; - assert_eq!(params["temperature"], 0.1); - assert_eq!(params["max_tokens"], 256); - assert_eq!(params["top_p"], 0.8); - assert_eq!(params["frequency_penalty"], -0.25); - assert_eq!(params["presence_penalty"], 0.5); - assert_eq!(params["stop"], json!(["END"])); - assert_eq!(params["tool_choice"], "required"); - assert_eq!(params["reasoning_effort"], "medium"); - assert_eq!(params["verbosity"], "high"); - assert_eq!(body["prompt_data"]["template_format"], "nunjucks"); - assert_eq!(body["metadata"]["owner"], "test-team"); - assert_eq!(body["metadata"]["__pass_threshold"], 0.7); + let body = build_scorer_definition(&args, "test-project", "Test scorer", "test-scorer") + .expect("definition"); + + assert_eq!( + body["metadata"], + json!({ "owner": "test-team", "__pass_threshold": 0.7 }) + ); } #[test] diff --git a/src/functions/prompt_config.rs b/src/functions/prompt_config.rs index cc3ef6cc..2a120a1b 100644 --- a/src/functions/prompt_config.rs +++ b/src/functions/prompt_config.rs @@ -1,7 +1,7 @@ use std::collections::HashSet; use anyhow::{bail, Context, Result}; -use clap::{Args, ValueEnum}; +use clap::{builder::BoolishValueParser, Args, ValueEnum}; use serde_json::{json, Map, Number, Value}; use crate::utils::read_text_source; @@ -44,6 +44,21 @@ pub(crate) struct PromptConfigArgs { #[arg(long, value_enum)] verbosity: Option, + /// Whether to use Braintrust's completion cache. Pass --use-cache=false to + /// bypass it. + #[arg( + long, + num_args = 0..=1, + default_missing_value = "true", + value_parser = BoolishValueParser::new() + )] + use_cache: Option, + + /// Model response format: text, json-object, or a response_format JSON + /// object supplied inline, from @PATH, or from stdin with -. + #[arg(long, value_name = "FORMAT|SOURCE")] + response_format: Option, + /// Prompt template syntax. Jinja is stored using Braintrust's `nunjucks` /// format; `nunjucks` and `jinja2` are accepted aliases. #[arg(long, value_enum, value_name = "FORMAT")] @@ -124,7 +139,14 @@ impl PromptConfigArgs { options.insert("model".to_string(), Value::String(model.to_string())); } - insert_optional_number(&mut params, "temperature", self.temperature)?; + let temperature = match (self.temperature, self.use_cache) { + (None, Some(true)) => Some(0.0), + (Some(temperature), Some(true)) if temperature != 0.0 => { + bail!("--use-cache=true requires --temperature=0") + } + (temperature, _) => temperature, + }; + insert_optional_number(&mut params, "temperature", temperature)?; if let Some(max_tokens) = self.max_tokens { params.insert("max_tokens".to_string(), Value::Number(max_tokens.into())); } @@ -174,6 +196,15 @@ impl PromptConfigArgs { Value::String(verbosity.as_str().to_string()), ); } + if let Some(use_cache) = self.use_cache { + params.insert("use_cache".to_string(), Value::Bool(use_cache)); + } + if let Some(source) = self.response_format.as_deref() { + params.insert( + "response_format".to_string(), + parse_response_format_source(source)?, + ); + } if !params.is_empty() { options.insert("params".to_string(), Value::Object(params)); @@ -192,6 +223,58 @@ impl PromptConfigArgs { } } +fn parse_response_format_source(source: &str) -> Result { + let value = match source { + "text" => json!({ "type": "text" }), + "json-object" | "json_object" => json!({ "type": "json_object" }), + _ => { + let raw = read_text_source(source, "response format")?; + serde_json::from_str(&raw) + .context("invalid response format; use text, json-object, or a JSON object")? + } + }; + + let Some(format) = value.as_object() else { + bail!("response format must be a JSON object"); + }; + match format.get("type").and_then(Value::as_str) { + Some("text" | "json_object") => {} + Some("json_schema") => validate_json_schema_response_format(format)?, + Some(other) => bail!( + "unsupported response format type '{other}'; expected text, json_object, or json_schema" + ), + None => bail!("response format must contain a string 'type' field"), + } + + Ok(value) +} + +fn validate_json_schema_response_format(format: &Map) -> Result<()> { + let Some(schema) = format.get("json_schema").and_then(Value::as_object) else { + bail!("json_schema response format must contain a 'json_schema' object"); + }; + match schema.get("name") { + Some(Value::String(_)) => {} + _ => bail!("json_schema response format must contain a string 'json_schema.name' field"), + } + if let Some(value) = schema.get("description") { + if !value.is_string() { + bail!("response format 'json_schema.description' must be a string"); + } + } + if let Some(value) = schema.get("schema") { + if !value.is_object() && !value.is_string() { + bail!("response format 'json_schema.schema' must be an object or template string"); + } + } + if let Some(value) = schema.get("strict") { + if !value.is_boolean() && !value.is_null() { + bail!("response format 'json_schema.strict' must be a boolean or null"); + } + } + Ok(()) +} + fn insert_optional_number( target: &mut Map, key: &str, @@ -310,6 +393,9 @@ mod tests { "high", "--verbosity", "low", + "--use-cache=false", + "--response-format", + r#"{"type":"json_schema","json_schema":{"name":"test_result","schema":{"type":"object"},"strict":true}}"#, "--template-format", "jinja", ]) @@ -332,9 +418,66 @@ mod tests { ); assert_eq!(patch["options"]["params"]["reasoning_effort"], "high"); assert_eq!(patch["options"]["params"]["verbosity"], "low"); + assert_eq!(patch["options"]["params"]["use_cache"], false); + assert_eq!( + patch["options"]["params"]["response_format"], + json!({ + "type": "json_schema", + "json_schema": { + "name": "test_result", + "schema": { "type": "object" }, + "strict": true, + } + }) + ); assert_eq!(patch["template_format"], "nunjucks"); } + #[test] + fn enabling_cache_sets_the_temperature_required_by_the_web_ui() { + let args = Harness::try_parse_from(["test", "--use-cache=true"]).expect("parse arguments"); + + let patch = args + .config + .build_prompt_data_patch(Some("claude-test")) + .expect("prompt data"); + assert_eq!(patch["options"]["params"]["temperature"], 0.0); + assert_eq!(patch["options"]["params"]["use_cache"], true); + } + + #[test] + fn rejects_cache_with_nonzero_temperature() { + let args = Harness::try_parse_from(["test", "--temperature", "0.5", "--use-cache=true"]) + .expect("parse arguments"); + + let error = args + .config + .build_prompt_data_patch(Some("claude-test")) + .expect_err("nonzero temperature should conflict with caching"); + assert!(error + .to_string() + .contains("--use-cache=true requires --temperature=0")); + } + + #[test] + fn supports_response_format_shorthands() { + assert_eq!( + parse_response_format_source("text").expect("text format"), + json!({ "type": "text" }) + ); + assert_eq!( + parse_response_format_source("json-object").expect("JSON object format"), + json!({ "type": "json_object" }) + ); + } + + #[test] + fn rejects_invalid_json_schema_response_format() { + let error = parse_response_format_source(r#"{"type":"json_schema"}"#) + .expect_err("missing json_schema should fail"); + assert!(error.to_string().contains("'json_schema' object")); + } + #[test] fn validates_scores_against_api_range() { let error = parse_choice_scores_source(r#"{"bad":1.5}"#) diff --git a/tests/cli.rs b/tests/cli.rs index 89213adf..d159d78d 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -957,6 +957,8 @@ fn scorers_create_help_includes_llm_judge_configuration() { .stdout(predicate::str::contains("--tool-choice")) .stdout(predicate::str::contains("--reasoning-effort")) .stdout(predicate::str::contains("--verbosity")) + .stdout(predicate::str::contains("--use-cache")) + .stdout(predicate::str::contains("--response-format")) .stdout(predicate::str::contains("--template-format")) .stdout(predicate::str::contains("--choice-scores")) .stdout(predicate::str::contains("--classifications")) From 8a927383c88e62014f82fef753348b5ade77b8fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Halber?= Date: Thu, 13 Aug 2026 18:35:01 -0700 Subject: [PATCH 08/12] feat(scorers): validate model parameters --- README.md | 2 + src/error.rs | 27 + src/functions/api.rs | 64 +- src/functions/create.rs | 129 +- src/functions/invoke.rs | 10 +- src/functions/mod.rs | 7 +- src/functions/model_capabilities.rs | 735 ++++++++++++ src/functions/prompt_config.rs | 1041 ++++++++++++++++- src/main.rs | 136 ++- src/scorers.rs | 22 - src/utils/cache.rs | 14 + src/utils/mod.rs | 2 + src/utils/text_source.rs | 24 +- .../snapshots-create/fixture.json | 4 +- tests/functions.rs | 8 +- 15 files changed, 2109 insertions(+), 116 deletions(-) create mode 100644 src/error.rs create mode 100644 src/functions/model_capabilities.rs create mode 100644 src/utils/cache.rs diff --git a/README.md b/README.md index ae5354d3..4bf8a06c 100644 --- a/README.md +++ b/README.md @@ -172,6 +172,8 @@ bt scorers create "Safety label" \ Use `--if-exists error|ignore|replace` to control slug conflicts. Text and structured input flags accept an inline value, `@PATH`, or `-` for stdin; only one flag per command may read stdin. Use `--template-format mustache|jinja|none`; `nunjucks` and `jinja2` are accepted aliases for Jinja. Model options include `--use-cache[=true|false]` and `--response-format text|json-object|`; structured output accepts a full `response_format` JSON object inline or from `@PATH`. +Model parameters are validated on a best-effort basis against the model catalog and org/project custom-model metadata used by the web UI. The catalog is cached for 24 hours per app URL, org, and project; lookup misses refetch immediately. Pass `--refresh-models` to bypass the cache after editing a custom model. If the model is not found or metadata cannot be loaded, `bt` warns that it performed only basic checks rather than rejecting models that may not yet appear in the catalog. + For TypeScript and Python code scorers, use the Braintrust SDK and `bt functions push`. ## `bt eval` diff --git a/src/error.rs b/src/error.rs new file mode 100644 index 00000000..3a9695be --- /dev/null +++ b/src/error.rs @@ -0,0 +1,27 @@ +use std::fmt; + +/// An expected error caused by command input rather than an internal failure. +#[derive(Debug)] +pub(crate) struct UserError { + source: Box, +} + +impl From for UserError { + fn from(error: anyhow::Error) -> Self { + Self { + source: error.into_boxed_dyn_error(), + } + } +} + +impl fmt::Display for UserError { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + self.source.fmt(formatter) + } +} + +impl std::error::Error for UserError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + Some(self.source.as_ref()) + } +} diff --git a/src/functions/api.rs b/src/functions/api.rs index ea5d6bd9..546dff94 100644 --- a/src/functions/api.rs +++ b/src/functions/api.rs @@ -3,7 +3,10 @@ use serde::{Deserialize, Serialize}; use serde_json::Value; use urlencoding::encode; -use crate::http::ApiClient; +use crate::{ + error::UserError, + http::{ApiClient, HttpError}, +}; fn escape_sql(s: &str) -> String { s.replace('\'', "''") @@ -164,9 +167,43 @@ pub async fn invoke_function( Vec::new() }; let timeout = std::time::Duration::from_secs(300); - client + let result = client .post_with_headers_timeout("/function/invoke", body, &headers, Some(timeout)) - .await + .await; + + match result { + Ok(value) => Ok(value), + Err(error) + if error + .downcast_ref::() + .is_some_and(is_provider_auth_response) => + { + Err(UserError::from(error).into()) + } + Err(error) => Err(error), + } +} + +fn is_provider_auth_response(error: &HttpError) -> bool { + if error.status != reqwest::StatusCode::UNAUTHORIZED + && error.status != reqwest::StatusCode::FORBIDDEN + { + return false; + } + + let Ok(body) = serde_json::from_str::(&error.body) else { + return false; + }; + let provider_error = body.get("error").unwrap_or(&body); + provider_error.get("code").and_then(Value::as_str) == Some("invalid_api_key") + || provider_error + .get("message") + .and_then(Value::as_str) + .is_some_and(|message| { + let message = message.to_ascii_lowercase(); + message.contains("incorrect api key provided") + || message.contains("llm provider") && message.contains("credential") + }) } pub async fn delete_function(client: &ApiClient, function_id: &str) -> Result<()> { @@ -333,6 +370,27 @@ fn ignored_count_from_function_results(raw: &Value, requests: &[Value]) -> Optio mod tests { use super::*; + #[test] + fn provider_auth_detection_requires_an_auth_status_and_provider_shape() { + let provider_error = HttpError { + status: reqwest::StatusCode::UNAUTHORIZED, + body: serde_json::json!({ + "error": { + "message": "Incorrect API key provided: synthetic-key", + "code": "invalid_api_key" + } + }) + .to_string(), + }; + assert!(is_provider_auth_response(&provider_error)); + + let bad_request = HttpError { + status: reqwest::StatusCode::BAD_REQUEST, + body: provider_error.body, + }; + assert!(!is_provider_auth_response(&bad_request)); + } + #[test] fn scorer_list_query_matches_web_ui_filter() { let query = list_functions_query("test-project-id", Some("scorer")); diff --git a/src/functions/create.rs b/src/functions/create.rs index 2a6f4cb5..b2c67827 100644 --- a/src/functions/create.rs +++ b/src/functions/create.rs @@ -4,6 +4,7 @@ use dialoguer::Input; use serde_json::{json, Map, Value}; use crate::{ + error::UserError, ui::{is_interactive, print_command_status, with_spinner, CommandStatus}, utils::{merge_json_objects, read_text_source, read_yaml_object_source}, }; @@ -11,8 +12,8 @@ use crate::{ use super::{ api, prompt_config::{ - parse_choice_scores_source, parse_classifications_source, validate_unit_interval, - PromptConfigArgs, + parse_choice_scores_source, parse_classifications_source, validate_prompt_data_patch, + validate_unit_interval, PromptConfigArgs, }, IfExistsMode, ResolvedContext, }; @@ -49,10 +50,10 @@ TypeScript and Python code scorers: ")] pub(crate) struct CreateArgs { /// Scorer name. - #[arg(value_name = "NAME", conflicts_with = "name")] + #[arg(value_name = "NAME")] name_positional: Option, - /// Scorer name (alternative to the positional name). + /// Scorer name (named form). #[arg(long, value_name = "NAME")] name: Option, @@ -116,9 +117,22 @@ pub(crate) struct CreateArgs { } pub(crate) async fn run(ctx: &ResolvedContext, args: &CreateArgs, json_output: bool) -> Result<()> { - let name = resolve_name(args)?; - let slug = resolve_slug(args, &name)?; - let definition = build_scorer_definition(args, &ctx.project.id, &name, &slug)?; + let name = resolve_name(args).map_err(UserError::from)?; + let slug = resolve_slug(args, &name).map_err(UserError::from)?; + let mut definition = + build_scorer_definition(args, &ctx.project.id, &name, &slug).map_err(UserError::from)?; + with_spinner( + "Validating model parameters...", + validate_prompt_data_patch( + ctx, + None, + &mut definition, + args.prompt_config.refresh_models(), + ), + ) + .await + .map_err(UserError::from)? + .warn_if_incomplete(); let result = match with_spinner( "Creating scorer...", @@ -169,15 +183,14 @@ pub(crate) async fn run(ctx: &ResolvedContext, args: &CreateArgs, json_output: b } fn resolve_name(args: &CreateArgs) -> Result { - let name = match (&args.name_positional, &args.name) { - (Some(_), Some(_)) => bail!("use either a positional name or --name, not both"), - (Some(name), None) | (None, Some(name)) => name.trim().to_string(), - (None, None) if is_interactive() => Input::::new() + let name = match args.name_positional.as_deref().or(args.name.as_deref()) { + Some(name) => name.trim().to_string(), + None if is_interactive() => Input::::new() .with_prompt("Scorer name") .interact_text()? .trim() .to_string(), - (None, None) => bail!("scorer name required. Use: bt scorers create ..."), + None => bail!("scorer name required. Use: bt scorers create ..."), }; if name.is_empty() { @@ -394,20 +407,6 @@ mod tests { assert_eq!(body["description"], "Synthetic test scorer"); } - #[test] - fn builds_chat_prompt_definition() { - let args = args(); - - let body = - build_scorer_definition(&args, "test-project", "Test", "test").expect("definition"); - - assert_eq!(body["prompt_data"]["prompt"]["type"], "chat"); - assert_eq!( - body["prompt_data"]["prompt"]["messages"], - json!([{ "role": "user", "content": "Judge {{output}}." }]) - ); - } - #[test] fn rejects_non_array_messages() { let mut args = args(); @@ -472,6 +471,84 @@ mod tests { ); } + #[test] + fn builds_model_params_template_metadata_and_pass_threshold() { + let parsed = CreateArgsHarness::try_parse_from([ + "bt-scorers-create", + "Test scorer", + "--model", + "gpt-test", + "--messages", + r#"[{"role":"user","content":"Judge {{output}}"}]"#, + "--choice-scores", + r#"{"yes":1,"no":0}"#, + "--temperature", + "0.1", + "--max-tokens", + "256", + "--top-p", + "0.8", + "--frequency-penalty", + "0.25", + "--presence-penalty", + "0.5", + "--stop-sequence", + "END", + "--tool-choice", + "required", + "--reasoning-effort", + "medium", + "--verbosity", + "high", + "--template-format", + "jinja", + "--pass-threshold", + "0.7", + "--metadata", + "owner: test-team", + ]) + .expect("parse create args"); + + let body = + build_scorer_definition(&parsed.args, "test-project", "Test scorer", "test-scorer") + .expect("definition"); + let params = &body["prompt_data"]["options"]["params"]; + assert_eq!(params["temperature"], 0.1); + assert_eq!(params["max_tokens"], 256); + assert_eq!(params["top_p"], 0.8); + assert_eq!(params["frequency_penalty"], 0.25); + assert_eq!(params["presence_penalty"], 0.5); + assert_eq!(params["stop"], json!(["END"])); + assert_eq!(params["tool_choice"], "required"); + assert_eq!(params["reasoning_effort"], "medium"); + assert_eq!(params["verbosity"], "high"); + assert_eq!(body["prompt_data"]["template_format"], "nunjucks"); + assert_eq!(body["metadata"]["owner"], "test-team"); + assert_eq!(body["metadata"]["__pass_threshold"], 0.7); + } + + #[test] + fn positional_name_takes_precedence_over_named_form() { + let parsed = CreateArgsHarness::try_parse_from([ + "bt-scorers-create", + "Positional name", + "--name", + "Named form", + "--model", + "gpt-test", + "--messages", + r#"[{"role":"user","content":"Judge {{output}}"}]"#, + "--choice-scores", + r#"{"yes":1,"no":0}"#, + ]) + .expect("parse both name forms"); + + assert_eq!( + resolve_name(&parsed.args).expect("resolve name"), + "Positional name" + ); + } + #[test] fn slugify_normalizes_name() { assert_eq!( diff --git a/src/functions/invoke.rs b/src/functions/invoke.rs index e7b294a1..9adc9ab9 100644 --- a/src/functions/invoke.rs +++ b/src/functions/invoke.rs @@ -128,17 +128,9 @@ mod tests { use super::resolve_mode; #[test] - fn json_output_requests_json_invoke_mode() { + fn resolves_invoke_mode() { assert_eq!(resolve_mode(None, true), Some("json")); - } - - #[test] - fn explicit_invoke_mode_takes_precedence_over_json_output() { assert_eq!(resolve_mode(Some("text"), true), Some("text")); - } - - #[test] - fn default_output_does_not_set_invoke_mode() { assert_eq!(resolve_mode(None, false), None); } } diff --git a/src/functions/mod.rs b/src/functions/mod.rs index a43e2618..09429bd7 100644 --- a/src/functions/mod.rs +++ b/src/functions/mod.rs @@ -17,6 +17,7 @@ pub(crate) mod create; mod delete; mod invoke; mod list; +mod model_capabilities; pub(crate) mod prompt_config; mod pull; mod push; @@ -185,11 +186,11 @@ pub struct FunctionArgs { pub(crate) enum FunctionCommands { /// List all in the current project List, - /// View a function's details + /// View details View(ViewArgs), - /// Delete a function + /// Delete by slug Delete(DeleteArgs), - /// Invoke a function + /// Invoke by slug Invoke(invoke::InvokeArgs), } diff --git a/src/functions/model_capabilities.rs b/src/functions/model_capabilities.rs new file mode 100644 index 00000000..99bef268 --- /dev/null +++ b/src/functions/model_capabilities.rs @@ -0,0 +1,735 @@ +use std::{ + collections::HashMap, + path::{Path, PathBuf}, + time::{Duration, SystemTime, UNIX_EPOCH}, +}; + +use anyhow::Result; +use serde::{Deserialize, Serialize}; +use serde_json::Value; +use sha2::{Digest, Sha256}; + +use crate::{ + http::build_http_client, + project_context::ProjectContext, + utils::{bt_cache_root, write_bytes_atomic}, +}; + +const MODEL_CATALOG_TIMEOUT: Duration = Duration::from_secs(5); +/// The catalog changes only on app deploy. A lookup miss refetches, so a newly +/// added custom model is still seen before the TTL expires. +const MODEL_CATALOG_TTL: Duration = Duration::from_secs(24 * 60 * 60); + +/// The model metadata used by the web UI to decide which controls and ranges +/// to expose. Unknown fields are intentionally ignored so newer app versions +/// can extend the catalog without breaking older CLI versions. +#[derive(Debug, Clone, Deserialize, Serialize)] +pub(crate) struct ModelSpec { + pub(crate) format: String, + pub(crate) flavor: String, + #[serde(default, rename = "displayName")] + pub(crate) display_name: Option, + #[serde(default)] + pub(crate) o1_like: Option, + #[serde(default)] + pub(crate) reasoning: Option, + #[serde(default)] + pub(crate) reasoning_budget: Option, + #[serde(default)] + pub(crate) max_output_tokens: Option, +} + +impl ModelSpec { + pub(crate) fn supports_reasoning(&self, model: &str) -> bool { + if self.reasoning.unwrap_or(false) || self.o1_like.unwrap_or(false) { + return true; + } + + // Match `modelProviderHasReasoning` from the web model catalog. The UI + // applies these fallbacks to custom models that omit `reasoning`. + let lower = model.to_ascii_lowercase(); + match self.format.as_str() { + "openai" => { + ["o1", "o2", "o3", "o4"] + .iter() + .any(|prefix| lower.starts_with(prefix)) + || lower.contains("gpt-5") + } + "anthropic" => lower.starts_with("claude-3.7"), + "google" => lower.ends_with("gemini-2.0-flash") || lower.contains("gemini-2.5"), + _ => false, + } + } +} + +#[derive(Debug, Deserialize)] +struct SecretWithMetadata { + #[serde(default)] + metadata: Option, +} + +#[derive(Debug, Deserialize)] +struct SecretListResponse { + objects: Vec, +} + +/// Both `Unknown` and `Unavailable` yield no spec, but only `Unavailable` means +/// the checks were skipped rather than deliberately not applicable. +#[derive(Debug, Clone)] +pub(crate) enum ModelLookup { + Found(ModelSpec), + /// Resolved from the on-disk cache after a fetch failed, so the spec may + /// predate changes made in the web UI. Yields a spec, but any limit it + /// implies is only as current as the last successful fetch. + Stale(ModelSpec), + /// Metadata loaded, but nothing defines this model. + Unknown, + /// Metadata could not be loaded; availability and ranges went unchecked. + Unavailable, +} + +impl ModelLookup { + pub(crate) fn spec(&self) -> Option<&ModelSpec> { + match self { + Self::Found(spec) | Self::Stale(spec) => Some(spec), + Self::Unknown | Self::Unavailable => None, + } + } + + pub(crate) fn is_unavailable(&self) -> bool { + matches!(self, Self::Unavailable) + } + + pub(crate) fn is_stale(&self) -> bool { + matches!(self, Self::Stale(_)) + } +} + +/// Resolve a model from the same shared catalog and configured custom-model +/// metadata used by the prompt UI. Project custom models take precedence over +/// org custom models, which take precedence over the shared catalog. Cached on +/// disk for [`MODEL_CATALOG_TTL`]. +/// +/// Missing metadata is non-fatal: callers fall back to provider-independent +/// validation so custom model names stay usable. +pub(crate) async fn resolve_model_lookup( + ctx: &ProjectContext, + model: &str, + refresh: bool, +) -> ModelLookup { + resolve_model_lookup_in(ctx, model, &catalog_cache_path(ctx), refresh).await +} + +async fn resolve_model_lookup_in( + ctx: &ProjectContext, + model: &str, + cache_path: &Path, + refresh: bool, +) -> ModelLookup { + if let Some(cached) = + read_cached_catalog(cache_path, Some(MODEL_CATALOG_TTL)).filter(|_| !refresh) + { + if let Some(spec) = resolve_from_models(&cached, model) { + return ModelLookup::Found(spec.clone()); + } + // Miss: fall through in case the model was added since we cached. + } + + let fetch = fetch_models(ctx).await; + if fetch.catalog_loaded && fetch.custom_models_loaded { + // A partial view would cache gaps as if they were absences. + let _ = write_cached_catalog(cache_path, &fetch.models); + } + + if fetch.custom_models_loaded { + if let Some(spec) = resolve_from_models(&fetch.models, model) { + return ModelLookup::Found(spec.clone()); + } + if fetch.catalog_loaded { + return ModelLookup::Unknown; + } + } + + // A partial fetch cannot establish precedence or absence. In particular, a + // shared match may be overridden by custom metadata we failed to load. The + // cache is consulted with no age bound here, including under `refresh`, so + // the result is reported as stale rather than current. + if let Some(cached) = read_cached_catalog(cache_path, None) { + if let Some(spec) = resolve_from_models(&cached, model) { + return ModelLookup::Stale(spec.clone()); + } + } + ModelLookup::Unavailable +} + +#[derive(Debug, Default)] +struct CatalogFetch { + models: HashMap, + catalog_loaded: bool, + custom_models_loaded: bool, +} + +async fn fetch_models(ctx: &ProjectContext) -> CatalogFetch { + let Ok(http) = build_http_client(MODEL_CATALOG_TIMEOUT) else { + return CatalogFetch::default(); + }; + let app_url = ctx.app_url.trim_end_matches('/'); + let catalog_url = format!("{app_url}/api/models/model_list.json"); + let org_secrets_url = format!("{app_url}/api/ai_secret/get"); + let project_secrets_path = format!( + "/v1/env_var?object_type=project&object_id={}&secret_category=ai_provider", + urlencoding::encode(&ctx.project.id), + ); + + let catalog_request = async { + let response = http.get(catalog_url).send().await.ok()?; + if !response.status().is_success() { + return None; + } + response.json::>().await.ok() + }; + let org_selector = if ctx.client.org_id().trim().is_empty() { + serde_json::json!({ "org_name": ctx.client.org_name() }) + } else { + serde_json::json!({ "org_id": ctx.client.org_id() }) + }; + let org_models_request = async { + let response = http + .post(org_secrets_url) + .bearer_auth(ctx.client.api_key()) + .json(&org_selector) + .send() + .await + .ok()?; + // Authentication and authorization failures do not prove that no + // custom models exist, so every unsuccessful response is inconclusive. + if !response.status().is_success() { + return None; + } + let secrets = response.json::>().await.ok()?; + custom_models_from_secrets(secrets) + }; + let project_models_request = async { + let response = tokio::time::timeout( + MODEL_CATALOG_TIMEOUT, + ctx.client.get::(&project_secrets_path), + ) + .await + .ok()?; + response + .ok() + .and_then(|response| custom_models_from_secrets(response.objects)) + }; + + let (catalog, org_models, project_models) = + tokio::join!(catalog_request, org_models_request, project_models_request); + + let catalog_loaded = catalog.is_some(); + let custom_models_loaded = org_models.is_some() && project_models.is_some(); + + let mut models = catalog.unwrap_or_default(); + models.extend(org_models.unwrap_or_default()); + models.extend(project_models.unwrap_or_default()); + + CatalogFetch { + models, + catalog_loaded, + custom_models_loaded, + } +} + +#[derive(Debug, Deserialize)] +struct CachedCatalog { + fetched_at: u64, + models: HashMap, +} + +#[derive(Debug, Serialize)] +struct CatalogToCache<'a> { + fetched_at: u64, + models: &'a HashMap, +} + +/// Keyed by app URL, org, and project because custom models are org- and +/// project-scoped. Version in the path invalidates on [`ModelSpec`] changes. +fn catalog_cache_path(ctx: &ProjectContext) -> PathBuf { + let mut hasher = Sha256::new(); + for part in [ + ctx.app_url.trim_end_matches('/'), + ctx.client.org_id(), + ctx.client.org_name(), + ctx.project.id.as_str(), + ] { + hasher.update(part.as_bytes()); + hasher.update([0]); + } + let key = hasher + .finalize() + .iter() + .take(8) + .map(|byte| format!("{byte:02x}")) + .collect::(); + + bt_cache_root() + .join("model-catalog") + .join(env!("CARGO_PKG_VERSION")) + .join(format!("{key}.json")) +} + +/// `max_age` of `None` accepts a cache of any age. +fn read_cached_catalog( + path: &Path, + max_age: Option, +) -> Option> { + let raw = std::fs::read(path).ok()?; + let cached: CachedCatalog = serde_json::from_slice(&raw).ok()?; + + if let Some(max_age) = max_age { + let now = SystemTime::now().duration_since(UNIX_EPOCH).ok()?.as_secs(); + // Saturate: a future timestamp is clock skew, not infinite freshness. + if Duration::from_secs(now.saturating_sub(cached.fetched_at)) > max_age { + return None; + } + } + + Some(cached.models) +} + +fn write_cached_catalog(path: &Path, models: &HashMap) -> Result<()> { + let fetched_at = SystemTime::now().duration_since(UNIX_EPOCH)?.as_secs(); + let payload = serde_json::to_vec(&CatalogToCache { fetched_at, models })?; + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent)?; + } + write_bytes_atomic(path, &payload) +} + +fn custom_models_from_secrets( + secrets: Vec, +) -> Option> { + let mut result = HashMap::new(); + for metadata in secrets.into_iter().filter_map(|secret| secret.metadata) { + let Some(models) = metadata.get("customModels") else { + continue; + }; + // A malformed custom-model entry makes absence inconclusive. Do not + // silently turn a parsing failure into an authoritative empty catalog. + let models = serde_json::from_value::>(models.clone()).ok()?; + // Preserve formats and flavors introduced by newer app versions. The + // caller treats unknown capabilities conservatively rather than making + // every custom model in this scope unavailable. + result.extend(models); + } + Some(result) +} + +fn resolve_from_models<'a>( + models: &'a HashMap, + model: &str, +) -> Option<&'a ModelSpec> { + // displayName is a UI label, not an identifier accepted by providers. + models.get(model) +} + +#[cfg(test)] +mod tests { + use actix_web::{dev::ServerHandle, http::StatusCode, web, App, HttpResponse, HttpServer}; + + use braintrust_sdk_rust::LoginState; + + use crate::{auth::LoginContext, http::ApiClient, projects::api::Project}; + + use super::*; + + fn spec(format: &str, display_name: Option<&str>) -> ModelSpec { + ModelSpec { + format: format.to_string(), + flavor: "chat".to_string(), + display_name: display_name.map(ToOwned::to_owned), + o1_like: None, + reasoning: None, + reasoning_budget: None, + max_output_tokens: None, + } + } + + #[test] + fn extracts_custom_models_from_secret_metadata() { + let secrets = vec![SecretWithMetadata { + metadata: Some(serde_json::json!({ + "customModels": { + "test-custom-model": { + "format": "anthropic", + "flavor": "chat", + "max_output_tokens": 4096 + } + } + })), + }]; + + let models = custom_models_from_secrets(secrets).expect("valid custom models"); + let model = models.get("test-custom-model").expect("custom model"); + assert_eq!(model.format, "anthropic"); + assert_eq!(model.max_output_tokens, Some(4096)); + } + + #[test] + fn applies_web_ui_reasoning_fallbacks_for_custom_models() { + let openai = spec("openai", None); + assert!(openai.supports_reasoning("o3")); + assert!(openai.supports_reasoning("test-gpt-5-deployment")); + assert!(!openai.supports_reasoning("gpt-4.1")); + + let anthropic = spec("anthropic", None); + assert!(anthropic.supports_reasoning("claude-3.7-sonnet")); + } + + #[test] + fn resolves_catalog_models_only_by_id() { + let models = HashMap::from([( + "test-model-id".to_string(), + spec("openai", Some("Test model")), + )]); + + assert!(resolve_from_models(&models, "test-model-id").is_some()); + assert!(resolve_from_models(&models, "Test model").is_none()); + assert!(resolve_from_models(&models, "missing").is_none()); + } + + #[test] + fn malformed_custom_models_make_the_result_inconclusive() { + for model in [ + serde_json::json!({"format": 42, "flavor": "chat"}), + serde_json::json!({"format": "openai"}), + ] { + let secrets = vec![SecretWithMetadata { + metadata: Some(serde_json::json!({ + "customModels": {"test-broken-model": model} + })), + }]; + + assert!(custom_models_from_secrets(secrets).is_none()); + } + } + + #[test] + fn preserves_unknown_custom_model_capabilities() { + let secrets = vec![SecretWithMetadata { + metadata: Some(serde_json::json!({ + "customModels": { + "test-future-model": { + "format": "test-future-format", + "flavor": "test-future-flavor" + } + } + })), + }]; + + let models = custom_models_from_secrets(secrets).expect("valid custom models"); + let spec = models.get("test-future-model").expect("future model"); + assert_eq!(spec.format, "test-future-format"); + assert_eq!(spec.flavor, "test-future-flavor"); + } + + struct MockServer { + base_url: String, + handle: ServerHandle, + } + + impl MockServer { + async fn start() -> Self { + Self::start_with_org_status(StatusCode::OK).await + } + + async fn start_with_org_status(org_status: StatusCode) -> Self { + let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("bind mock server"); + let address = listener.local_addr().expect("mock server address"); + let base_url = format!("http://{address}"); + let server = HttpServer::new(move || { + App::new() + .route( + "/api/models/model_list.json", + web::get().to(|| async { + HttpResponse::Ok().json(serde_json::json!({ + "test-shared-model": { + "format": "openai", + "flavor": "chat", + "max_output_tokens": 100 + } + })) + }), + ) + .route( + "/api/ai_secret/get", + web::post().to(move || async move { + if !org_status.is_success() { + return HttpResponse::build(org_status).finish(); + } + HttpResponse::Ok().json(serde_json::json!([{ + "metadata": { + "customModels": { + "test-precedence-model": { + "format": "anthropic", + "flavor": "chat", + "max_output_tokens": 200 + } + } + } + }])) + }), + ) + .route( + "/v1/env_var", + web::get().to(|| async { + HttpResponse::Ok().json(serde_json::json!({ + "objects": [{ + "metadata": { + "customModels": { + "test-precedence-model": { + "format": "google", + "flavor": "chat", + "max_output_tokens": 300 + } + } + } + }] + })) + }), + ) + }) + .workers(1) + .listen(listener) + .expect("listen mock server") + .run(); + let handle = server.handle(); + tokio::spawn(server); + Self { base_url, handle } + } + + async fn stop(self) { + self.handle.stop(false).await; + } + } + + fn test_context(base_url: &str) -> ProjectContext { + let login = LoginState::new(); + login.set( + "test-key".to_string(), + "test-org-id".to_string(), + "test-org".to_string(), + base_url.to_string(), + base_url.to_string(), + ); + let client = ApiClient::new(&LoginContext { + login, + api_url: base_url.to_string(), + app_url: base_url.to_string(), + profile: None, + }) + .expect("API client"); + ProjectContext { + client, + app_url: base_url.to_string(), + project: Project { + id: "test-project-id".to_string(), + name: "test-project".to_string(), + org_id: "test-org-id".to_string(), + description: None, + }, + } + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn resolves_shared_and_custom_models_with_project_precedence() { + let server = MockServer::start().await; + let ctx = test_context(&server.base_url); + let cache = tempfile::tempdir().expect("tempdir"); + let cache_path = cache.path().join("catalog.json"); + + let shared = resolve_model_lookup_in(&ctx, "test-shared-model", &cache_path, false) + .await + .spec() + .cloned() + .expect("shared model"); + assert_eq!(shared.format, "openai"); + assert_eq!(shared.max_output_tokens, Some(100)); + + let custom = resolve_model_lookup_in(&ctx, "test-precedence-model", &cache_path, false) + .await + .spec() + .cloned() + .expect("custom model"); + assert_eq!(custom.format, "google"); + assert_eq!(custom.max_output_tokens, Some(300)); + + server.stop().await; + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_shared_match_is_unavailable_when_custom_models_cannot_be_loaded() { + let server = MockServer::start_with_org_status(StatusCode::FORBIDDEN).await; + let ctx = test_context(&server.base_url); + let cache = tempfile::tempdir().expect("tempdir"); + + let lookup = resolve_model_lookup_in( + &ctx, + "test-shared-model", + &cache.path().join("catalog.json"), + false, + ) + .await; + assert!(lookup.is_unavailable()); + assert!(lookup.spec().is_none()); + + server.stop().await; + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_known_model_is_unknown_not_unavailable() { + let server = MockServer::start().await; + let ctx = test_context(&server.base_url); + let cache = tempfile::tempdir().expect("tempdir"); + + let lookup = resolve_model_lookup_in( + &ctx, + "test-absent-model", + &cache.path().join("catalog.json"), + false, + ) + .await; + assert!(matches!(lookup, ModelLookup::Unknown)); + assert!(!lookup.is_unavailable()); + + server.stop().await; + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_fresh_cache_serves_lookups_after_the_server_is_gone() { + let server = MockServer::start().await; + let ctx = test_context(&server.base_url); + let cache = tempfile::tempdir().expect("tempdir"); + let cache_path = cache.path().join("catalog.json"); + + resolve_model_lookup_in(&ctx, "test-shared-model", &cache_path, false) + .await + .spec() + .expect("warm the cache"); + server.stop().await; + + // Nothing is listening, so a hit can only be from the cache. + let cached = resolve_model_lookup_in(&ctx, "test-shared-model", &cache_path, false) + .await + .spec() + .cloned() + .expect("cached model"); + assert_eq!(cached.max_output_tokens, Some(100)); + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn refresh_bypasses_a_fresh_cache() { + let cache = tempfile::tempdir().expect("tempdir"); + let cache_path = cache.path().join("catalog.json"); + // A fresh cache claiming a model the server does not serve. + write_cached_catalog( + &cache_path, + &HashMap::from([("test-only-in-cache".to_string(), spec("openai", None))]), + ) + .expect("seed cache"); + + let server = MockServer::start().await; + let ctx = test_context(&server.base_url); + + assert!( + resolve_model_lookup_in(&ctx, "test-only-in-cache", &cache_path, false) + .await + .spec() + .is_some(), + "the cached entry should satisfy a normal lookup" + ); + assert!( + resolve_model_lookup_in(&ctx, "test-only-in-cache", &cache_path, true) + .await + .spec() + .is_none(), + "refresh should ignore the cache and see only the server's catalog" + ); + + server.stop().await; + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn an_unreachable_catalog_without_a_cache_is_unavailable() { + // Port 9 refuses connections. + let ctx = test_context("http://127.0.0.1:9"); + let cache = tempfile::tempdir().expect("tempdir"); + + let lookup = resolve_model_lookup_in( + &ctx, + "gpt-4.1-mini", + &cache.path().join("catalog.json"), + false, + ) + .await; + assert!(lookup.is_unavailable()); + assert!(lookup.spec().is_none()); + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn an_unreachable_catalog_falls_back_to_a_stale_cache() { + let cache = tempfile::tempdir().expect("tempdir"); + let cache_path = cache.path().join("catalog.json"); + let models = HashMap::from([("test-stale-model".to_string(), spec("openai", None))]); + write_cached_catalog(&cache_path, &models).expect("seed cache"); + // Backdated past the TTL, so only the stale-fallback path can hit. + let stale = serde_json::json!({ "fetched_at": 0, "models": models }); + std::fs::write(&cache_path, serde_json::to_vec(&stale).unwrap()).expect("backdate cache"); + + let ctx = test_context("http://127.0.0.1:9"); + let lookup = resolve_model_lookup_in(&ctx, "test-stale-model", &cache_path, false).await; + assert_eq!(lookup.spec().expect("stale model").format, "openai"); + } + + #[test] + fn cached_catalog_round_trips_and_expires() { + let cache = tempfile::tempdir().expect("tempdir"); + let cache_path = cache.path().join("catalog.json"); + let models = HashMap::from([( + "test-model-id".to_string(), + spec("anthropic", Some("Test model")), + )]); + + write_cached_catalog(&cache_path, &models).expect("write cache"); + let read = read_cached_catalog(&cache_path, Some(MODEL_CATALOG_TTL)).expect("fresh cache"); + assert_eq!(read["test-model-id"].format, "anthropic"); + assert_eq!( + read["test-model-id"].display_name.as_deref(), + Some("Test model") + ); + + // Past the TTL: rejected as fresh, still returned when unbounded. + let now = SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("clock") + .as_secs(); + let stale = serde_json::json!({ + "fetched_at": now - MODEL_CATALOG_TTL.as_secs() - 60, + "models": models, + }); + std::fs::write(&cache_path, serde_json::to_vec(&stale).expect("encode")) + .expect("backdate cache"); + assert!(read_cached_catalog(&cache_path, Some(MODEL_CATALOG_TTL)).is_none()); + assert!(read_cached_catalog(&cache_path, None).is_some()); + } + + #[test] + fn cache_paths_are_scoped_per_org_and_project() { + let a = catalog_cache_path(&test_context("https://app.test.example")); + let mut other = test_context("https://app.test.example"); + other.project.id = "test-other-project".to_string(); + let b = catalog_cache_path(&other); + + assert_ne!(a, b); + assert!(a.starts_with(bt_cache_root().join("model-catalog"))); + assert!(a.to_string_lossy().contains(env!("CARGO_PKG_VERSION"))); + } +} diff --git a/src/functions/prompt_config.rs b/src/functions/prompt_config.rs index 2a120a1b..c9e6ef76 100644 --- a/src/functions/prompt_config.rs +++ b/src/functions/prompt_config.rs @@ -4,27 +4,39 @@ use anyhow::{bail, Context, Result}; use clap::{builder::BoolishValueParser, Args, ValueEnum}; use serde_json::{json, Map, Number, Value}; -use crate::utils::read_text_source; +use crate::{ + project_context::ProjectContext, + ui::{print_command_status, CommandStatus}, + utils::{merge_json_objects, read_text_source}, +}; + +use super::model_capabilities::{resolve_model_lookup, ModelLookup, ModelSpec}; #[derive(Debug, Clone, Default, Args)] pub(crate) struct PromptConfigArgs { - /// Sampling temperature. + /// Sampling temperature, between 0 and 2. Some models support a smaller + /// range or do not support custom temperatures. #[arg(long, value_name = "NUMBER")] temperature: Option, - /// Maximum number of generated tokens. + /// Maximum number of generated tokens. Must be greater than 0. #[arg(long, value_name = "N")] max_tokens: Option, - /// Nucleus sampling probability. + /// Nucleus sampling probability, between 0 and 1. #[arg(long, value_name = "NUMBER")] top_p: Option, - /// Frequency penalty. + /// Top-k sampling value, between 1 and 100. Availability depends on the + /// model provider. + #[arg(long, value_name = "N")] + top_k: Option, + + /// Frequency penalty. Availability and range depend on the model. #[arg(long, value_name = "NUMBER", allow_hyphen_values = true)] frequency_penalty: Option, - /// Presence penalty. + /// Presence penalty. Availability and range depend on the model. #[arg(long, value_name = "NUMBER", allow_hyphen_values = true)] presence_penalty: Option, @@ -40,6 +52,20 @@ pub(crate) struct PromptConfigArgs { #[arg(long, value_enum)] reasoning_effort: Option, + /// Enable reasoning for models that configure reasoning with a token + /// budget rather than an effort level. + #[arg( + long, + num_args = 0..=1, + default_missing_value = "true", + value_parser = BoolishValueParser::new() + )] + reasoning_enabled: Option, + + /// Reasoning token budget, between 0 and 32768, for supported models. + #[arg(long, value_name = "N")] + reasoning_budget: Option, + /// Response verbosity for supported models. #[arg(long, value_enum)] verbosity: Option, @@ -63,6 +89,11 @@ pub(crate) struct PromptConfigArgs { /// format; `nunjucks` and `jinja2` are accepted aliases. #[arg(long, value_enum, value_name = "FORMAT")] template_format: Option, + + /// Refetch the model catalog instead of using the local 24h cache. Use this + /// after editing custom models in the web UI. + #[arg(long)] + refresh_models: bool, } #[derive(Debug, Clone, Copy, ValueEnum)] @@ -122,6 +153,10 @@ impl TemplateFormat { } impl PromptConfigArgs { + pub(crate) fn refresh_models(&self) -> bool { + self.refresh_models + } + /// Build a partial `prompt_data` object matching the app's prompt schema. pub(crate) fn build_prompt_data_patch( &self, @@ -154,6 +189,9 @@ impl PromptConfigArgs { validate_unit_interval(top_p, "--top-p")?; insert_number(&mut params, "top_p", top_p, "--top-p")?; } + if let Some(top_k) = self.top_k { + params.insert("top_k".to_string(), Value::Number(top_k.into())); + } insert_optional_number(&mut params, "frequency_penalty", self.frequency_penalty)?; insert_optional_number(&mut params, "presence_penalty", self.presence_penalty)?; @@ -190,6 +228,15 @@ impl PromptConfigArgs { Value::String(reasoning_effort.as_str().to_string()), ); } + if let Some(reasoning_enabled) = self.reasoning_enabled { + params.insert("reasoning_enabled".to_string(), reasoning_enabled.into()); + } + if let Some(reasoning_budget) = self.reasoning_budget { + params.insert( + "reasoning_budget".to_string(), + Value::Number(reasoning_budget.into()), + ); + } if let Some(verbosity) = self.verbosity { params.insert( "verbosity".to_string(), @@ -206,6 +253,10 @@ impl PromptConfigArgs { ); } + let effective_model = options.get("model").and_then(Value::as_str); + let changed_params = params.keys().cloned().collect(); + validate_model_params(effective_model, ¶ms, &changed_params, None)?; + if !params.is_empty() { options.insert("params".to_string(), Value::Object(params)); } @@ -275,6 +326,550 @@ fn validate_json_schema_response_format(format: &Map) -> Result<( Ok(()) } +/// Whether the model-specific half of validation ran. Capability metadata comes +/// over the network, so it can be missing — not an error, but worth reporting. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum ValidationOutcome { + /// Capabilities were resolved and checked, or nothing needed checking. + Checked, + /// Metadata loaded successfully, but did not contain this model. Basic + /// checks ran because the catalog is not authoritative for every model. + ModelUnknown { model: String }, + /// Only provider-independent type and range checks ran. + CapabilitiesUnavailable { model: String }, + /// Checks ran, but against a cached spec the fetch could not refresh. + CapabilitiesStale { model: String }, +} + +impl ValidationOutcome { + /// Call after any spinner finishes, or its repainting overwrites this. + pub(crate) fn warn_if_incomplete(&self) { + let message = match self { + Self::Checked => return, + Self::ModelUnknown { model } => format!( + "Model '{model}' was not found in the shared catalog or configured custom models; \ + checked only basic value ranges. Check the model ID or pass --refresh-models if \ + it was recently configured." + ), + Self::CapabilitiesUnavailable { model } => format!( + "Could not load model metadata for '{model}'; checked only basic value ranges. \ + Confirm this model supports the parameters you set." + ), + Self::CapabilitiesStale { model } => format!( + "Could not refresh model metadata for '{model}'; checked only basic value ranges. \ + Provider-specific validation and parameter normalization were skipped." + ), + }; + print_command_status(CommandStatus::Warning, &message); + } +} + +/// Validate `patch` against the model it will apply to and normalize parameter +/// names into the form expected by the API and prompt editor. +/// +/// Validation judges the effective configuration — `patch` merged over +/// `existing_prompt_data` — not the patch alone. Without capability metadata, +/// only provider-independent checks run and the outcome says so. +pub(crate) async fn validate_prompt_data_patch( + ctx: &ProjectContext, + existing_prompt_data: Option<&Value>, + patch: &mut Value, + refresh_models: bool, +) -> Result { + // `changed_params` must reflect what the caller actually set. + let update = prepare_model_params_update(existing_prompt_data, &*patch)?; + let lookup = match update.as_ref().and_then(|update| update.model.as_deref()) { + Some(model) => Some(resolve_model_lookup(ctx, model, refresh_models).await), + None => None, + }; + let spec = lookup.as_ref().and_then(ModelLookup::spec); + let stale = lookup.as_ref().is_some_and(ModelLookup::is_stale); + // Stale metadata must neither reject nor rewrite a configuration that may + // have become valid since it was cached. Canonical parameter names remain + // accepted by the proxy even when UI-specific normalization is skipped. + let current_spec = if stale { None } else { spec }; + if let Some(update) = &update { + validate_model_params( + update.model.as_deref(), + &update.params, + &update.changed_params, + current_spec, + )?; + } + + apply_provider_param_names(patch, current_spec); + + // An error is its own signal; only report skipped checks otherwise. + match (lookup, update.and_then(|update| update.model)) { + (Some(ModelLookup::Unknown), Some(model)) => Ok(ValidationOutcome::ModelUnknown { model }), + (Some(lookup), Some(model)) if lookup.is_unavailable() => { + Ok(ValidationOutcome::CapabilitiesUnavailable { model }) + } + (Some(lookup), Some(model)) if lookup.is_stale() => { + Ok(ValidationOutcome::CapabilitiesStale { model }) + } + _ => Ok(ValidationOutcome::Checked), + } +} + +/// Google prompts store these two under different keys. The proxy maps both +/// spellings, but the prompt editor reads only these, so the canonical names +/// leave its fields uninitialized. +const GOOGLE_PARAM_NAMES: &[(&str, &str)] = &[ + ("max_tokens", "maxOutputTokens"), + ("top_p", "topP"), + ("top_k", "topK"), +]; + +/// Rewrite parameter names to the spelling `spec`'s format expects. An unknown +/// format keeps the canonical names. +fn apply_provider_param_names(patch: &mut Value, spec: Option<&ModelSpec>) { + if spec.map(|spec| spec.format.as_str()) != Some("google") { + return; + } + let Some(params) = patch + .get_mut("prompt_data") + .and_then(|prompt_data| prompt_data.get_mut("options")) + .and_then(|options| options.get_mut("params")) + .and_then(Value::as_object_mut) + else { + return; + }; + for (canonical, google) in GOOGLE_PARAM_NAMES { + if let Some(value) = params.remove(*canonical) { + params.insert((*google).to_string(), value); + } + } +} + +#[derive(Debug)] +struct ModelParamsUpdate { + model: Option, + params: Map, + changed_params: HashSet, +} + +fn prepare_model_params_update( + existing_prompt_data: Option<&Value>, + patch: &Value, +) -> Result> { + let Some(patch_prompt_data) = patch.get("prompt_data").and_then(Value::as_object) else { + return Ok(None); + }; + let Some(patch_options_value) = patch_prompt_data.get("options") else { + return Ok(None); + }; + if patch_options_value.is_null() { + return Ok(None); + } + let patch_options = patch_options_value + .as_object() + .ok_or_else(|| anyhow::anyhow!("prompt_data.options must be a JSON object"))?; + + let mut effective_prompt_data = existing_prompt_data + .and_then(Value::as_object) + .cloned() + .unwrap_or_default(); + merge_json_objects(&mut effective_prompt_data, patch_prompt_data); + + let options = effective_prompt_data + .get("options") + .and_then(Value::as_object) + .ok_or_else(|| anyhow::anyhow!("prompt_data.options must be a JSON object"))?; + let model = match options.get("model") { + Some(Value::String(model)) if !model.trim().is_empty() => Some(model.trim().to_string()), + Some(Value::String(_)) => bail!("model cannot be empty"), + Some(Value::Null) | None => None, + Some(_) => bail!("prompt_data.options.model must be a string"), + }; + let params = match options.get("params") { + Some(Value::Object(params)) => params.clone(), + Some(Value::Null) | None => Map::new(), + Some(_) => bail!("prompt_data.options.params must be a JSON object"), + }; + + let model_changed = patch_options.contains_key("model"); + let mut changed_params = if model_changed { + params.keys().cloned().collect() + } else { + match patch_options.get("params") { + Some(Value::Object(params)) => params.keys().cloned().collect(), + Some(Value::Null) | None => HashSet::new(), + Some(_) => bail!("prompt_data.options.params must be a JSON object"), + } + }; + + // Temperature support can depend on reasoning effort, so changing either + // side of that relationship must validate the effective temperature. + if changed_params.contains("reasoning_effort") && params.contains_key("temperature") { + changed_params.insert("temperature".to_string()); + } + + // A model-only patch still needs lookup so miss/unavailable warnings are + // emitted even when the user did not set any optional model parameters. + if changed_params.is_empty() && !model_changed { + return Ok(None); + } + + Ok(Some(ModelParamsUpdate { + model, + params, + changed_params, + })) +} + +fn validate_model_params( + model: Option<&str>, + params: &Map, + changed_params: &HashSet, + spec: Option<&ModelSpec>, +) -> Result<()> { + if changed_params.contains("temperature") { + ensure_parameter_supported(spec, model, "--temperature", TEMPERATURE_FORMATS)?; + if let Some(temperature) = optional_number(params, "temperature", "--temperature")? { + let max = match spec.map(|spec| spec.format.as_str()) { + Some("anthropic" | "converse") => 1.0, + _ => 2.0, + }; + validate_number_range(temperature, 0.0, max, "--temperature")?; + if let Some(model) = model { + validate_temperature_support(model, params)?; + } + } + } + + if changed_params.contains("top_p") { + ensure_parameter_supported(spec, model, "--top-p", TOP_P_FORMATS)?; + if let Some(top_p) = optional_number(params, "top_p", "--top-p")? { + validate_number_range(top_p, 0.0, 1.0, "--top-p")?; + if let Some(model) = model.filter(|model| has_unsupported_opus_sampling_params(model)) { + bail!("--top-p is not supported by model '{model}'"); + } + } + } + + if changed_params.contains("top_k") { + ensure_parameter_supported(spec, model, "--top-k", TOP_K_FORMATS)?; + match params.get("top_k") { + Some(Value::Null) | None => {} + Some(Value::Number(number)) + if number + .as_u64() + .is_some_and(|value| (1..=100).contains(&value)) => {} + _ => bail!("--top-k must be an integer between 1 and 100"), + } + if let Some(model) = model.filter(|model| has_unsupported_opus_sampling_params(model)) { + bail!("--top-k is not supported by model '{model}'"); + } + } + + for (key, label) in [ + ("frequency_penalty", "--frequency-penalty"), + ("presence_penalty", "--presence-penalty"), + ] { + if changed_params.contains(key) { + ensure_parameter_supported(spec, model, label, PENALTY_FORMATS)?; + if let Some(value) = optional_number(params, key, label)? { + // The UI slider currently exposes 0..=1, but the backend's + // OpenAI schema and provider API accept the full -2..=2 range. + validate_number_range(value, -2.0, 2.0, label)?; + } + } + } + + if changed_params.contains("max_tokens") { + ensure_parameter_supported(spec, model, "--max-tokens", MAX_TOKENS_FORMATS)?; + if let Some(max_tokens) = params.get("max_tokens") { + match max_tokens { + Value::Null => {} + Value::Number(number) if number.as_u64().is_some_and(|value| value > 0) => { + if let (Some(spec), Some(value)) = (spec, number.as_u64()) { + let max = spec + .max_output_tokens + .filter(|max| *max > 0) + .unwrap_or(32_768); + if value > max { + bail!( + "--max-tokens must be between 1 and {max} for model '{}'", + model.unwrap_or("") + ); + } + } + } + _ => bail!("--max-tokens must be a positive integer"), + } + } + } + + if changed_params.contains("stop") { + match params.get("stop") { + Some(Value::Null) | None => {} + Some(Value::Array(values)) if values.iter().all(Value::is_string) => {} + _ => bail!("--stop-sequence values must be strings"), + } + } + + if changed_params.contains("tool_choice") { + if let Some(value) = params.get("tool_choice").filter(|value| !value.is_null()) { + ensure_parameter_supported(spec, model, "--tool-choice", TOOL_FORMATS)?; + validate_tool_choice(value)?; + } + } + + if changed_params.contains("reasoning_effort") { + validate_reasoning_effort(model, params, spec)?; + } + + if changed_params.contains("reasoning_enabled") || changed_params.contains("reasoning_budget") { + validate_reasoning_budget_params(model, params, spec)?; + } + + if changed_params.contains("verbosity") { + if let Some(value) = params.get("verbosity").filter(|value| !value.is_null()) { + let verbosity = value + .as_str() + .ok_or_else(|| anyhow::anyhow!("--verbosity must be a string"))?; + if !["low", "medium", "high"].contains(&verbosity) { + bail!("--verbosity must be one of low, medium, high"); + } + if let (Some(model), Some(spec)) = (model, spec) { + // Match the prompt UI: displayName is the visible model name + // when present; otherwise the model identifier is used. + let name = spec.display_name.as_deref().unwrap_or(model); + if !name.to_ascii_lowercase().contains("gpt-5") { + bail!("--verbosity is not supported by model '{model}'"); + } + } + } + } + + Ok(()) +} + +// Keep these in sync with `defaultModelParamSettings`, `getSliderSpecs`, and +// `modelProviderHasTools` in `proxy/packages/proxy/schema/index.ts`. +const KNOWN_FORMATS: &[&str] = &["openai", "anthropic", "google", "js", "window", "converse"]; +const TEMPERATURE_FORMATS: &[&str] = &["openai", "anthropic", "google", "window", "converse"]; +const MAX_TOKENS_FORMATS: &[&str] = &["openai", "anthropic", "google", "converse"]; +const TOP_P_FORMATS: &[&str] = &["openai", "anthropic", "google", "converse"]; +const TOP_K_FORMATS: &[&str] = &["anthropic", "google", "window"]; +const PENALTY_FORMATS: &[&str] = &["openai"]; +const TOOL_FORMATS: &[&str] = &["openai", "anthropic", "google", "converse"]; + +fn ensure_parameter_supported( + spec: Option<&ModelSpec>, + model: Option<&str>, + label: &str, + supported_formats: &[&str], +) -> Result<()> { + let Some(spec) = spec else { + return Ok(()); + }; + if KNOWN_FORMATS.contains(&spec.format.as_str()) + && !supported_formats.contains(&spec.format.as_str()) + { + bail!( + "{label} is not supported by model '{}' (format: {})", + model.unwrap_or(""), + spec.format, + ); + } + Ok(()) +} + +fn validate_tool_choice(value: &Value) -> Result<()> { + match value { + Value::String(choice) if ["auto", "none", "required"].contains(&choice.as_str()) => Ok(()), + Value::Object(choice) + if choice.get("type").and_then(Value::as_str) == Some("function") + && choice + .get("function") + .and_then(Value::as_object) + .and_then(|function| function.get("name")) + .and_then(Value::as_str) + .is_some_and(|name| !name.trim().is_empty()) => + { + Ok(()) + } + _ => bail!("--tool-choice must be auto, none, required, or a non-empty function name"), + } +} + +fn validate_reasoning_effort( + model: Option<&str>, + params: &Map, + spec: Option<&ModelSpec>, +) -> Result<()> { + let Some(effort) = params.get("reasoning_effort") else { + return Ok(()); + }; + if effort.is_null() { + return Ok(()); + } + let effort = effort + .as_str() + .ok_or_else(|| anyhow::anyhow!("--reasoning-effort must be a string"))?; + let (Some(model), Some(spec)) = (model, spec) else { + return Ok(()); + }; + if !KNOWN_FORMATS.contains(&spec.format.as_str()) { + return Ok(()); + } + if !spec.supports_reasoning(model) { + bail!("--reasoning-effort is not supported by model '{model}'"); + } + + let gemini_thinking_level = spec.format == "google" && is_gemini_3_model(model); + if spec.format != "openai" && spec.reasoning_budget.unwrap_or(false) && !gemini_thinking_level { + bail!( + "--reasoning-effort is not supported by model '{model}'; this model uses a reasoning budget" + ); + } + + let options = reasoning_effort_options(model, gemini_thinking_level); + if !options.contains(&effort) { + bail!( + "--reasoning-effort must be one of {} for model '{model}'", + options.join(", ") + ); + } + Ok(()) +} + +fn validate_reasoning_budget_params( + model: Option<&str>, + params: &Map, + spec: Option<&ModelSpec>, +) -> Result<()> { + if let Some(enabled) = params.get("reasoning_enabled") { + if !enabled.is_null() && !enabled.is_boolean() { + bail!("--reasoning-enabled must be true or false"); + } + } + if let Some(budget) = params.get("reasoning_budget") { + match budget { + Value::Null => {} + Value::Number(number) if number.as_u64().is_some_and(|value| value <= 32_768) => {} + _ => bail!("--reasoning-budget must be an integer between 0 and 32768"), + } + } + + let (Some(model), Some(spec)) = (model, spec) else { + return Ok(()); + }; + let uses_budget = spec.format != "openai" + && spec.reasoning_budget.unwrap_or(false) + && !(spec.format == "google" && is_gemini_3_model(model)); + if !uses_budget { + bail!("--reasoning-enabled and --reasoning-budget are not supported by model '{model}'"); + } + if !spec.supports_reasoning(model) { + bail!("reasoning is not supported by model '{model}'"); + } + Ok(()) +} + +fn optional_number(params: &Map, key: &str, label: &str) -> Result> { + match params.get(key) { + Some(Value::Null) | None => Ok(None), + Some(Value::Number(number)) => number + .as_f64() + .filter(|value| value.is_finite()) + .map(Some) + .ok_or_else(|| anyhow::anyhow!("{label} must be a finite number")), + Some(_) => bail!("{label} must be a number"), + } +} + +fn validate_number_range(value: f64, min: f64, max: f64, label: &str) -> Result<()> { + if !value.is_finite() || !(min..=max).contains(&value) { + bail!("{label} must be between {min} and {max}"); + } + Ok(()) +} + +fn reasoning_effort_options(model: &str, gemini_thinking_level: bool) -> &'static [&'static str] { + if is_gpt_5_pro_model(model) { + &["high"] + } else if is_gpt_5_1_or_later(model) { + &["none", "low", "medium", "high"] + } else if is_gpt_5_model(model) || gemini_thinking_level { + &["minimal", "low", "medium", "high"] + } else { + &["low", "medium", "high"] + } +} + +/// Keep this in sync with `modelSupportsCustomTemperature` in the backend's +/// `typespecs/src/model-capabilities.ts`. +fn validate_temperature_support(model: &str, params: &Map) -> Result<()> { + let lower = model.to_ascii_lowercase(); + + if lower.contains("claude-opus-4-7") { + bail!("--temperature is not supported by model '{model}'"); + } + + if lower.contains("gpt-5") { + let has_no_reasoning_effort = params + .get("reasoning_effort") + .and_then(Value::as_str) + .is_some_and(|effort| effort == "none"); + if !has_no_reasoning_effort { + // Only advise `--reasoning-effort none` on models that accept it; + // on the rest temperature is simply unusable. + if reasoning_effort_options(model, false).contains(&"none") { + bail!( + "--temperature is not supported by model '{model}' unless reasoning effort is 'none'; pass `--reasoning-effort none` or omit `--temperature`" + ); + } + bail!("--temperature is not supported by model '{model}'"); + } + } else if ["o1", "o2", "o3", "o4"] + .iter() + .any(|prefix| lower.starts_with(prefix)) + { + bail!("--temperature is not supported by model '{model}'"); + } + + Ok(()) +} + +fn has_unsupported_opus_sampling_params(model: &str) -> bool { + model.to_ascii_lowercase().contains("claude-opus-4-7") +} + +fn is_gpt_5_pro_model(model: &str) -> bool { + model.to_ascii_lowercase().contains("gpt-5-pro") +} + +fn is_gpt_5_1_or_later(model: &str) -> bool { + let lower = model.to_ascii_lowercase(); + for marker in ["gpt-5.", "databricks-gpt-5-"] { + let Some(start) = lower.find(marker) else { + continue; + }; + let version = lower[start + marker.len()..] + .chars() + .take_while(char::is_ascii_digit) + .collect::(); + if version.parse::().is_ok_and(|version| version >= 1) { + return true; + } + } + false +} + +fn is_gpt_5_model(model: &str) -> bool { + model.to_ascii_lowercase().contains("gpt-5") + && !is_gpt_5_1_or_later(model) + && !is_gpt_5_pro_model(model) +} + +fn is_gemini_3_model(model: &str) -> bool { + let lower = model.to_ascii_lowercase(); + lower.starts_with("gemini-3") || lower.contains("/gemini-3") +} + fn insert_optional_number( target: &mut Map, key: &str, @@ -369,6 +964,39 @@ mod tests { config: PromptConfigArgs, } + fn model_spec( + format: &str, + reasoning: bool, + reasoning_budget: bool, + max_output_tokens: Option, + ) -> ModelSpec { + ModelSpec { + format: format.to_string(), + flavor: "chat".to_string(), + display_name: None, + o1_like: None, + reasoning: Some(reasoning), + reasoning_budget: Some(reasoning_budget), + max_output_tokens, + } + } + + fn validate_patch( + existing_prompt_data: Option<&Value>, + patch: &Value, + spec: Option<&ModelSpec>, + ) -> Result<()> { + let Some(update) = prepare_model_params_update(existing_prompt_data, patch)? else { + return Ok(()); + }; + validate_model_params( + update.model.as_deref(), + &update.params, + &update.changed_params, + spec, + ) + } + #[test] fn builds_web_ui_compatible_prompt_configuration() { let args = Harness::try_parse_from([ @@ -380,7 +1008,7 @@ mod tests { "--top-p", "0.9", "--frequency-penalty", - "-0.5", + "0.5", "--presence-penalty", "0.25", "--stop-sequence", @@ -409,7 +1037,7 @@ mod tests { assert_eq!(patch["options"]["params"]["temperature"], 0.2); assert_eq!(patch["options"]["params"]["max_tokens"], 512); assert_eq!(patch["options"]["params"]["top_p"], 0.9); - assert_eq!(patch["options"]["params"]["frequency_penalty"], -0.5); + assert_eq!(patch["options"]["params"]["frequency_penalty"], 0.5); assert_eq!(patch["options"]["params"]["presence_penalty"], 0.25); assert_eq!(patch["options"]["params"]["stop"], json!(["END", "DONE"])); assert_eq!( @@ -471,6 +1099,403 @@ mod tests { ); } + #[test] + fn rejects_model_parameters_outside_provider_ranges() { + for (arguments, expected) in [ + ( + vec!["--temperature", "99"], + "--temperature must be between 0 and 2", + ), + ( + vec!["--max-tokens", "0"], + "--max-tokens must be a positive integer", + ), + ( + vec!["--frequency-penalty", "-2.1"], + "--frequency-penalty must be between -2 and 2", + ), + ( + vec!["--presence-penalty", "2.1"], + "--presence-penalty must be between -2 and 2", + ), + ] { + let parsed = + Harness::try_parse_from(std::iter::once("test").chain(arguments.iter().copied())) + .expect("parse arguments"); + let error = parsed + .config + .build_prompt_data_patch(Some("gpt-4.1-mini")) + .expect_err("out-of-range parameter should fail"); + assert_eq!(error.to_string(), expected); + } + } + + #[test] + fn enforces_model_specific_temperature_support() { + let parsed = + Harness::try_parse_from(["test", "--temperature", "0.2"]).expect("parse arguments"); + + for model in ["gpt-5.4-nano", "o3", "claude-opus-4-7"] { + let error = parsed + .config + .build_prompt_data_patch(Some(model)) + .expect_err("unsupported temperature should fail"); + assert!(error.to_string().contains("not supported by model")); + } + } + + #[test] + fn allows_gpt5_temperature_when_reasoning_effort_is_none() { + let parsed = + Harness::try_parse_from(["test", "--temperature", "0.2", "--reasoning-effort", "none"]) + .expect("parse arguments"); + + let patch = parsed + .config + .build_prompt_data_patch(Some("gpt-5.4-nano")) + .expect("compatible parameters"); + assert_eq!(patch["options"]["params"]["temperature"], 0.2); + assert_eq!(patch["options"]["params"]["reasoning_effort"], "none"); + } + + #[test] + fn does_not_advise_reasoning_effort_none_when_the_model_rejects_it() { + for model in ["gpt-5", "gpt-5-mini", "gpt-5-pro"] { + let parsed = + Harness::try_parse_from(["test", "--temperature", "0.2"]).expect("parse arguments"); + let error = parsed + .config + .build_prompt_data_patch(Some(model)) + .expect_err("temperature is unsupported"); + let error = error.to_string(); + assert!(error.contains("not supported by model"), "{error}"); + assert!(!error.contains("--reasoning-effort none"), "{error}"); + } + } + + #[test] + fn validates_update_against_the_existing_model_and_params() { + let existing = json!({ + "options": { + "model": "gpt-5.4-nano", + "params": { "reasoning_effort": "medium" } + } + }); + let patch = json!({ + "prompt_data": { + "options": { "params": { "temperature": 0.2 } } + } + }); + let error = validate_patch(Some(&existing), &patch, None) + .expect_err("effective model does not support temperature"); + assert!(error.to_string().contains("--reasoning-effort none")); + + let existing = json!({ + "options": { + "model": "gpt-5.4-nano", + "params": { "reasoning_effort": "none" } + } + }); + validate_patch(Some(&existing), &patch, None) + .expect("existing reasoning effort makes temperature valid"); + } + + #[test] + fn applies_format_ranges_without_model_name_heuristics() { + let patch = json!({ + "prompt_data": { + "options": { + "model": "test-custom-model", + "params": { "temperature": 1.5 } + } + } + }); + let anthropic = model_spec("anthropic", false, false, None); + let error = validate_patch(None, &patch, Some(&anthropic)) + .expect_err("Anthropic temperature should use the smaller range"); + assert_eq!(error.to_string(), "--temperature must be between 0 and 1"); + + validate_patch(None, &patch, None) + .expect("unknown custom models should not be assigned a format by name"); + } + + #[test] + fn applies_web_ui_parameter_availability_and_model_token_limit() { + let window = model_spec("window", false, false, None); + let tool_patch = json!({ + "prompt_data": { + "options": { + "model": "test-window-model", + "params": { "tool_choice": "auto" } + } + } + }); + let error = validate_patch(None, &tool_patch, Some(&window)) + .expect_err("Window models do not expose tool choice in the UI"); + assert!(error.to_string().contains("--tool-choice is not supported")); + + let openai = model_spec("openai", false, false, Some(4096)); + let token_patch = json!({ + "prompt_data": { + "options": { + "model": "test-limited-model", + "params": { "max_tokens": 4097 } + } + } + }); + let error = validate_patch(None, &token_patch, Some(&openai)) + .expect_err("model output token limit should be enforced"); + assert!(error.to_string().contains("between 1 and 4096")); + } + + #[test] + fn applies_web_ui_reasoning_options() { + let reasoning = model_spec("openai", true, false, None); + let invalid = json!({ + "prompt_data": { + "options": { + "model": "o3", + "params": { "reasoning_effort": "minimal" } + } + } + }); + let error = validate_patch(None, &invalid, Some(&reasoning)) + .expect_err("generic reasoning models accept low, medium, or high"); + assert!(error.to_string().contains("low, medium, high")); + + let non_reasoning = model_spec("openai", false, false, None); + let non_reasoning_patch = json!({ + "prompt_data": { + "options": { + "model": "gpt-4.1", + "params": { "reasoning_effort": "low" } + } + } + }); + let error = validate_patch(None, &non_reasoning_patch, Some(&non_reasoning)) + .expect_err("non-reasoning models should reject reasoning effort"); + assert!(error.to_string().contains("not supported")); + } + + #[test] + fn validates_existing_parameters_when_switching_models() { + let existing = json!({ + "options": { + "model": "gpt-4.1", + "params": { "temperature": 0.5 } + } + }); + let patch = json!({ + "prompt_data": { + "options": { "model": "o3" } + } + }); + let error = validate_patch( + Some(&existing), + &patch, + Some(&model_spec("openai", true, false, None)), + ) + .expect_err("switching models must validate retained parameters"); + assert!(error.to_string().contains("--temperature is not supported")); + } + + #[test] + fn model_only_patch_is_prepared_for_capability_lookup() { + let patch = json!({ + "prompt_data": {"options": {"model": "test-model"}} + }); + let update = prepare_model_params_update(None, &patch) + .expect("prepare update") + .expect("model change requires lookup"); + assert_eq!(update.model.as_deref(), Some("test-model")); + assert!(update.changed_params.is_empty()); + } + + #[test] + fn validates_only_parameters_touched_by_an_update() { + let existing = json!({ + "options": { + "model": "test-model", + "params": { "temperature": 99 } + } + }); + validate_patch( + Some(&existing), + &json!({"prompt_data": {"options": {"params": {"top_p": 0.5}}}}), + Some(&model_spec("openai", false, false, None)), + ) + .expect("an unrelated stale parameter should not block an update"); + + validate_patch(Some(&existing), &json!({"description": "Updated"}), None) + .expect("an unrelated metadata update should remain possible"); + } + + #[test] + fn known_openai_models_use_the_backend_penalty_range() { + // The UI slider exposes 0..=1, but the backend and OpenAI API accept + // -2..=2. CLI validation should not reject a backend-valid value. + let openai = model_spec("openai", false, false, None); + for key in ["frequency_penalty", "presence_penalty"] { + let patch = json!({ + "prompt_data": { + "options": { + "model": "gpt-4.1-mini", + "params": { key: -0.5 } + } + } + }); + validate_patch(None, &patch, Some(&openai)).expect("backend-valid penalty"); + } + } + + #[test] + fn unknown_models_retain_the_wider_provider_penalty_range() { + // Without metadata, keep the provider range rather than reject a + // possibly valid value. + let patch = json!({ + "prompt_data": { + "options": { + "model": "test-custom-model", + "params": { "frequency_penalty": -0.5 } + } + } + }); + validate_patch(None, &patch, None).expect("unknown models keep the -2..2 range"); + } + + #[test] + fn verbosity_uses_display_name_then_model_id_like_the_ui() { + let openai = model_spec("openai", false, false, None); + let mut renamed = model_spec("openai", false, false, None); + renamed.display_name = Some("Fast internal judge".to_string()); + let mut labeled_gpt5 = model_spec("openai", false, false, None); + labeled_gpt5.display_name = Some("GPT-5 internal judge".to_string()); + + let patch = |model: &str| { + json!({ + "prompt_data": { + "options": { + "model": model, + "params": { "verbosity": "low" } + } + } + }) + }; + + let error = validate_patch(None, &patch("internal-gpt-5-deployment"), Some(&renamed)) + .expect_err("a non-GPT-5 display name overrides the model id in the UI"); + assert!(error.to_string().contains("--verbosity is not supported")); + + validate_patch(None, &patch("internal-deployment"), Some(&labeled_gpt5)) + .expect("a GPT-5 display name should allow verbosity"); + let error = validate_patch(None, &patch("gpt-4.1-mini"), Some(&openai)) + .expect_err("non-gpt-5 models should reject verbosity"); + assert!(error.to_string().contains("--verbosity is not supported")); + } + + #[test] + fn google_models_get_the_parameter_names_the_prompt_editor_reads() { + let mut patch = json!({ + "prompt_data": { + "options": { + "model": "gemini-2.5-pro", + "params": { "max_tokens": 1000, "top_p": 0.9, "temperature": 0.2 } + } + } + }); + apply_provider_param_names( + &mut patch, + Some(&model_spec("google", true, true, Some(65535))), + ); + + let params = &patch["prompt_data"]["options"]["params"]; + assert_eq!(params["maxOutputTokens"], 1000); + assert_eq!(params["topP"], 0.9); + // Temperature keeps its name in every format. + assert_eq!(params["temperature"], 0.2); + assert!(params.get("max_tokens").is_none()); + assert!(params.get("top_p").is_none()); + } + + #[test] + fn other_formats_keep_canonical_parameter_names() { + let original = json!({ + "prompt_data": { + "options": { + "model": "gpt-4.1-mini", + "params": { "max_tokens": 1000, "top_p": 0.9 } + } + } + }); + + for spec in [ + Some(model_spec("openai", false, false, None)), + Some(model_spec("anthropic", false, false, None)), + Some(model_spec("converse", false, false, None)), + // An unresolved model: the format is unknown, so do not guess. + None, + ] { + let mut patch = original.clone(); + apply_provider_param_names(&mut patch, spec.as_ref()); + assert_eq!(patch, original); + } + } + + #[test] + fn renaming_tolerates_a_patch_without_params() { + let mut patch = json!({ "description": "no prompt data at all" }); + let original = patch.clone(); + apply_provider_param_names(&mut patch, Some(&model_spec("google", false, false, None))); + assert_eq!(patch, original); + } + + #[test] + fn validates_and_translates_top_k() { + let parsed = Harness::try_parse_from(["test", "--top-k", "42"]).expect("parse top-k"); + let mut patch = Value::Object( + parsed + .config + .build_prompt_data_patch(Some("gemini-test")) + .expect("prompt data"), + ); + let google = model_spec("google", false, false, None); + validate_patch(None, &json!({"prompt_data": patch.clone()}), Some(&google)) + .expect("valid Google top-k"); + + // The validator receives a full function patch in production. + patch = json!({"prompt_data": patch}); + apply_provider_param_names(&mut patch, Some(&google)); + assert_eq!(patch["prompt_data"]["options"]["params"]["topK"], 42); + } + + #[test] + fn supports_reasoning_budget_models() { + let parsed = + Harness::try_parse_from(["test", "--reasoning-enabled", "--reasoning-budget", "2048"]) + .expect("parse reasoning budget"); + let prompt_data = parsed + .config + .build_prompt_data_patch(Some("test-budget-model")) + .expect("prompt data"); + let patch = json!({"prompt_data": prompt_data}); + let budget_model = model_spec("anthropic", true, true, None); + validate_patch(None, &patch, Some(&budget_model)).expect("budget model parameters"); + + let effort_model = model_spec("openai", true, false, None); + let error = validate_patch(None, &patch, Some(&effort_model)) + .expect_err("effort-based model should reject budget parameters"); + assert!(error.to_string().contains("not supported")); + } + + #[test] + fn matches_backend_databricks_gpt5_reasoning_options() { + assert!(is_gpt_5_1_or_later("databricks-gpt-5-2")); + assert_eq!( + reasoning_effort_options("databricks-gpt-5-2", false), + &["none", "low", "medium", "high"] + ); + } + #[test] fn rejects_invalid_json_schema_response_format() { let error = parse_response_format_source(r#"{"type":"json_schema"}"#) diff --git a/src/main.rs b/src/main.rs index 12cf43b1..df9d28b1 100644 --- a/src/main.rs +++ b/src/main.rs @@ -8,6 +8,7 @@ mod auth; mod config; mod datasets; mod env; +mod error; #[cfg(unix)] mod eval; mod experiments; @@ -436,9 +437,13 @@ fn classify_error(err: &anyhow::Error, missing_credential: bool) -> ExitCode { return ExitCode::Auth; } + if has_user_error(err) { + return ExitCode::User; + } + if let Some(http_error) = find_http_error(err) { let status = http_error.status.as_u16(); - if (status == 401 || status == 403) && !is_upstream_provider_auth_error(http_error) { + if status == 401 || status == 403 { return ExitCode::Auth; } if (400..=499).contains(&status) { @@ -473,22 +478,6 @@ fn find_http_error(err: &anyhow::Error) -> Option<&crate::http::HttpError> { .find_map(|source| source.downcast_ref::()) } -fn is_upstream_provider_auth_error(error: &crate::http::HttpError) -> bool { - let Ok(body) = serde_json::from_str::(&error.body) else { - return false; - }; - let provider_error = body.get("error").unwrap_or(&body); - provider_error.get("code").and_then(|value| value.as_str()) == Some("invalid_api_key") - || provider_error - .get("message") - .and_then(|value| value.as_str()) - .is_some_and(|message| { - let message = message.to_ascii_lowercase(); - message.contains("incorrect api key provided") - || message.contains("llm provider") && message.contains("credential") - }) -} - fn classify_sdk_error(err: &anyhow::Error) -> Option { let sdk_err = err .chain() @@ -532,6 +521,11 @@ fn has_io_error(err: &anyhow::Error) -> bool { .any(|source| source.downcast_ref::().is_some()) } +fn has_user_error(err: &anyhow::Error) -> bool { + err.chain() + .any(|source| source.downcast_ref::().is_some()) +} + fn looks_like_user_error(err: &anyhow::Error) -> bool { let message = err.to_string().to_lowercase(); message.contains("required") @@ -541,14 +535,36 @@ fn looks_like_user_error(err: &anyhow::Error) -> bool { } fn json_error_payload(err: &anyhow::Error) -> serde_json::Value { - find_http_error(err) - .and_then(|error| serde_json::from_str(&error.body).ok()) - .unwrap_or_else(|| serde_json::json!({ "error": { "message": err.to_string() } })) + let details = find_http_error(err) + .and_then(|error| serde_json::from_str::(&error.body).ok()); + let message = details + .as_ref() + .and_then(json_error_message) + .unwrap_or_else(|| err.to_string()); + + match details { + Some(details) => serde_json::json!({ + "error": { + "message": message, + "details": details, + } + }), + None => serde_json::json!({ "error": { "message": message } }), + } +} + +fn json_error_message(details: &serde_json::Value) -> Option { + details + .pointer("/error/message") + .and_then(serde_json::Value::as_str) + .or_else(|| details.get("message").and_then(serde_json::Value::as_str)) + .or_else(|| details.get("error").and_then(serde_json::Value::as_str)) + .map(ToOwned::to_owned) } fn print_error(err: &anyhow::Error, code: ExitCode, missing_credential: bool, json_output: bool) { if json_output { - println!("{}", json_error_payload(err)); + eprintln!("{}", json_error_payload(err)); return; } @@ -719,33 +735,77 @@ mod tests { parts.iter().map(OsString::from).collect() } + #[test] + fn typed_user_errors_use_the_user_exit_code() { + let err = anyhow::Error::new(crate::error::UserError::from(anyhow::anyhow!( + "--temperature must be between 0 and 2" + ))); + + assert_eq!(classify_error(&err, false), ExitCode::User); + } + #[test] fn provider_credential_errors_are_not_classified_as_bt_auth_errors() { - let err = anyhow::Error::new(crate::http::HttpError { - status: reqwest::StatusCode::UNAUTHORIZED, - body: serde_json::json!({ - "error": { - "message": "Incorrect API key provided: synthetic-key", - "type": "invalid_request_error", - "code": "invalid_api_key" - }, - "status": 401 - }) - .to_string(), - }); + let err = anyhow::Error::new(crate::error::UserError::from(anyhow::Error::new( + crate::http::HttpError { + status: reqwest::StatusCode::UNAUTHORIZED, + body: serde_json::json!({ + "error": { + "message": "Incorrect API key provided: synthetic-key", + "type": "invalid_request_error", + "code": "invalid_api_key" + }, + "status": 401 + }) + .to_string(), + }, + ))); assert_eq!(classify_error(&err, false), ExitCode::User); - assert_eq!(json_error_payload(&err)["error"]["code"], "invalid_api_key"); + let payload = json_error_payload(&err); + assert_eq!( + payload["error"]["message"], + "Incorrect API key provided: synthetic-key" + ); + assert_eq!( + payload["error"]["details"]["error"]["code"], + "invalid_api_key" + ); } #[test] - fn bt_unauthorized_errors_remain_auth_errors() { + fn json_http_errors_use_a_stable_envelope() { let err = anyhow::Error::new(crate::http::HttpError { - status: reqwest::StatusCode::UNAUTHORIZED, - body: r#"{"error":"Unauthorized"}"#.to_string(), + status: reqwest::StatusCode::BAD_REQUEST, + body: serde_json::json!(["synthetic", "details"]).to_string(), }); - assert_eq!(classify_error(&err, false), ExitCode::Auth); + let payload = json_error_payload(&err); + assert!(payload["error"]["message"].is_string()); + assert_eq!( + payload["error"]["details"], + serde_json::json!(["synthetic", "details"]) + ); + } + + #[test] + fn bt_unauthorized_errors_remain_auth_errors() { + for body in [ + serde_json::json!({ "error": "Unauthorized" }), + serde_json::json!({ + "error": { + "message": "Invalid Braintrust API key", + "code": "invalid_api_key" + } + }), + ] { + let err = anyhow::Error::new(crate::http::HttpError { + status: reqwest::StatusCode::UNAUTHORIZED, + body: body.to_string(), + }); + + assert_eq!(classify_error(&err, false), ExitCode::Auth); + } } #[test] diff --git a/src/scorers.rs b/src/scorers.rs index be8a5c47..d2485a6e 100644 --- a/src/scorers.rs +++ b/src/scorers.rs @@ -96,26 +96,4 @@ mod tests { Some(ScorersCommands::Create(_)) )); } - - #[test] - fn parses_create_classifier() { - let parsed = ScorersArgsHarness::try_parse_from([ - "bt-scorers", - "create", - "Test classifier", - "--model", - "gpt-test", - "--messages", - r#"[{"role":"user","content":"Classify {{output}}"}]"#, - "--classifications", - r#"["safe","unsafe"]"#, - "--allow-no-match", - ]) - .expect("parse create classifier"); - - assert!(matches!( - parsed.args.command, - Some(ScorersCommands::Create(_)) - )); - } } diff --git a/src/utils/cache.rs b/src/utils/cache.rs new file mode 100644 index 00000000..b1bb3522 --- /dev/null +++ b/src/utils/cache.rs @@ -0,0 +1,14 @@ +use std::path::PathBuf; + +/// Root directory for `bt`'s on-disk caches. +/// +/// Path discovery, not configuration: standard `XDG_CACHE_HOME`/`HOME` only, no +/// bt-specific variable. Falls back to the temp directory. +pub(crate) fn bt_cache_root() -> PathBuf { + let root = std::env::var_os("XDG_CACHE_HOME") + .map(PathBuf::from) + .or_else(|| std::env::var_os("HOME").map(|home| PathBuf::from(home).join(".cache"))) + .unwrap_or_else(std::env::temp_dir); + + root.join("bt") +} diff --git a/src/utils/mod.rs b/src/utils/mod.rs index 1429bebe..922e10a1 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -1,4 +1,5 @@ mod app_url; +mod cache; mod duration; mod fs_atomic; mod git; @@ -10,6 +11,7 @@ mod structured_source; mod text_source; pub(crate) use app_url::{app_project_url, app_project_url_with_encoded_path}; +pub(crate) use cache::bt_cache_root; pub use duration::parse_duration_to_seconds; pub use fs_atomic::{ write_bytes_atomic, write_json_atomic, write_json_atomic_private, write_text_atomic, diff --git a/src/utils/text_source.rs b/src/utils/text_source.rs index 1f2ce754..489d6775 100644 --- a/src/utils/text_source.rs +++ b/src/utils/text_source.rs @@ -1,4 +1,4 @@ -use std::io::Read; +use std::io::{IsTerminal, Read}; use std::sync::Mutex; use anyhow::{bail, Context, Result}; @@ -20,6 +20,8 @@ fn read_text_source_with_stdin_guard( stdin_reader: &Mutex>, ) -> Result { if value == "-" { + ensure_stdin_is_piped(label, std::io::stdin().is_terminal())?; + // The second reader would otherwise see "" and call it malformed input. let mut reader = stdin_reader .lock() @@ -52,6 +54,15 @@ fn read_text_source_with_stdin_guard( Ok(value.to_string()) } +fn ensure_stdin_is_piped(label: &str, stdin_is_terminal: bool) -> Result<()> { + if stdin_is_terminal { + bail!( + "cannot read {label} from interactive stdin; pipe input or use an inline value or @PATH" + ); + } + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -91,6 +102,17 @@ mod tests { assert!(error.to_string().contains("cannot be empty")); } + #[test] + fn rejects_interactive_stdin_source() { + let error = ensure_stdin_is_piped("messages", true) + .expect_err("interactive stdin should not wait for EOF"); + + assert_eq!( + error.to_string(), + "cannot read messages from interactive stdin; pipe input or use an inline value or @PATH" + ); + } + #[test] fn rejects_a_second_stdin_source() { // A local guard avoids draining the suite's shared stdin; the rejection diff --git a/tests/datasets-fixtures/snapshots-create/fixture.json b/tests/datasets-fixtures/snapshots-create/fixture.json index 6c67196e..a519fea0 100644 --- a/tests/datasets-fixtures/snapshots-create/fixture.json +++ b/tests/datasets-fixtures/snapshots-create/fixture.json @@ -126,7 +126,7 @@ "baseline" ], "expect_success": false, - "stderr_contains": [ + "stdout_contains": [ "snapshot delete requires --force in non-interactive mode" ] }, @@ -169,7 +169,7 @@ "snapshot-source" ], "expect_success": false, - "stderr_contains": [ + "stdout_contains": [ "dataset delete requires --force in non-interactive mode" ] }, diff --git a/tests/functions.rs b/tests/functions.rs index 19e14d42..a23938c2 100644 --- a/tests/functions.rs +++ b/tests/functions.rs @@ -678,9 +678,9 @@ fn functions_push_requires_app_url_with_custom_api_url() { .expect("run push with custom API URL and no app URL"); assert!(!output.status.success()); - let stderr = String::from_utf8_lossy(&output.stderr); - assert!(stderr.contains("--app-url or BRAINTRUST_APP_URL")); - assert!(!stderr.contains("https://www.braintrust.dev/api/apikey/login")); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!(stdout.contains("--app-url or BRAINTRUST_APP_URL")); + assert!(!stdout.contains("https://www.braintrust.dev/api/apikey/login")); } #[test] @@ -725,7 +725,7 @@ fn root_login_refresh_uses_selected_profile() { let output = cmd.output().expect("run bt login --refresh"); assert!(!output.status.success()); - assert!(String::from_utf8_lossy(&output.stderr) + assert!(String::from_utf8_lossy(&output.stdout) .contains("`bt login --refresh` only applies to oauth profiles")); } From 4dba7eca862e9d0d016d2d5bdcaf622236da5320 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Halber?= Date: Tue, 18 Aug 2026 13:55:04 -0700 Subject: [PATCH 09/12] chore(scorer): use backend validation instead of local validation --- README.md | 2 +- src/functions/api.rs | 48 ++ src/functions/create.rs | 69 +- src/functions/mod.rs | 1 - src/functions/model_capabilities.rs | 735 ------------------- src/functions/prompt_config.rs | 1032 +-------------------------- src/utils/cache.rs | 14 - src/utils/mod.rs | 2 - 8 files changed, 112 insertions(+), 1791 deletions(-) delete mode 100644 src/functions/model_capabilities.rs delete mode 100644 src/utils/cache.rs diff --git a/README.md b/README.md index 4bf8a06c..d7382d6e 100644 --- a/README.md +++ b/README.md @@ -172,7 +172,7 @@ bt scorers create "Safety label" \ Use `--if-exists error|ignore|replace` to control slug conflicts. Text and structured input flags accept an inline value, `@PATH`, or `-` for stdin; only one flag per command may read stdin. Use `--template-format mustache|jinja|none`; `nunjucks` and `jinja2` are accepted aliases for Jinja. Model options include `--use-cache[=true|false]` and `--response-format text|json-object|`; structured output accepts a full `response_format` JSON object inline or from `@PATH`. -Model parameters are validated on a best-effort basis against the model catalog and org/project custom-model metadata used by the web UI. The catalog is cached for 24 hours per app URL, org, and project; lookup misses refetch immediately. Pass `--refresh-models` to bypass the cache after editing a custom model. If the model is not found or metadata cannot be loaded, `bt` warns that it performed only basic checks rather than rejecting models that may not yet appear in the catalog. +Before writing a scorer, `bt` sends the complete candidate definition to Braintrust for validation. The backend applies the same model-parameter and replacement checks as the write and returns structured issues with normalization suggestions when available. For TypeScript and Python code scorers, use the Braintrust SDK and `bt functions push`. diff --git a/src/functions/api.rs b/src/functions/api.rs index 546dff94..6b62aa10 100644 --- a/src/functions/api.rs +++ b/src/functions/api.rs @@ -69,6 +69,34 @@ pub struct InsertedFunctionResult { pub found_existing: bool, } +#[derive(Debug, Clone, Deserialize)] +pub struct FunctionValidationSuggestion { + pub action: String, + #[serde(default)] + pub value: Option, +} + +#[derive(Debug, Clone, Deserialize)] +pub struct FunctionValidationIssue { + pub code: String, + pub path: Vec, + pub message: String, + pub blocking: bool, + #[serde(default)] + pub suggestion: Option, +} + +#[derive(Debug, Clone, Deserialize)] +pub struct FunctionValidationResult { + pub issues: Vec, +} + +#[derive(Debug, Clone, Deserialize)] +pub struct FunctionValidationReport { + pub valid: bool, + pub results: Vec, +} + #[derive(Debug, Clone)] pub struct InsertFunctionsResult { pub ignored_entries: Option, @@ -315,6 +343,26 @@ pub async fn upload_bundle( .context("failed to upload code bundle to signed URL") } +pub async fn validate_functions( + client: &ApiClient, + functions: &[Value], +) -> Result { + let body = insert_functions_body(functions); + match client.post("/validate-functions", &body).await { + Ok(report) => Ok(report), + Err(error) => { + let Some(http_error) = error.downcast_ref::() else { + return Err(error).context("failed to validate functions"); + }; + if http_error.status != reqwest::StatusCode::UNPROCESSABLE_ENTITY { + return Err(error).context("failed to validate functions"); + } + serde_json::from_str(&http_error.body) + .context("unexpected validate-functions error response shape") + } + } +} + pub async fn insert_functions( client: &ApiClient, functions: &[Value], diff --git a/src/functions/create.rs b/src/functions/create.rs index b2c67827..28cce67e 100644 --- a/src/functions/create.rs +++ b/src/functions/create.rs @@ -12,8 +12,8 @@ use crate::{ use super::{ api, prompt_config::{ - parse_choice_scores_source, parse_classifications_source, validate_prompt_data_patch, - validate_unit_interval, PromptConfigArgs, + parse_choice_scores_source, parse_classifications_source, validate_unit_interval, + PromptConfigArgs, }, IfExistsMode, ResolvedContext, }; @@ -119,20 +119,14 @@ pub(crate) struct CreateArgs { pub(crate) async fn run(ctx: &ResolvedContext, args: &CreateArgs, json_output: bool) -> Result<()> { let name = resolve_name(args).map_err(UserError::from)?; let slug = resolve_slug(args, &name).map_err(UserError::from)?; - let mut definition = + let definition = build_scorer_definition(args, &ctx.project.id, &name, &slug).map_err(UserError::from)?; - with_spinner( - "Validating model parameters...", - validate_prompt_data_patch( - ctx, - None, - &mut definition, - args.prompt_config.refresh_models(), - ), + let validation = with_spinner( + "Validating scorer...", + api::validate_functions(&ctx.client, std::slice::from_ref(&definition)), ) - .await - .map_err(UserError::from)? - .warn_if_incomplete(); + .await?; + report_validation_issues(&validation).map_err(UserError::from)?; let result = match with_spinner( "Creating scorer...", @@ -182,6 +176,53 @@ pub(crate) async fn run(ctx: &ResolvedContext, args: &CreateArgs, json_output: b Ok(()) } +fn report_validation_issues(report: &api::FunctionValidationReport) -> Result<()> { + let mut blocking = Vec::new(); + for result in &report.results { + for issue in &result.issues { + let path = issue + .path + .iter() + .map(|part| { + part.as_str() + .map(ToOwned::to_owned) + .unwrap_or_else(|| part.to_string()) + }) + .collect::>() + .join("."); + let location = if path.is_empty() { + issue.code.clone() + } else { + path + }; + let suggestion = issue + .suggestion + .as_ref() + .map( + |suggestion| match (suggestion.action.as_str(), &suggestion.value) { + ("remove", _) => "; suggestion: remove this parameter".to_string(), + ("set", Some(value)) => format!("; suggestion: set it to {value}"), + _ => String::new(), + }, + ) + .unwrap_or_default(); + let message = format!("{location}: {}{suggestion}", issue.message); + if issue.blocking { + blocking.push(message); + } else { + print_command_status(CommandStatus::Warning, &message); + } + } + } + if blocking.is_empty() && report.valid { + Ok(()) + } else if blocking.is_empty() { + bail!("the backend rejected the scorer definition") + } else { + bail!(blocking.join("; ")) + } +} + fn resolve_name(args: &CreateArgs) -> Result { let name = match args.name_positional.as_deref().or(args.name.as_deref()) { Some(name) => name.trim().to_string(), diff --git a/src/functions/mod.rs b/src/functions/mod.rs index 09429bd7..03c5301a 100644 --- a/src/functions/mod.rs +++ b/src/functions/mod.rs @@ -17,7 +17,6 @@ pub(crate) mod create; mod delete; mod invoke; mod list; -mod model_capabilities; pub(crate) mod prompt_config; mod pull; mod push; diff --git a/src/functions/model_capabilities.rs b/src/functions/model_capabilities.rs deleted file mode 100644 index 99bef268..00000000 --- a/src/functions/model_capabilities.rs +++ /dev/null @@ -1,735 +0,0 @@ -use std::{ - collections::HashMap, - path::{Path, PathBuf}, - time::{Duration, SystemTime, UNIX_EPOCH}, -}; - -use anyhow::Result; -use serde::{Deserialize, Serialize}; -use serde_json::Value; -use sha2::{Digest, Sha256}; - -use crate::{ - http::build_http_client, - project_context::ProjectContext, - utils::{bt_cache_root, write_bytes_atomic}, -}; - -const MODEL_CATALOG_TIMEOUT: Duration = Duration::from_secs(5); -/// The catalog changes only on app deploy. A lookup miss refetches, so a newly -/// added custom model is still seen before the TTL expires. -const MODEL_CATALOG_TTL: Duration = Duration::from_secs(24 * 60 * 60); - -/// The model metadata used by the web UI to decide which controls and ranges -/// to expose. Unknown fields are intentionally ignored so newer app versions -/// can extend the catalog without breaking older CLI versions. -#[derive(Debug, Clone, Deserialize, Serialize)] -pub(crate) struct ModelSpec { - pub(crate) format: String, - pub(crate) flavor: String, - #[serde(default, rename = "displayName")] - pub(crate) display_name: Option, - #[serde(default)] - pub(crate) o1_like: Option, - #[serde(default)] - pub(crate) reasoning: Option, - #[serde(default)] - pub(crate) reasoning_budget: Option, - #[serde(default)] - pub(crate) max_output_tokens: Option, -} - -impl ModelSpec { - pub(crate) fn supports_reasoning(&self, model: &str) -> bool { - if self.reasoning.unwrap_or(false) || self.o1_like.unwrap_or(false) { - return true; - } - - // Match `modelProviderHasReasoning` from the web model catalog. The UI - // applies these fallbacks to custom models that omit `reasoning`. - let lower = model.to_ascii_lowercase(); - match self.format.as_str() { - "openai" => { - ["o1", "o2", "o3", "o4"] - .iter() - .any(|prefix| lower.starts_with(prefix)) - || lower.contains("gpt-5") - } - "anthropic" => lower.starts_with("claude-3.7"), - "google" => lower.ends_with("gemini-2.0-flash") || lower.contains("gemini-2.5"), - _ => false, - } - } -} - -#[derive(Debug, Deserialize)] -struct SecretWithMetadata { - #[serde(default)] - metadata: Option, -} - -#[derive(Debug, Deserialize)] -struct SecretListResponse { - objects: Vec, -} - -/// Both `Unknown` and `Unavailable` yield no spec, but only `Unavailable` means -/// the checks were skipped rather than deliberately not applicable. -#[derive(Debug, Clone)] -pub(crate) enum ModelLookup { - Found(ModelSpec), - /// Resolved from the on-disk cache after a fetch failed, so the spec may - /// predate changes made in the web UI. Yields a spec, but any limit it - /// implies is only as current as the last successful fetch. - Stale(ModelSpec), - /// Metadata loaded, but nothing defines this model. - Unknown, - /// Metadata could not be loaded; availability and ranges went unchecked. - Unavailable, -} - -impl ModelLookup { - pub(crate) fn spec(&self) -> Option<&ModelSpec> { - match self { - Self::Found(spec) | Self::Stale(spec) => Some(spec), - Self::Unknown | Self::Unavailable => None, - } - } - - pub(crate) fn is_unavailable(&self) -> bool { - matches!(self, Self::Unavailable) - } - - pub(crate) fn is_stale(&self) -> bool { - matches!(self, Self::Stale(_)) - } -} - -/// Resolve a model from the same shared catalog and configured custom-model -/// metadata used by the prompt UI. Project custom models take precedence over -/// org custom models, which take precedence over the shared catalog. Cached on -/// disk for [`MODEL_CATALOG_TTL`]. -/// -/// Missing metadata is non-fatal: callers fall back to provider-independent -/// validation so custom model names stay usable. -pub(crate) async fn resolve_model_lookup( - ctx: &ProjectContext, - model: &str, - refresh: bool, -) -> ModelLookup { - resolve_model_lookup_in(ctx, model, &catalog_cache_path(ctx), refresh).await -} - -async fn resolve_model_lookup_in( - ctx: &ProjectContext, - model: &str, - cache_path: &Path, - refresh: bool, -) -> ModelLookup { - if let Some(cached) = - read_cached_catalog(cache_path, Some(MODEL_CATALOG_TTL)).filter(|_| !refresh) - { - if let Some(spec) = resolve_from_models(&cached, model) { - return ModelLookup::Found(spec.clone()); - } - // Miss: fall through in case the model was added since we cached. - } - - let fetch = fetch_models(ctx).await; - if fetch.catalog_loaded && fetch.custom_models_loaded { - // A partial view would cache gaps as if they were absences. - let _ = write_cached_catalog(cache_path, &fetch.models); - } - - if fetch.custom_models_loaded { - if let Some(spec) = resolve_from_models(&fetch.models, model) { - return ModelLookup::Found(spec.clone()); - } - if fetch.catalog_loaded { - return ModelLookup::Unknown; - } - } - - // A partial fetch cannot establish precedence or absence. In particular, a - // shared match may be overridden by custom metadata we failed to load. The - // cache is consulted with no age bound here, including under `refresh`, so - // the result is reported as stale rather than current. - if let Some(cached) = read_cached_catalog(cache_path, None) { - if let Some(spec) = resolve_from_models(&cached, model) { - return ModelLookup::Stale(spec.clone()); - } - } - ModelLookup::Unavailable -} - -#[derive(Debug, Default)] -struct CatalogFetch { - models: HashMap, - catalog_loaded: bool, - custom_models_loaded: bool, -} - -async fn fetch_models(ctx: &ProjectContext) -> CatalogFetch { - let Ok(http) = build_http_client(MODEL_CATALOG_TIMEOUT) else { - return CatalogFetch::default(); - }; - let app_url = ctx.app_url.trim_end_matches('/'); - let catalog_url = format!("{app_url}/api/models/model_list.json"); - let org_secrets_url = format!("{app_url}/api/ai_secret/get"); - let project_secrets_path = format!( - "/v1/env_var?object_type=project&object_id={}&secret_category=ai_provider", - urlencoding::encode(&ctx.project.id), - ); - - let catalog_request = async { - let response = http.get(catalog_url).send().await.ok()?; - if !response.status().is_success() { - return None; - } - response.json::>().await.ok() - }; - let org_selector = if ctx.client.org_id().trim().is_empty() { - serde_json::json!({ "org_name": ctx.client.org_name() }) - } else { - serde_json::json!({ "org_id": ctx.client.org_id() }) - }; - let org_models_request = async { - let response = http - .post(org_secrets_url) - .bearer_auth(ctx.client.api_key()) - .json(&org_selector) - .send() - .await - .ok()?; - // Authentication and authorization failures do not prove that no - // custom models exist, so every unsuccessful response is inconclusive. - if !response.status().is_success() { - return None; - } - let secrets = response.json::>().await.ok()?; - custom_models_from_secrets(secrets) - }; - let project_models_request = async { - let response = tokio::time::timeout( - MODEL_CATALOG_TIMEOUT, - ctx.client.get::(&project_secrets_path), - ) - .await - .ok()?; - response - .ok() - .and_then(|response| custom_models_from_secrets(response.objects)) - }; - - let (catalog, org_models, project_models) = - tokio::join!(catalog_request, org_models_request, project_models_request); - - let catalog_loaded = catalog.is_some(); - let custom_models_loaded = org_models.is_some() && project_models.is_some(); - - let mut models = catalog.unwrap_or_default(); - models.extend(org_models.unwrap_or_default()); - models.extend(project_models.unwrap_or_default()); - - CatalogFetch { - models, - catalog_loaded, - custom_models_loaded, - } -} - -#[derive(Debug, Deserialize)] -struct CachedCatalog { - fetched_at: u64, - models: HashMap, -} - -#[derive(Debug, Serialize)] -struct CatalogToCache<'a> { - fetched_at: u64, - models: &'a HashMap, -} - -/// Keyed by app URL, org, and project because custom models are org- and -/// project-scoped. Version in the path invalidates on [`ModelSpec`] changes. -fn catalog_cache_path(ctx: &ProjectContext) -> PathBuf { - let mut hasher = Sha256::new(); - for part in [ - ctx.app_url.trim_end_matches('/'), - ctx.client.org_id(), - ctx.client.org_name(), - ctx.project.id.as_str(), - ] { - hasher.update(part.as_bytes()); - hasher.update([0]); - } - let key = hasher - .finalize() - .iter() - .take(8) - .map(|byte| format!("{byte:02x}")) - .collect::(); - - bt_cache_root() - .join("model-catalog") - .join(env!("CARGO_PKG_VERSION")) - .join(format!("{key}.json")) -} - -/// `max_age` of `None` accepts a cache of any age. -fn read_cached_catalog( - path: &Path, - max_age: Option, -) -> Option> { - let raw = std::fs::read(path).ok()?; - let cached: CachedCatalog = serde_json::from_slice(&raw).ok()?; - - if let Some(max_age) = max_age { - let now = SystemTime::now().duration_since(UNIX_EPOCH).ok()?.as_secs(); - // Saturate: a future timestamp is clock skew, not infinite freshness. - if Duration::from_secs(now.saturating_sub(cached.fetched_at)) > max_age { - return None; - } - } - - Some(cached.models) -} - -fn write_cached_catalog(path: &Path, models: &HashMap) -> Result<()> { - let fetched_at = SystemTime::now().duration_since(UNIX_EPOCH)?.as_secs(); - let payload = serde_json::to_vec(&CatalogToCache { fetched_at, models })?; - if let Some(parent) = path.parent() { - std::fs::create_dir_all(parent)?; - } - write_bytes_atomic(path, &payload) -} - -fn custom_models_from_secrets( - secrets: Vec, -) -> Option> { - let mut result = HashMap::new(); - for metadata in secrets.into_iter().filter_map(|secret| secret.metadata) { - let Some(models) = metadata.get("customModels") else { - continue; - }; - // A malformed custom-model entry makes absence inconclusive. Do not - // silently turn a parsing failure into an authoritative empty catalog. - let models = serde_json::from_value::>(models.clone()).ok()?; - // Preserve formats and flavors introduced by newer app versions. The - // caller treats unknown capabilities conservatively rather than making - // every custom model in this scope unavailable. - result.extend(models); - } - Some(result) -} - -fn resolve_from_models<'a>( - models: &'a HashMap, - model: &str, -) -> Option<&'a ModelSpec> { - // displayName is a UI label, not an identifier accepted by providers. - models.get(model) -} - -#[cfg(test)] -mod tests { - use actix_web::{dev::ServerHandle, http::StatusCode, web, App, HttpResponse, HttpServer}; - - use braintrust_sdk_rust::LoginState; - - use crate::{auth::LoginContext, http::ApiClient, projects::api::Project}; - - use super::*; - - fn spec(format: &str, display_name: Option<&str>) -> ModelSpec { - ModelSpec { - format: format.to_string(), - flavor: "chat".to_string(), - display_name: display_name.map(ToOwned::to_owned), - o1_like: None, - reasoning: None, - reasoning_budget: None, - max_output_tokens: None, - } - } - - #[test] - fn extracts_custom_models_from_secret_metadata() { - let secrets = vec![SecretWithMetadata { - metadata: Some(serde_json::json!({ - "customModels": { - "test-custom-model": { - "format": "anthropic", - "flavor": "chat", - "max_output_tokens": 4096 - } - } - })), - }]; - - let models = custom_models_from_secrets(secrets).expect("valid custom models"); - let model = models.get("test-custom-model").expect("custom model"); - assert_eq!(model.format, "anthropic"); - assert_eq!(model.max_output_tokens, Some(4096)); - } - - #[test] - fn applies_web_ui_reasoning_fallbacks_for_custom_models() { - let openai = spec("openai", None); - assert!(openai.supports_reasoning("o3")); - assert!(openai.supports_reasoning("test-gpt-5-deployment")); - assert!(!openai.supports_reasoning("gpt-4.1")); - - let anthropic = spec("anthropic", None); - assert!(anthropic.supports_reasoning("claude-3.7-sonnet")); - } - - #[test] - fn resolves_catalog_models_only_by_id() { - let models = HashMap::from([( - "test-model-id".to_string(), - spec("openai", Some("Test model")), - )]); - - assert!(resolve_from_models(&models, "test-model-id").is_some()); - assert!(resolve_from_models(&models, "Test model").is_none()); - assert!(resolve_from_models(&models, "missing").is_none()); - } - - #[test] - fn malformed_custom_models_make_the_result_inconclusive() { - for model in [ - serde_json::json!({"format": 42, "flavor": "chat"}), - serde_json::json!({"format": "openai"}), - ] { - let secrets = vec![SecretWithMetadata { - metadata: Some(serde_json::json!({ - "customModels": {"test-broken-model": model} - })), - }]; - - assert!(custom_models_from_secrets(secrets).is_none()); - } - } - - #[test] - fn preserves_unknown_custom_model_capabilities() { - let secrets = vec![SecretWithMetadata { - metadata: Some(serde_json::json!({ - "customModels": { - "test-future-model": { - "format": "test-future-format", - "flavor": "test-future-flavor" - } - } - })), - }]; - - let models = custom_models_from_secrets(secrets).expect("valid custom models"); - let spec = models.get("test-future-model").expect("future model"); - assert_eq!(spec.format, "test-future-format"); - assert_eq!(spec.flavor, "test-future-flavor"); - } - - struct MockServer { - base_url: String, - handle: ServerHandle, - } - - impl MockServer { - async fn start() -> Self { - Self::start_with_org_status(StatusCode::OK).await - } - - async fn start_with_org_status(org_status: StatusCode) -> Self { - let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("bind mock server"); - let address = listener.local_addr().expect("mock server address"); - let base_url = format!("http://{address}"); - let server = HttpServer::new(move || { - App::new() - .route( - "/api/models/model_list.json", - web::get().to(|| async { - HttpResponse::Ok().json(serde_json::json!({ - "test-shared-model": { - "format": "openai", - "flavor": "chat", - "max_output_tokens": 100 - } - })) - }), - ) - .route( - "/api/ai_secret/get", - web::post().to(move || async move { - if !org_status.is_success() { - return HttpResponse::build(org_status).finish(); - } - HttpResponse::Ok().json(serde_json::json!([{ - "metadata": { - "customModels": { - "test-precedence-model": { - "format": "anthropic", - "flavor": "chat", - "max_output_tokens": 200 - } - } - } - }])) - }), - ) - .route( - "/v1/env_var", - web::get().to(|| async { - HttpResponse::Ok().json(serde_json::json!({ - "objects": [{ - "metadata": { - "customModels": { - "test-precedence-model": { - "format": "google", - "flavor": "chat", - "max_output_tokens": 300 - } - } - } - }] - })) - }), - ) - }) - .workers(1) - .listen(listener) - .expect("listen mock server") - .run(); - let handle = server.handle(); - tokio::spawn(server); - Self { base_url, handle } - } - - async fn stop(self) { - self.handle.stop(false).await; - } - } - - fn test_context(base_url: &str) -> ProjectContext { - let login = LoginState::new(); - login.set( - "test-key".to_string(), - "test-org-id".to_string(), - "test-org".to_string(), - base_url.to_string(), - base_url.to_string(), - ); - let client = ApiClient::new(&LoginContext { - login, - api_url: base_url.to_string(), - app_url: base_url.to_string(), - profile: None, - }) - .expect("API client"); - ProjectContext { - client, - app_url: base_url.to_string(), - project: Project { - id: "test-project-id".to_string(), - name: "test-project".to_string(), - org_id: "test-org-id".to_string(), - description: None, - }, - } - } - - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn resolves_shared_and_custom_models_with_project_precedence() { - let server = MockServer::start().await; - let ctx = test_context(&server.base_url); - let cache = tempfile::tempdir().expect("tempdir"); - let cache_path = cache.path().join("catalog.json"); - - let shared = resolve_model_lookup_in(&ctx, "test-shared-model", &cache_path, false) - .await - .spec() - .cloned() - .expect("shared model"); - assert_eq!(shared.format, "openai"); - assert_eq!(shared.max_output_tokens, Some(100)); - - let custom = resolve_model_lookup_in(&ctx, "test-precedence-model", &cache_path, false) - .await - .spec() - .cloned() - .expect("custom model"); - assert_eq!(custom.format, "google"); - assert_eq!(custom.max_output_tokens, Some(300)); - - server.stop().await; - } - - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn a_shared_match_is_unavailable_when_custom_models_cannot_be_loaded() { - let server = MockServer::start_with_org_status(StatusCode::FORBIDDEN).await; - let ctx = test_context(&server.base_url); - let cache = tempfile::tempdir().expect("tempdir"); - - let lookup = resolve_model_lookup_in( - &ctx, - "test-shared-model", - &cache.path().join("catalog.json"), - false, - ) - .await; - assert!(lookup.is_unavailable()); - assert!(lookup.spec().is_none()); - - server.stop().await; - } - - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn a_known_model_is_unknown_not_unavailable() { - let server = MockServer::start().await; - let ctx = test_context(&server.base_url); - let cache = tempfile::tempdir().expect("tempdir"); - - let lookup = resolve_model_lookup_in( - &ctx, - "test-absent-model", - &cache.path().join("catalog.json"), - false, - ) - .await; - assert!(matches!(lookup, ModelLookup::Unknown)); - assert!(!lookup.is_unavailable()); - - server.stop().await; - } - - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn a_fresh_cache_serves_lookups_after_the_server_is_gone() { - let server = MockServer::start().await; - let ctx = test_context(&server.base_url); - let cache = tempfile::tempdir().expect("tempdir"); - let cache_path = cache.path().join("catalog.json"); - - resolve_model_lookup_in(&ctx, "test-shared-model", &cache_path, false) - .await - .spec() - .expect("warm the cache"); - server.stop().await; - - // Nothing is listening, so a hit can only be from the cache. - let cached = resolve_model_lookup_in(&ctx, "test-shared-model", &cache_path, false) - .await - .spec() - .cloned() - .expect("cached model"); - assert_eq!(cached.max_output_tokens, Some(100)); - } - - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn refresh_bypasses_a_fresh_cache() { - let cache = tempfile::tempdir().expect("tempdir"); - let cache_path = cache.path().join("catalog.json"); - // A fresh cache claiming a model the server does not serve. - write_cached_catalog( - &cache_path, - &HashMap::from([("test-only-in-cache".to_string(), spec("openai", None))]), - ) - .expect("seed cache"); - - let server = MockServer::start().await; - let ctx = test_context(&server.base_url); - - assert!( - resolve_model_lookup_in(&ctx, "test-only-in-cache", &cache_path, false) - .await - .spec() - .is_some(), - "the cached entry should satisfy a normal lookup" - ); - assert!( - resolve_model_lookup_in(&ctx, "test-only-in-cache", &cache_path, true) - .await - .spec() - .is_none(), - "refresh should ignore the cache and see only the server's catalog" - ); - - server.stop().await; - } - - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn an_unreachable_catalog_without_a_cache_is_unavailable() { - // Port 9 refuses connections. - let ctx = test_context("http://127.0.0.1:9"); - let cache = tempfile::tempdir().expect("tempdir"); - - let lookup = resolve_model_lookup_in( - &ctx, - "gpt-4.1-mini", - &cache.path().join("catalog.json"), - false, - ) - .await; - assert!(lookup.is_unavailable()); - assert!(lookup.spec().is_none()); - } - - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn an_unreachable_catalog_falls_back_to_a_stale_cache() { - let cache = tempfile::tempdir().expect("tempdir"); - let cache_path = cache.path().join("catalog.json"); - let models = HashMap::from([("test-stale-model".to_string(), spec("openai", None))]); - write_cached_catalog(&cache_path, &models).expect("seed cache"); - // Backdated past the TTL, so only the stale-fallback path can hit. - let stale = serde_json::json!({ "fetched_at": 0, "models": models }); - std::fs::write(&cache_path, serde_json::to_vec(&stale).unwrap()).expect("backdate cache"); - - let ctx = test_context("http://127.0.0.1:9"); - let lookup = resolve_model_lookup_in(&ctx, "test-stale-model", &cache_path, false).await; - assert_eq!(lookup.spec().expect("stale model").format, "openai"); - } - - #[test] - fn cached_catalog_round_trips_and_expires() { - let cache = tempfile::tempdir().expect("tempdir"); - let cache_path = cache.path().join("catalog.json"); - let models = HashMap::from([( - "test-model-id".to_string(), - spec("anthropic", Some("Test model")), - )]); - - write_cached_catalog(&cache_path, &models).expect("write cache"); - let read = read_cached_catalog(&cache_path, Some(MODEL_CATALOG_TTL)).expect("fresh cache"); - assert_eq!(read["test-model-id"].format, "anthropic"); - assert_eq!( - read["test-model-id"].display_name.as_deref(), - Some("Test model") - ); - - // Past the TTL: rejected as fresh, still returned when unbounded. - let now = SystemTime::now() - .duration_since(UNIX_EPOCH) - .expect("clock") - .as_secs(); - let stale = serde_json::json!({ - "fetched_at": now - MODEL_CATALOG_TTL.as_secs() - 60, - "models": models, - }); - std::fs::write(&cache_path, serde_json::to_vec(&stale).expect("encode")) - .expect("backdate cache"); - assert!(read_cached_catalog(&cache_path, Some(MODEL_CATALOG_TTL)).is_none()); - assert!(read_cached_catalog(&cache_path, None).is_some()); - } - - #[test] - fn cache_paths_are_scoped_per_org_and_project() { - let a = catalog_cache_path(&test_context("https://app.test.example")); - let mut other = test_context("https://app.test.example"); - other.project.id = "test-other-project".to_string(); - let b = catalog_cache_path(&other); - - assert_ne!(a, b); - assert!(a.starts_with(bt_cache_root().join("model-catalog"))); - assert!(a.to_string_lossy().contains(env!("CARGO_PKG_VERSION"))); - } -} diff --git a/src/functions/prompt_config.rs b/src/functions/prompt_config.rs index c9e6ef76..f77cb095 100644 --- a/src/functions/prompt_config.rs +++ b/src/functions/prompt_config.rs @@ -4,13 +4,7 @@ use anyhow::{bail, Context, Result}; use clap::{builder::BoolishValueParser, Args, ValueEnum}; use serde_json::{json, Map, Number, Value}; -use crate::{ - project_context::ProjectContext, - ui::{print_command_status, CommandStatus}, - utils::{merge_json_objects, read_text_source}, -}; - -use super::model_capabilities::{resolve_model_lookup, ModelLookup, ModelSpec}; +use crate::utils::read_text_source; #[derive(Debug, Clone, Default, Args)] pub(crate) struct PromptConfigArgs { @@ -89,11 +83,6 @@ pub(crate) struct PromptConfigArgs { /// format; `nunjucks` and `jinja2` are accepted aliases. #[arg(long, value_enum, value_name = "FORMAT")] template_format: Option, - - /// Refetch the model catalog instead of using the local 24h cache. Use this - /// after editing custom models in the web UI. - #[arg(long)] - refresh_models: bool, } #[derive(Debug, Clone, Copy, ValueEnum)] @@ -153,10 +142,6 @@ impl TemplateFormat { } impl PromptConfigArgs { - pub(crate) fn refresh_models(&self) -> bool { - self.refresh_models - } - /// Build a partial `prompt_data` object matching the app's prompt schema. pub(crate) fn build_prompt_data_patch( &self, @@ -174,21 +159,11 @@ impl PromptConfigArgs { options.insert("model".to_string(), Value::String(model.to_string())); } - let temperature = match (self.temperature, self.use_cache) { - (None, Some(true)) => Some(0.0), - (Some(temperature), Some(true)) if temperature != 0.0 => { - bail!("--use-cache=true requires --temperature=0") - } - (temperature, _) => temperature, - }; - insert_optional_number(&mut params, "temperature", temperature)?; + insert_optional_number(&mut params, "temperature", self.temperature)?; if let Some(max_tokens) = self.max_tokens { params.insert("max_tokens".to_string(), Value::Number(max_tokens.into())); } - if let Some(top_p) = self.top_p { - validate_unit_interval(top_p, "--top-p")?; - insert_number(&mut params, "top_p", top_p, "--top-p")?; - } + insert_optional_number(&mut params, "top_p", self.top_p)?; if let Some(top_k) = self.top_k { params.insert("top_k".to_string(), Value::Number(top_k.into())); } @@ -253,10 +228,6 @@ impl PromptConfigArgs { ); } - let effective_model = options.get("model").and_then(Value::as_str); - let changed_params = params.keys().cloned().collect(); - validate_model_params(effective_model, ¶ms, &changed_params, None)?; - if !params.is_empty() { options.insert("params".to_string(), Value::Object(params)); } @@ -326,550 +297,6 @@ fn validate_json_schema_response_format(format: &Map) -> Result<( Ok(()) } -/// Whether the model-specific half of validation ran. Capability metadata comes -/// over the network, so it can be missing — not an error, but worth reporting. -#[derive(Debug, Clone, PartialEq, Eq)] -pub(crate) enum ValidationOutcome { - /// Capabilities were resolved and checked, or nothing needed checking. - Checked, - /// Metadata loaded successfully, but did not contain this model. Basic - /// checks ran because the catalog is not authoritative for every model. - ModelUnknown { model: String }, - /// Only provider-independent type and range checks ran. - CapabilitiesUnavailable { model: String }, - /// Checks ran, but against a cached spec the fetch could not refresh. - CapabilitiesStale { model: String }, -} - -impl ValidationOutcome { - /// Call after any spinner finishes, or its repainting overwrites this. - pub(crate) fn warn_if_incomplete(&self) { - let message = match self { - Self::Checked => return, - Self::ModelUnknown { model } => format!( - "Model '{model}' was not found in the shared catalog or configured custom models; \ - checked only basic value ranges. Check the model ID or pass --refresh-models if \ - it was recently configured." - ), - Self::CapabilitiesUnavailable { model } => format!( - "Could not load model metadata for '{model}'; checked only basic value ranges. \ - Confirm this model supports the parameters you set." - ), - Self::CapabilitiesStale { model } => format!( - "Could not refresh model metadata for '{model}'; checked only basic value ranges. \ - Provider-specific validation and parameter normalization were skipped." - ), - }; - print_command_status(CommandStatus::Warning, &message); - } -} - -/// Validate `patch` against the model it will apply to and normalize parameter -/// names into the form expected by the API and prompt editor. -/// -/// Validation judges the effective configuration — `patch` merged over -/// `existing_prompt_data` — not the patch alone. Without capability metadata, -/// only provider-independent checks run and the outcome says so. -pub(crate) async fn validate_prompt_data_patch( - ctx: &ProjectContext, - existing_prompt_data: Option<&Value>, - patch: &mut Value, - refresh_models: bool, -) -> Result { - // `changed_params` must reflect what the caller actually set. - let update = prepare_model_params_update(existing_prompt_data, &*patch)?; - let lookup = match update.as_ref().and_then(|update| update.model.as_deref()) { - Some(model) => Some(resolve_model_lookup(ctx, model, refresh_models).await), - None => None, - }; - let spec = lookup.as_ref().and_then(ModelLookup::spec); - let stale = lookup.as_ref().is_some_and(ModelLookup::is_stale); - // Stale metadata must neither reject nor rewrite a configuration that may - // have become valid since it was cached. Canonical parameter names remain - // accepted by the proxy even when UI-specific normalization is skipped. - let current_spec = if stale { None } else { spec }; - if let Some(update) = &update { - validate_model_params( - update.model.as_deref(), - &update.params, - &update.changed_params, - current_spec, - )?; - } - - apply_provider_param_names(patch, current_spec); - - // An error is its own signal; only report skipped checks otherwise. - match (lookup, update.and_then(|update| update.model)) { - (Some(ModelLookup::Unknown), Some(model)) => Ok(ValidationOutcome::ModelUnknown { model }), - (Some(lookup), Some(model)) if lookup.is_unavailable() => { - Ok(ValidationOutcome::CapabilitiesUnavailable { model }) - } - (Some(lookup), Some(model)) if lookup.is_stale() => { - Ok(ValidationOutcome::CapabilitiesStale { model }) - } - _ => Ok(ValidationOutcome::Checked), - } -} - -/// Google prompts store these two under different keys. The proxy maps both -/// spellings, but the prompt editor reads only these, so the canonical names -/// leave its fields uninitialized. -const GOOGLE_PARAM_NAMES: &[(&str, &str)] = &[ - ("max_tokens", "maxOutputTokens"), - ("top_p", "topP"), - ("top_k", "topK"), -]; - -/// Rewrite parameter names to the spelling `spec`'s format expects. An unknown -/// format keeps the canonical names. -fn apply_provider_param_names(patch: &mut Value, spec: Option<&ModelSpec>) { - if spec.map(|spec| spec.format.as_str()) != Some("google") { - return; - } - let Some(params) = patch - .get_mut("prompt_data") - .and_then(|prompt_data| prompt_data.get_mut("options")) - .and_then(|options| options.get_mut("params")) - .and_then(Value::as_object_mut) - else { - return; - }; - for (canonical, google) in GOOGLE_PARAM_NAMES { - if let Some(value) = params.remove(*canonical) { - params.insert((*google).to_string(), value); - } - } -} - -#[derive(Debug)] -struct ModelParamsUpdate { - model: Option, - params: Map, - changed_params: HashSet, -} - -fn prepare_model_params_update( - existing_prompt_data: Option<&Value>, - patch: &Value, -) -> Result> { - let Some(patch_prompt_data) = patch.get("prompt_data").and_then(Value::as_object) else { - return Ok(None); - }; - let Some(patch_options_value) = patch_prompt_data.get("options") else { - return Ok(None); - }; - if patch_options_value.is_null() { - return Ok(None); - } - let patch_options = patch_options_value - .as_object() - .ok_or_else(|| anyhow::anyhow!("prompt_data.options must be a JSON object"))?; - - let mut effective_prompt_data = existing_prompt_data - .and_then(Value::as_object) - .cloned() - .unwrap_or_default(); - merge_json_objects(&mut effective_prompt_data, patch_prompt_data); - - let options = effective_prompt_data - .get("options") - .and_then(Value::as_object) - .ok_or_else(|| anyhow::anyhow!("prompt_data.options must be a JSON object"))?; - let model = match options.get("model") { - Some(Value::String(model)) if !model.trim().is_empty() => Some(model.trim().to_string()), - Some(Value::String(_)) => bail!("model cannot be empty"), - Some(Value::Null) | None => None, - Some(_) => bail!("prompt_data.options.model must be a string"), - }; - let params = match options.get("params") { - Some(Value::Object(params)) => params.clone(), - Some(Value::Null) | None => Map::new(), - Some(_) => bail!("prompt_data.options.params must be a JSON object"), - }; - - let model_changed = patch_options.contains_key("model"); - let mut changed_params = if model_changed { - params.keys().cloned().collect() - } else { - match patch_options.get("params") { - Some(Value::Object(params)) => params.keys().cloned().collect(), - Some(Value::Null) | None => HashSet::new(), - Some(_) => bail!("prompt_data.options.params must be a JSON object"), - } - }; - - // Temperature support can depend on reasoning effort, so changing either - // side of that relationship must validate the effective temperature. - if changed_params.contains("reasoning_effort") && params.contains_key("temperature") { - changed_params.insert("temperature".to_string()); - } - - // A model-only patch still needs lookup so miss/unavailable warnings are - // emitted even when the user did not set any optional model parameters. - if changed_params.is_empty() && !model_changed { - return Ok(None); - } - - Ok(Some(ModelParamsUpdate { - model, - params, - changed_params, - })) -} - -fn validate_model_params( - model: Option<&str>, - params: &Map, - changed_params: &HashSet, - spec: Option<&ModelSpec>, -) -> Result<()> { - if changed_params.contains("temperature") { - ensure_parameter_supported(spec, model, "--temperature", TEMPERATURE_FORMATS)?; - if let Some(temperature) = optional_number(params, "temperature", "--temperature")? { - let max = match spec.map(|spec| spec.format.as_str()) { - Some("anthropic" | "converse") => 1.0, - _ => 2.0, - }; - validate_number_range(temperature, 0.0, max, "--temperature")?; - if let Some(model) = model { - validate_temperature_support(model, params)?; - } - } - } - - if changed_params.contains("top_p") { - ensure_parameter_supported(spec, model, "--top-p", TOP_P_FORMATS)?; - if let Some(top_p) = optional_number(params, "top_p", "--top-p")? { - validate_number_range(top_p, 0.0, 1.0, "--top-p")?; - if let Some(model) = model.filter(|model| has_unsupported_opus_sampling_params(model)) { - bail!("--top-p is not supported by model '{model}'"); - } - } - } - - if changed_params.contains("top_k") { - ensure_parameter_supported(spec, model, "--top-k", TOP_K_FORMATS)?; - match params.get("top_k") { - Some(Value::Null) | None => {} - Some(Value::Number(number)) - if number - .as_u64() - .is_some_and(|value| (1..=100).contains(&value)) => {} - _ => bail!("--top-k must be an integer between 1 and 100"), - } - if let Some(model) = model.filter(|model| has_unsupported_opus_sampling_params(model)) { - bail!("--top-k is not supported by model '{model}'"); - } - } - - for (key, label) in [ - ("frequency_penalty", "--frequency-penalty"), - ("presence_penalty", "--presence-penalty"), - ] { - if changed_params.contains(key) { - ensure_parameter_supported(spec, model, label, PENALTY_FORMATS)?; - if let Some(value) = optional_number(params, key, label)? { - // The UI slider currently exposes 0..=1, but the backend's - // OpenAI schema and provider API accept the full -2..=2 range. - validate_number_range(value, -2.0, 2.0, label)?; - } - } - } - - if changed_params.contains("max_tokens") { - ensure_parameter_supported(spec, model, "--max-tokens", MAX_TOKENS_FORMATS)?; - if let Some(max_tokens) = params.get("max_tokens") { - match max_tokens { - Value::Null => {} - Value::Number(number) if number.as_u64().is_some_and(|value| value > 0) => { - if let (Some(spec), Some(value)) = (spec, number.as_u64()) { - let max = spec - .max_output_tokens - .filter(|max| *max > 0) - .unwrap_or(32_768); - if value > max { - bail!( - "--max-tokens must be between 1 and {max} for model '{}'", - model.unwrap_or("") - ); - } - } - } - _ => bail!("--max-tokens must be a positive integer"), - } - } - } - - if changed_params.contains("stop") { - match params.get("stop") { - Some(Value::Null) | None => {} - Some(Value::Array(values)) if values.iter().all(Value::is_string) => {} - _ => bail!("--stop-sequence values must be strings"), - } - } - - if changed_params.contains("tool_choice") { - if let Some(value) = params.get("tool_choice").filter(|value| !value.is_null()) { - ensure_parameter_supported(spec, model, "--tool-choice", TOOL_FORMATS)?; - validate_tool_choice(value)?; - } - } - - if changed_params.contains("reasoning_effort") { - validate_reasoning_effort(model, params, spec)?; - } - - if changed_params.contains("reasoning_enabled") || changed_params.contains("reasoning_budget") { - validate_reasoning_budget_params(model, params, spec)?; - } - - if changed_params.contains("verbosity") { - if let Some(value) = params.get("verbosity").filter(|value| !value.is_null()) { - let verbosity = value - .as_str() - .ok_or_else(|| anyhow::anyhow!("--verbosity must be a string"))?; - if !["low", "medium", "high"].contains(&verbosity) { - bail!("--verbosity must be one of low, medium, high"); - } - if let (Some(model), Some(spec)) = (model, spec) { - // Match the prompt UI: displayName is the visible model name - // when present; otherwise the model identifier is used. - let name = spec.display_name.as_deref().unwrap_or(model); - if !name.to_ascii_lowercase().contains("gpt-5") { - bail!("--verbosity is not supported by model '{model}'"); - } - } - } - } - - Ok(()) -} - -// Keep these in sync with `defaultModelParamSettings`, `getSliderSpecs`, and -// `modelProviderHasTools` in `proxy/packages/proxy/schema/index.ts`. -const KNOWN_FORMATS: &[&str] = &["openai", "anthropic", "google", "js", "window", "converse"]; -const TEMPERATURE_FORMATS: &[&str] = &["openai", "anthropic", "google", "window", "converse"]; -const MAX_TOKENS_FORMATS: &[&str] = &["openai", "anthropic", "google", "converse"]; -const TOP_P_FORMATS: &[&str] = &["openai", "anthropic", "google", "converse"]; -const TOP_K_FORMATS: &[&str] = &["anthropic", "google", "window"]; -const PENALTY_FORMATS: &[&str] = &["openai"]; -const TOOL_FORMATS: &[&str] = &["openai", "anthropic", "google", "converse"]; - -fn ensure_parameter_supported( - spec: Option<&ModelSpec>, - model: Option<&str>, - label: &str, - supported_formats: &[&str], -) -> Result<()> { - let Some(spec) = spec else { - return Ok(()); - }; - if KNOWN_FORMATS.contains(&spec.format.as_str()) - && !supported_formats.contains(&spec.format.as_str()) - { - bail!( - "{label} is not supported by model '{}' (format: {})", - model.unwrap_or(""), - spec.format, - ); - } - Ok(()) -} - -fn validate_tool_choice(value: &Value) -> Result<()> { - match value { - Value::String(choice) if ["auto", "none", "required"].contains(&choice.as_str()) => Ok(()), - Value::Object(choice) - if choice.get("type").and_then(Value::as_str) == Some("function") - && choice - .get("function") - .and_then(Value::as_object) - .and_then(|function| function.get("name")) - .and_then(Value::as_str) - .is_some_and(|name| !name.trim().is_empty()) => - { - Ok(()) - } - _ => bail!("--tool-choice must be auto, none, required, or a non-empty function name"), - } -} - -fn validate_reasoning_effort( - model: Option<&str>, - params: &Map, - spec: Option<&ModelSpec>, -) -> Result<()> { - let Some(effort) = params.get("reasoning_effort") else { - return Ok(()); - }; - if effort.is_null() { - return Ok(()); - } - let effort = effort - .as_str() - .ok_or_else(|| anyhow::anyhow!("--reasoning-effort must be a string"))?; - let (Some(model), Some(spec)) = (model, spec) else { - return Ok(()); - }; - if !KNOWN_FORMATS.contains(&spec.format.as_str()) { - return Ok(()); - } - if !spec.supports_reasoning(model) { - bail!("--reasoning-effort is not supported by model '{model}'"); - } - - let gemini_thinking_level = spec.format == "google" && is_gemini_3_model(model); - if spec.format != "openai" && spec.reasoning_budget.unwrap_or(false) && !gemini_thinking_level { - bail!( - "--reasoning-effort is not supported by model '{model}'; this model uses a reasoning budget" - ); - } - - let options = reasoning_effort_options(model, gemini_thinking_level); - if !options.contains(&effort) { - bail!( - "--reasoning-effort must be one of {} for model '{model}'", - options.join(", ") - ); - } - Ok(()) -} - -fn validate_reasoning_budget_params( - model: Option<&str>, - params: &Map, - spec: Option<&ModelSpec>, -) -> Result<()> { - if let Some(enabled) = params.get("reasoning_enabled") { - if !enabled.is_null() && !enabled.is_boolean() { - bail!("--reasoning-enabled must be true or false"); - } - } - if let Some(budget) = params.get("reasoning_budget") { - match budget { - Value::Null => {} - Value::Number(number) if number.as_u64().is_some_and(|value| value <= 32_768) => {} - _ => bail!("--reasoning-budget must be an integer between 0 and 32768"), - } - } - - let (Some(model), Some(spec)) = (model, spec) else { - return Ok(()); - }; - let uses_budget = spec.format != "openai" - && spec.reasoning_budget.unwrap_or(false) - && !(spec.format == "google" && is_gemini_3_model(model)); - if !uses_budget { - bail!("--reasoning-enabled and --reasoning-budget are not supported by model '{model}'"); - } - if !spec.supports_reasoning(model) { - bail!("reasoning is not supported by model '{model}'"); - } - Ok(()) -} - -fn optional_number(params: &Map, key: &str, label: &str) -> Result> { - match params.get(key) { - Some(Value::Null) | None => Ok(None), - Some(Value::Number(number)) => number - .as_f64() - .filter(|value| value.is_finite()) - .map(Some) - .ok_or_else(|| anyhow::anyhow!("{label} must be a finite number")), - Some(_) => bail!("{label} must be a number"), - } -} - -fn validate_number_range(value: f64, min: f64, max: f64, label: &str) -> Result<()> { - if !value.is_finite() || !(min..=max).contains(&value) { - bail!("{label} must be between {min} and {max}"); - } - Ok(()) -} - -fn reasoning_effort_options(model: &str, gemini_thinking_level: bool) -> &'static [&'static str] { - if is_gpt_5_pro_model(model) { - &["high"] - } else if is_gpt_5_1_or_later(model) { - &["none", "low", "medium", "high"] - } else if is_gpt_5_model(model) || gemini_thinking_level { - &["minimal", "low", "medium", "high"] - } else { - &["low", "medium", "high"] - } -} - -/// Keep this in sync with `modelSupportsCustomTemperature` in the backend's -/// `typespecs/src/model-capabilities.ts`. -fn validate_temperature_support(model: &str, params: &Map) -> Result<()> { - let lower = model.to_ascii_lowercase(); - - if lower.contains("claude-opus-4-7") { - bail!("--temperature is not supported by model '{model}'"); - } - - if lower.contains("gpt-5") { - let has_no_reasoning_effort = params - .get("reasoning_effort") - .and_then(Value::as_str) - .is_some_and(|effort| effort == "none"); - if !has_no_reasoning_effort { - // Only advise `--reasoning-effort none` on models that accept it; - // on the rest temperature is simply unusable. - if reasoning_effort_options(model, false).contains(&"none") { - bail!( - "--temperature is not supported by model '{model}' unless reasoning effort is 'none'; pass `--reasoning-effort none` or omit `--temperature`" - ); - } - bail!("--temperature is not supported by model '{model}'"); - } - } else if ["o1", "o2", "o3", "o4"] - .iter() - .any(|prefix| lower.starts_with(prefix)) - { - bail!("--temperature is not supported by model '{model}'"); - } - - Ok(()) -} - -fn has_unsupported_opus_sampling_params(model: &str) -> bool { - model.to_ascii_lowercase().contains("claude-opus-4-7") -} - -fn is_gpt_5_pro_model(model: &str) -> bool { - model.to_ascii_lowercase().contains("gpt-5-pro") -} - -fn is_gpt_5_1_or_later(model: &str) -> bool { - let lower = model.to_ascii_lowercase(); - for marker in ["gpt-5.", "databricks-gpt-5-"] { - let Some(start) = lower.find(marker) else { - continue; - }; - let version = lower[start + marker.len()..] - .chars() - .take_while(char::is_ascii_digit) - .collect::(); - if version.parse::().is_ok_and(|version| version >= 1) { - return true; - } - } - false -} - -fn is_gpt_5_model(model: &str) -> bool { - model.to_ascii_lowercase().contains("gpt-5") - && !is_gpt_5_1_or_later(model) - && !is_gpt_5_pro_model(model) -} - -fn is_gemini_3_model(model: &str) -> bool { - let lower = model.to_ascii_lowercase(); - lower.starts_with("gemini-3") || lower.contains("/gemini-3") -} - fn insert_optional_number( target: &mut Map, key: &str, @@ -964,39 +391,6 @@ mod tests { config: PromptConfigArgs, } - fn model_spec( - format: &str, - reasoning: bool, - reasoning_budget: bool, - max_output_tokens: Option, - ) -> ModelSpec { - ModelSpec { - format: format.to_string(), - flavor: "chat".to_string(), - display_name: None, - o1_like: None, - reasoning: Some(reasoning), - reasoning_budget: Some(reasoning_budget), - max_output_tokens, - } - } - - fn validate_patch( - existing_prompt_data: Option<&Value>, - patch: &Value, - spec: Option<&ModelSpec>, - ) -> Result<()> { - let Some(update) = prepare_model_params_update(existing_prompt_data, patch)? else { - return Ok(()); - }; - validate_model_params( - update.model.as_deref(), - &update.params, - &update.changed_params, - spec, - ) - } - #[test] fn builds_web_ui_compatible_prompt_configuration() { let args = Harness::try_parse_from([ @@ -1062,31 +456,18 @@ mod tests { } #[test] - fn enabling_cache_sets_the_temperature_required_by_the_web_ui() { - let args = Harness::try_parse_from(["test", "--use-cache=true"]).expect("parse arguments"); + fn sends_cache_and_temperature_values_without_client_normalization() { + let args = Harness::try_parse_from(["test", "--temperature", "0.5", "--use-cache=true"]) + .expect("parse arguments"); let patch = args .config - .build_prompt_data_patch(Some("claude-test")) + .build_prompt_data_patch(Some("test-model")) .expect("prompt data"); - assert_eq!(patch["options"]["params"]["temperature"], 0.0); + assert_eq!(patch["options"]["params"]["temperature"], 0.5); assert_eq!(patch["options"]["params"]["use_cache"], true); } - #[test] - fn rejects_cache_with_nonzero_temperature() { - let args = Harness::try_parse_from(["test", "--temperature", "0.5", "--use-cache=true"]) - .expect("parse arguments"); - - let error = args - .config - .build_prompt_data_patch(Some("claude-test")) - .expect_err("nonzero temperature should conflict with caching"); - assert!(error - .to_string() - .contains("--use-cache=true requires --temperature=0")); - } - #[test] fn supports_response_format_shorthands() { assert_eq!( @@ -1099,403 +480,6 @@ mod tests { ); } - #[test] - fn rejects_model_parameters_outside_provider_ranges() { - for (arguments, expected) in [ - ( - vec!["--temperature", "99"], - "--temperature must be between 0 and 2", - ), - ( - vec!["--max-tokens", "0"], - "--max-tokens must be a positive integer", - ), - ( - vec!["--frequency-penalty", "-2.1"], - "--frequency-penalty must be between -2 and 2", - ), - ( - vec!["--presence-penalty", "2.1"], - "--presence-penalty must be between -2 and 2", - ), - ] { - let parsed = - Harness::try_parse_from(std::iter::once("test").chain(arguments.iter().copied())) - .expect("parse arguments"); - let error = parsed - .config - .build_prompt_data_patch(Some("gpt-4.1-mini")) - .expect_err("out-of-range parameter should fail"); - assert_eq!(error.to_string(), expected); - } - } - - #[test] - fn enforces_model_specific_temperature_support() { - let parsed = - Harness::try_parse_from(["test", "--temperature", "0.2"]).expect("parse arguments"); - - for model in ["gpt-5.4-nano", "o3", "claude-opus-4-7"] { - let error = parsed - .config - .build_prompt_data_patch(Some(model)) - .expect_err("unsupported temperature should fail"); - assert!(error.to_string().contains("not supported by model")); - } - } - - #[test] - fn allows_gpt5_temperature_when_reasoning_effort_is_none() { - let parsed = - Harness::try_parse_from(["test", "--temperature", "0.2", "--reasoning-effort", "none"]) - .expect("parse arguments"); - - let patch = parsed - .config - .build_prompt_data_patch(Some("gpt-5.4-nano")) - .expect("compatible parameters"); - assert_eq!(patch["options"]["params"]["temperature"], 0.2); - assert_eq!(patch["options"]["params"]["reasoning_effort"], "none"); - } - - #[test] - fn does_not_advise_reasoning_effort_none_when_the_model_rejects_it() { - for model in ["gpt-5", "gpt-5-mini", "gpt-5-pro"] { - let parsed = - Harness::try_parse_from(["test", "--temperature", "0.2"]).expect("parse arguments"); - let error = parsed - .config - .build_prompt_data_patch(Some(model)) - .expect_err("temperature is unsupported"); - let error = error.to_string(); - assert!(error.contains("not supported by model"), "{error}"); - assert!(!error.contains("--reasoning-effort none"), "{error}"); - } - } - - #[test] - fn validates_update_against_the_existing_model_and_params() { - let existing = json!({ - "options": { - "model": "gpt-5.4-nano", - "params": { "reasoning_effort": "medium" } - } - }); - let patch = json!({ - "prompt_data": { - "options": { "params": { "temperature": 0.2 } } - } - }); - let error = validate_patch(Some(&existing), &patch, None) - .expect_err("effective model does not support temperature"); - assert!(error.to_string().contains("--reasoning-effort none")); - - let existing = json!({ - "options": { - "model": "gpt-5.4-nano", - "params": { "reasoning_effort": "none" } - } - }); - validate_patch(Some(&existing), &patch, None) - .expect("existing reasoning effort makes temperature valid"); - } - - #[test] - fn applies_format_ranges_without_model_name_heuristics() { - let patch = json!({ - "prompt_data": { - "options": { - "model": "test-custom-model", - "params": { "temperature": 1.5 } - } - } - }); - let anthropic = model_spec("anthropic", false, false, None); - let error = validate_patch(None, &patch, Some(&anthropic)) - .expect_err("Anthropic temperature should use the smaller range"); - assert_eq!(error.to_string(), "--temperature must be between 0 and 1"); - - validate_patch(None, &patch, None) - .expect("unknown custom models should not be assigned a format by name"); - } - - #[test] - fn applies_web_ui_parameter_availability_and_model_token_limit() { - let window = model_spec("window", false, false, None); - let tool_patch = json!({ - "prompt_data": { - "options": { - "model": "test-window-model", - "params": { "tool_choice": "auto" } - } - } - }); - let error = validate_patch(None, &tool_patch, Some(&window)) - .expect_err("Window models do not expose tool choice in the UI"); - assert!(error.to_string().contains("--tool-choice is not supported")); - - let openai = model_spec("openai", false, false, Some(4096)); - let token_patch = json!({ - "prompt_data": { - "options": { - "model": "test-limited-model", - "params": { "max_tokens": 4097 } - } - } - }); - let error = validate_patch(None, &token_patch, Some(&openai)) - .expect_err("model output token limit should be enforced"); - assert!(error.to_string().contains("between 1 and 4096")); - } - - #[test] - fn applies_web_ui_reasoning_options() { - let reasoning = model_spec("openai", true, false, None); - let invalid = json!({ - "prompt_data": { - "options": { - "model": "o3", - "params": { "reasoning_effort": "minimal" } - } - } - }); - let error = validate_patch(None, &invalid, Some(&reasoning)) - .expect_err("generic reasoning models accept low, medium, or high"); - assert!(error.to_string().contains("low, medium, high")); - - let non_reasoning = model_spec("openai", false, false, None); - let non_reasoning_patch = json!({ - "prompt_data": { - "options": { - "model": "gpt-4.1", - "params": { "reasoning_effort": "low" } - } - } - }); - let error = validate_patch(None, &non_reasoning_patch, Some(&non_reasoning)) - .expect_err("non-reasoning models should reject reasoning effort"); - assert!(error.to_string().contains("not supported")); - } - - #[test] - fn validates_existing_parameters_when_switching_models() { - let existing = json!({ - "options": { - "model": "gpt-4.1", - "params": { "temperature": 0.5 } - } - }); - let patch = json!({ - "prompt_data": { - "options": { "model": "o3" } - } - }); - let error = validate_patch( - Some(&existing), - &patch, - Some(&model_spec("openai", true, false, None)), - ) - .expect_err("switching models must validate retained parameters"); - assert!(error.to_string().contains("--temperature is not supported")); - } - - #[test] - fn model_only_patch_is_prepared_for_capability_lookup() { - let patch = json!({ - "prompt_data": {"options": {"model": "test-model"}} - }); - let update = prepare_model_params_update(None, &patch) - .expect("prepare update") - .expect("model change requires lookup"); - assert_eq!(update.model.as_deref(), Some("test-model")); - assert!(update.changed_params.is_empty()); - } - - #[test] - fn validates_only_parameters_touched_by_an_update() { - let existing = json!({ - "options": { - "model": "test-model", - "params": { "temperature": 99 } - } - }); - validate_patch( - Some(&existing), - &json!({"prompt_data": {"options": {"params": {"top_p": 0.5}}}}), - Some(&model_spec("openai", false, false, None)), - ) - .expect("an unrelated stale parameter should not block an update"); - - validate_patch(Some(&existing), &json!({"description": "Updated"}), None) - .expect("an unrelated metadata update should remain possible"); - } - - #[test] - fn known_openai_models_use_the_backend_penalty_range() { - // The UI slider exposes 0..=1, but the backend and OpenAI API accept - // -2..=2. CLI validation should not reject a backend-valid value. - let openai = model_spec("openai", false, false, None); - for key in ["frequency_penalty", "presence_penalty"] { - let patch = json!({ - "prompt_data": { - "options": { - "model": "gpt-4.1-mini", - "params": { key: -0.5 } - } - } - }); - validate_patch(None, &patch, Some(&openai)).expect("backend-valid penalty"); - } - } - - #[test] - fn unknown_models_retain_the_wider_provider_penalty_range() { - // Without metadata, keep the provider range rather than reject a - // possibly valid value. - let patch = json!({ - "prompt_data": { - "options": { - "model": "test-custom-model", - "params": { "frequency_penalty": -0.5 } - } - } - }); - validate_patch(None, &patch, None).expect("unknown models keep the -2..2 range"); - } - - #[test] - fn verbosity_uses_display_name_then_model_id_like_the_ui() { - let openai = model_spec("openai", false, false, None); - let mut renamed = model_spec("openai", false, false, None); - renamed.display_name = Some("Fast internal judge".to_string()); - let mut labeled_gpt5 = model_spec("openai", false, false, None); - labeled_gpt5.display_name = Some("GPT-5 internal judge".to_string()); - - let patch = |model: &str| { - json!({ - "prompt_data": { - "options": { - "model": model, - "params": { "verbosity": "low" } - } - } - }) - }; - - let error = validate_patch(None, &patch("internal-gpt-5-deployment"), Some(&renamed)) - .expect_err("a non-GPT-5 display name overrides the model id in the UI"); - assert!(error.to_string().contains("--verbosity is not supported")); - - validate_patch(None, &patch("internal-deployment"), Some(&labeled_gpt5)) - .expect("a GPT-5 display name should allow verbosity"); - let error = validate_patch(None, &patch("gpt-4.1-mini"), Some(&openai)) - .expect_err("non-gpt-5 models should reject verbosity"); - assert!(error.to_string().contains("--verbosity is not supported")); - } - - #[test] - fn google_models_get_the_parameter_names_the_prompt_editor_reads() { - let mut patch = json!({ - "prompt_data": { - "options": { - "model": "gemini-2.5-pro", - "params": { "max_tokens": 1000, "top_p": 0.9, "temperature": 0.2 } - } - } - }); - apply_provider_param_names( - &mut patch, - Some(&model_spec("google", true, true, Some(65535))), - ); - - let params = &patch["prompt_data"]["options"]["params"]; - assert_eq!(params["maxOutputTokens"], 1000); - assert_eq!(params["topP"], 0.9); - // Temperature keeps its name in every format. - assert_eq!(params["temperature"], 0.2); - assert!(params.get("max_tokens").is_none()); - assert!(params.get("top_p").is_none()); - } - - #[test] - fn other_formats_keep_canonical_parameter_names() { - let original = json!({ - "prompt_data": { - "options": { - "model": "gpt-4.1-mini", - "params": { "max_tokens": 1000, "top_p": 0.9 } - } - } - }); - - for spec in [ - Some(model_spec("openai", false, false, None)), - Some(model_spec("anthropic", false, false, None)), - Some(model_spec("converse", false, false, None)), - // An unresolved model: the format is unknown, so do not guess. - None, - ] { - let mut patch = original.clone(); - apply_provider_param_names(&mut patch, spec.as_ref()); - assert_eq!(patch, original); - } - } - - #[test] - fn renaming_tolerates_a_patch_without_params() { - let mut patch = json!({ "description": "no prompt data at all" }); - let original = patch.clone(); - apply_provider_param_names(&mut patch, Some(&model_spec("google", false, false, None))); - assert_eq!(patch, original); - } - - #[test] - fn validates_and_translates_top_k() { - let parsed = Harness::try_parse_from(["test", "--top-k", "42"]).expect("parse top-k"); - let mut patch = Value::Object( - parsed - .config - .build_prompt_data_patch(Some("gemini-test")) - .expect("prompt data"), - ); - let google = model_spec("google", false, false, None); - validate_patch(None, &json!({"prompt_data": patch.clone()}), Some(&google)) - .expect("valid Google top-k"); - - // The validator receives a full function patch in production. - patch = json!({"prompt_data": patch}); - apply_provider_param_names(&mut patch, Some(&google)); - assert_eq!(patch["prompt_data"]["options"]["params"]["topK"], 42); - } - - #[test] - fn supports_reasoning_budget_models() { - let parsed = - Harness::try_parse_from(["test", "--reasoning-enabled", "--reasoning-budget", "2048"]) - .expect("parse reasoning budget"); - let prompt_data = parsed - .config - .build_prompt_data_patch(Some("test-budget-model")) - .expect("prompt data"); - let patch = json!({"prompt_data": prompt_data}); - let budget_model = model_spec("anthropic", true, true, None); - validate_patch(None, &patch, Some(&budget_model)).expect("budget model parameters"); - - let effort_model = model_spec("openai", true, false, None); - let error = validate_patch(None, &patch, Some(&effort_model)) - .expect_err("effort-based model should reject budget parameters"); - assert!(error.to_string().contains("not supported")); - } - - #[test] - fn matches_backend_databricks_gpt5_reasoning_options() { - assert!(is_gpt_5_1_or_later("databricks-gpt-5-2")); - assert_eq!( - reasoning_effort_options("databricks-gpt-5-2", false), - &["none", "low", "medium", "high"] - ); - } - #[test] fn rejects_invalid_json_schema_response_format() { let error = parse_response_format_source(r#"{"type":"json_schema"}"#) diff --git a/src/utils/cache.rs b/src/utils/cache.rs deleted file mode 100644 index b1bb3522..00000000 --- a/src/utils/cache.rs +++ /dev/null @@ -1,14 +0,0 @@ -use std::path::PathBuf; - -/// Root directory for `bt`'s on-disk caches. -/// -/// Path discovery, not configuration: standard `XDG_CACHE_HOME`/`HOME` only, no -/// bt-specific variable. Falls back to the temp directory. -pub(crate) fn bt_cache_root() -> PathBuf { - let root = std::env::var_os("XDG_CACHE_HOME") - .map(PathBuf::from) - .or_else(|| std::env::var_os("HOME").map(|home| PathBuf::from(home).join(".cache"))) - .unwrap_or_else(std::env::temp_dir); - - root.join("bt") -} diff --git a/src/utils/mod.rs b/src/utils/mod.rs index 922e10a1..1429bebe 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -1,5 +1,4 @@ mod app_url; -mod cache; mod duration; mod fs_atomic; mod git; @@ -11,7 +10,6 @@ mod structured_source; mod text_source; pub(crate) use app_url::{app_project_url, app_project_url_with_encoded_path}; -pub(crate) use cache::bt_cache_root; pub use duration::parse_duration_to_seconds; pub use fs_atomic::{ write_bytes_atomic, write_json_atomic, write_json_atomic_private, write_text_atomic, From 45191f33dcd544dc2e4f565a22d692aa06366e0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Halber?= Date: Thu, 13 Aug 2026 18:36:16 -0700 Subject: [PATCH 10/12] feat(functions): update functions tools and scorers --- README.md | 15 +- src/functions/api.rs | 14 + src/functions/mod.rs | 47 ++- src/functions/prompt_patch.rs | 53 +++ src/functions/update.rs | 734 ++++++++++++++++++++++++++++++++++ src/scorers.rs | 60 ++- tests/cli.rs | 15 + 7 files changed, 916 insertions(+), 22 deletions(-) create mode 100644 src/functions/prompt_patch.rs create mode 100644 src/functions/update.rs diff --git a/README.md b/README.md index d7382d6e..d28feff8 100644 --- a/README.md +++ b/README.md @@ -149,7 +149,9 @@ Remove-Item -Recurse -Force (Join-Path $env:APPDATA "bt") -ErrorAction SilentlyC | `bt projects` | Manage projects (list, create, view, delete) | | `bt datasets` | Manage remote datasets (list, create, update, view, delete) | | `bt prompts` | Manage prompts (list, view, delete) | -| `bt scorers` | Manage scorers (list, create, view, invoke, delete) | +| `bt functions` | Manage functions (list, view, invoke, update, push, pull, delete) | +| `bt tools` | Manage tools (list, view, invoke, update, delete) | +| `bt scorers` | Manage scorers (list, create, view, invoke, update, delete) | | `bt sync` | Synchronize project logs between Braintrust and local NDJSON files | | `bt update` | Update bt in-place | @@ -174,6 +176,17 @@ Use `--if-exists error|ignore|replace` to control slug conflicts. Text and struc Before writing a scorer, `bt` sends the complete candidate definition to Braintrust for validation. The backend applies the same model-parameter and replacement checks as the write and returns structured issues with normalization suggestions when available. +Update only the fields you specify, or use `--patch` for fields without dedicated flags: + +```bash +bt scorers update helpfulness --messages @messages.json +bt scorers update helpfulness --model gpt-5.4-nano +bt functions update my-function --description "Updated" +bt tools update my-tool --patch @tool-patch.json +``` + +The API replaces `prompt_data` rather than merging it, so `bt` reads the current definition and sends a materialized replacement with your changes. A concurrent edit can therefore be overwritten. + For TypeScript and Python code scorers, use the Braintrust SDK and `bt functions push`. ## `bt eval` diff --git a/src/functions/api.rs b/src/functions/api.rs index 6b62aa10..f3806b40 100644 --- a/src/functions/api.rs +++ b/src/functions/api.rs @@ -239,6 +239,20 @@ pub async fn delete_function(client: &ApiClient, function_id: &str) -> Result<() client.delete(&path).await } +/// Partially update a function (scorer/tool/prompt/...) by id. +/// +/// Top-level fields are patched, but object-valued fields such as `prompt_data` +/// are replaced wholesale. Callers updating `prompt_data` must materialize the +/// complete value before sending the request. +pub async fn patch_function( + client: &ApiClient, + function_id: &str, + body: &serde_json::Value, +) -> Result { + let path = format!("/v1/function/{}", encode(function_id)); + client.patch(&path, body).await +} + pub async fn list_functions_page( client: &ApiClient, query: &FunctionListQuery, diff --git a/src/functions/mod.rs b/src/functions/mod.rs index 03c5301a..aa445272 100644 --- a/src/functions/mod.rs +++ b/src/functions/mod.rs @@ -18,9 +18,11 @@ mod delete; mod invoke; mod list; pub(crate) mod prompt_config; +pub(crate) mod prompt_patch; mod pull; mod push; pub(crate) mod report; +mod update; mod view; use api::Function; @@ -173,8 +175,7 @@ Examples: bt tools view my-tool bt tools view fn_123 bt tools view --id fn_123 - bt scorers list - bt scorers delete my-scorer + bt tools update my-tool --patch @tool-patch.json ")] pub struct FunctionArgs { #[command(subcommand)] @@ -191,6 +192,8 @@ pub(crate) enum FunctionCommands { Delete(DeleteArgs), /// Invoke by slug Invoke(invoke::InvokeArgs), + /// Update a function in place (prompt configuration, metadata, or arbitrary patch) + Update(Box), } #[derive(Debug, Clone, Args)] @@ -223,6 +226,8 @@ enum FunctionsCommands { Delete(FunctionsDeleteArgs), /// Invoke a function Invoke(FunctionsInvokeArgs), + /// Update a function in place (prompt configuration, metadata, or arbitrary patch) + Update(Box), /// Push local function definitions Push(PushArgs), /// Pull remote function definitions @@ -272,6 +277,15 @@ struct FunctionsInvokeArgs { function_type: Option, } +#[derive(Debug, Clone, Args)] +struct FunctionsUpdateArgs { + #[command(flatten)] + inner: update::UpdateArgs, + /// Filter by function type (for interactive selection) + #[arg(long = "type", short = 't', value_enum)] + function_type: Option, +} + #[derive(Debug, Clone, Args)] pub(crate) struct PushArgs { /// File or directory path(s) to scan for function definitions. @@ -657,6 +671,7 @@ pub(crate) async fn run_typed_command( None | Some(FunctionCommands::List) => list::run(&ctx, base.json, ft).await, Some(FunctionCommands::Delete(d)) => delete::run(&ctx, d.slug(), d.force, ft).await, Some(FunctionCommands::Invoke(i)) => invoke::run(&ctx, &i, base.json, ft).await, + Some(FunctionCommands::Update(u)) => update::run(&ctx, &u, base.json, ft).await, Some(FunctionCommands::View(_)) => { unreachable!("handled before context resolution") } @@ -720,6 +735,9 @@ pub async fn run(base: BaseArgs, args: FunctionsArgs) -> Result<()> { Some(FunctionsCommands::Invoke(i)) => { invoke::run(&ctx, &i.inner, base.json, i.function_type.or(function_type)).await } + Some(FunctionsCommands::Update(u)) => { + update::run(&ctx, &u.inner, base.json, u.function_type.or(function_type)).await + } Some(FunctionsCommands::Push(_)) | Some(FunctionsCommands::Pull(_)) | Some(FunctionsCommands::View(_)) => { @@ -1201,6 +1219,31 @@ mod tests { ) } + #[test] + fn typed_function_update_command_is_not_read_only() { + let _guard = test_lock(); + let parsed = FunctionArgsHarness::try_parse_from(["bt-tools", "update", "my-tool", "-y"]) + .expect("parse"); + assert!(!function_command_is_read_only(parsed.args.command.as_ref())); + } + + #[test] + fn functions_update_command_is_not_read_only() { + let _guard = test_lock(); + let parsed = FunctionsArgsHarness::try_parse_from([ + "bt-functions", + "update", + "my-fn", + "--description", + "x", + "-y", + ]) + .expect("parse"); + assert!(!functions_command_is_read_only( + parsed.args.command.as_ref() + )); + } + #[test] fn typed_function_commands_map_to_expected_auth_mode() { let _guard = test_lock(); diff --git a/src/functions/prompt_patch.rs b/src/functions/prompt_patch.rs new file mode 100644 index 00000000..3a3ba7af --- /dev/null +++ b/src/functions/prompt_patch.rs @@ -0,0 +1,53 @@ +use serde_json::Value; + +use crate::utils::merge_json_objects; + +/// Replace a partial `prompt_data` patch with the full merged object. +/// +/// The function and prompt PATCH endpoints merge top-level fields but replace +/// `prompt_data` wholesale. Materializing it before the request preserves fields +/// that the user did not change. +pub(crate) fn materialize_prompt_data_patch( + patch: &mut Value, + existing_prompt_data: Option<&Value>, +) { + let Some(patch_prompt_data) = patch.get("prompt_data").and_then(Value::as_object).cloned() + else { + return; + }; + let mut merged = existing_prompt_data + .and_then(Value::as_object) + .cloned() + .unwrap_or_default(); + merge_json_objects(&mut merged, &patch_prompt_data); + patch["prompt_data"] = Value::Object(merged); +} + +#[cfg(test)] +mod tests { + use serde_json::json; + + use super::*; + + #[test] + fn materializes_complete_prompt_data_for_patch() { + let existing = json!({ + "prompt": {"type": "chat", "messages": []}, + "parser": {"type": "llm_classifier"}, + "options": {"model": "test-model", "params": {"temperature": 0.5}} + }); + let mut patch = json!({ + "prompt_data": {"options": {"params": {"temperature": 0.2}}} + }); + + materialize_prompt_data_patch(&mut patch, Some(&existing)); + + assert_eq!(patch["prompt_data"]["prompt"], existing["prompt"]); + assert_eq!(patch["prompt_data"]["parser"], existing["parser"]); + assert_eq!(patch["prompt_data"]["options"]["model"], "test-model"); + assert_eq!( + patch["prompt_data"]["options"]["params"]["temperature"], + 0.2 + ); + } +} diff --git a/src/functions/update.rs b/src/functions/update.rs new file mode 100644 index 00000000..7be92192 --- /dev/null +++ b/src/functions/update.rs @@ -0,0 +1,734 @@ +use anyhow::{anyhow, bail, Context, Result}; +use clap::{builder::BoolishValueParser, Args}; +use dialoguer::Confirm; +use serde_json::{json, Map, Value}; + +use crate::{ + ui::{is_interactive, print_command_status, with_spinner, CommandStatus}, + utils::{merge_json_objects, read_text_source, read_yaml_object_source}, +}; + +use super::{api, label, label_plural, select_function_interactive}; +use super::{ + prompt_config::{ + parse_choice_scores_source, parse_classifications_source, validate_prompt_data_patch, + validate_unit_interval, PromptConfigArgs, + }, + prompt_patch::materialize_prompt_data_patch, + FunctionTypeFilter, ResolvedContext, +}; + +/// Update a function's prompt configuration or metadata in place. +/// +/// This wraps `PATCH /v1/function/{id}`. The endpoint replaces `prompt_data` +/// wholesale, so the command reads the current definition and materializes a +/// complete replacement while changing only the fields requested by the user. +#[derive(Debug, Clone, Args)] +#[command(after_help = "\ +Examples: + bt scorers update my-scorer --messages @messages.json + bt scorers update my-scorer --model gpt-5.4-nano --reasoning-effort none --temperature 0.1 + bt scorers update my-scorer --template-format jinja --pass-threshold 0.7 + bt scorers update my-scorer --classifications '[\"safe\",\"unsafe\"]' + bt scorers update my-scorer --metadata @metadata.yaml + bt scorers update my-scorer --description \"Helpfulness judge\" + bt scorers update my-scorer --patch '{\"prompt_data\":{\"options\":{\"model\":\"gpt-5.4-nano\"}}}' + bt scorers update --id fn_123 --patch @scorer-patch.json + bt tools update my-tool --patch @tool-patch.json +")] +pub struct UpdateArgs { + #[command(flatten)] + slug: super::SlugArgs, + + /// Function id (alternative to slug). Auto-detected for `fn_`/`func_` prefixes. + #[arg(long = "id")] + id: Option, + + /// Replacement chat messages source: inline JSON, @PATH to read from a + /// file, or - for stdin. + #[arg(long, value_name = "SOURCE")] + messages: Option, + + /// Update the model used by an LLM scorer/prompt. + #[arg(long, short = 'm', value_name = "MODEL")] + model: Option, + + #[command(flatten)] + prompt_config: PromptConfigArgs, + + /// Replace choice-to-score mappings for score output. Accepts inline JSON, + /// @PATH to read from a file, or - for stdin. + #[arg(long, value_name = "SOURCE", conflicts_with = "classifications")] + choice_scores: Option, + + /// Replace labels for classification output. Accepts an inline JSON array, + /// @PATH to read from a file, or - for stdin. + #[arg(long, value_name = "SOURCE", conflicts_with = "choice_scores")] + classifications: Option, + + /// Update chain-of-thought reasoning. Pass --use-cot=false to disable it. + #[arg( + long, + num_args = 0..=1, + default_missing_value = "true", + value_parser = BoolishValueParser::new() + )] + use_cot: Option, + + /// Update whether a classifier may return no matching classification. + #[arg( + long, + num_args = 0..=1, + default_missing_value = "true", + value_parser = BoolishValueParser::new() + )] + allow_no_match: Option, + + /// Update the score threshold for passing, between 0 and 1. + #[arg(long, value_name = "NUMBER", conflicts_with = "classifications")] + pass_threshold: Option, + + /// Deep-merge metadata from inline YAML, @PATH, or stdin (-). + #[arg(long, value_name = "SOURCE")] + metadata: Option, + + /// Update the function description. + #[arg(long, short = 'd', value_name = "TEXT")] + description: Option, + + /// Arbitrary JSON object deep-merged into the function. Accepts inline + /// JSON, @PATH to read JSON from a file, or - for stdin. + #[arg(long, value_name = "SOURCE")] + patch: Option, + + /// Skip the confirmation prompt. + #[arg(long, short = 'y')] + yes: bool, +} + +impl UpdateArgs { + /// Flags that only make sense for LLM scorers and classifiers. + /// + /// Returns the flag names that were set so callers can reject them on other + /// function kinds (for example tools) with an actionable message. + fn scorer_output_flags(&self) -> Vec<&'static str> { + let mut flags = Vec::new(); + if self.choice_scores.is_some() { + flags.push("--choice-scores"); + } + if self.classifications.is_some() { + flags.push("--classifications"); + } + if self.allow_no_match.is_some() { + flags.push("--allow-no-match"); + } + if self.use_cot.is_some() { + flags.push("--use-cot"); + } + if self.pass_threshold.is_some() { + flags.push("--pass-threshold"); + } + flags + } + + fn selector(&self) -> Result> { + match ( + self.id.as_deref(), + self.slug.slug_positional(), + self.slug.slug_flag(), + ) { + (Some(_), Some(_), _) | (Some(_), _, Some(_)) => { + bail!("use either --id or a slug, not both") + } + (Some(id), None, None) => Ok(UpdateSelector::Id(id)), + (None, Some(positional), None) if super::is_likely_function_id(positional) => { + Ok(UpdateSelector::Id(positional)) + } + (None, positional, flag) => Ok(UpdateSelector::Slug(positional.or(flag))), + } + } +} + +#[derive(Debug)] +enum UpdateSelector<'a> { + Id(&'a str), + Slug(Option<&'a str>), +} + +pub async fn run( + ctx: &ResolvedContext, + args: &UpdateArgs, + json_output: bool, + ft: Option, +) -> Result<()> { + let mut body = build_patch_body(args)?; + + let function = resolve_target_function(ctx, args, ft).await?; + + // LLM scorer/classifier output flags only apply to prompt-based scorers and + // classifiers. Reject them on other function kinds (for example tools) so an + // unrelated function is not silently patched with a parser it cannot use. + let is_scorer_like = matches!( + function.function_type.as_deref(), + Some("scorer") | Some("classifier") + ); + let scorer_flags = args.scorer_output_flags(); + if !scorer_flags.is_empty() && !is_scorer_like { + bail!( + "{} apply to LLM scorers and classifiers, not {} '{}'. \ + Run `bt scorers update` on a scorer instead.", + scorer_flags.join(", "), + label(ft), + function.name, + ); + } + + // Mirrors `create`, where --allow-no-match requires --classifications: a + // score parser would never consult it. + let produces_classifications = + args.classifications.is_some() || function.function_type.as_deref() == Some("classifier"); + if args.allow_no_match.is_some() && !produces_classifications { + bail!( + "--allow-no-match applies to classification output, but '{}' produces scores. \ + Pass --classifications to switch it to labels.", + function.name, + ); + } + + // Last of the up-front checks because it may hit the network. + with_spinner( + "Validating model parameters...", + validate_prompt_data_patch( + ctx, + function.prompt_data.as_ref(), + &mut body, + args.prompt_config.refresh_models(), + ), + ) + .await? + .warn_if_incomplete(); + materialize_prompt_data_patch(&mut body, function.prompt_data.as_ref()); + + // Switching output mode updates function_type, but materialization merges + // the parser and does not drop the previous mode's keys. Warn so the + // user can review or recreate for a clean switch. + if !crate::ui::is_quiet() { + match function.function_type.as_deref() { + Some("classifier") if args.choice_scores.is_some() => print_command_status( + CommandStatus::Warning, + "Switching to score output; previous classification labels may remain in the definition. Review with `bt scorers view`.", + ), + Some("scorer") if args.classifications.is_some() => print_command_status( + CommandStatus::Warning, + "Switching to classification output; previous choice scores may remain in the definition. Review with `bt scorers view`.", + ), + _ => {} + } + } + + if !args.yes && is_interactive() { + let confirm = Confirm::new() + .with_prompt(format!( + "Update {} '{}' in {}?", + label(ft), + function.name, + ctx.project.name + )) + .default(false) + .interact()?; + if !confirm { + return Ok(()); + } + } + + let updated = match with_spinner( + &format!("Updating {}...", label(ft)), + api::patch_function(&ctx.client, &function.id, &body), + ) + .await + { + Ok(value) => { + print_command_status( + CommandStatus::Success, + &format!("Updated '{}'", function.name), + ); + value + } + Err(error) => { + print_command_status( + CommandStatus::Error, + &format!("Failed to update '{}'", function.name), + ); + return Err(error); + } + }; + + if json_output { + println!("{}", serde_json::to_string(&updated)?); + } else if !crate::ui::is_quiet() { + eprintln!( + "Run `bt {} view {}` to inspect the updated definition.", + label_plural(ft), + function.slug + ); + } + + Ok(()) +} + +async fn resolve_target_function( + ctx: &ResolvedContext, + args: &UpdateArgs, + ft: Option, +) -> Result { + let project_id = &ctx.project.id; + match args.selector()? { + UpdateSelector::Id(id) => api::get_function_by_id(&ctx.client, id, None) + .await? + .ok_or_else(|| anyhow!("{} with id '{id}' not found", label(ft))), + UpdateSelector::Slug(Some(slug)) => { + api::get_function_by_slug(&ctx.client, project_id, slug, None) + .await? + .ok_or_else(|| anyhow!("{} with slug '{slug}' not found", label(ft))) + } + UpdateSelector::Slug(None) => { + if !is_interactive() { + bail!( + "{} slug or --id required. Use: bt {} update [--patch ...]", + label(ft), + label_plural(ft), + ); + } + Ok(select_function_interactive(&ctx.client, project_id, ft).await?) + } + } +} + +fn build_patch_body(args: &UpdateArgs) -> Result { + let mut patch: Map = Map::new(); + + if let Some(description) = args.description.as_deref() { + patch.insert( + "description".to_string(), + Value::String(description.to_string()), + ); + } + + let metadata = resolve_metadata(args)?; + if !metadata.is_empty() { + patch.insert("metadata".to_string(), Value::Object(metadata)); + } + + if let Some(messages) = resolve_messages(args)? { + let prompt_data_patch = json!({ + "prompt_data": { + "prompt": { "type": "chat", "messages": messages }, + }, + }); + merge_json_objects( + &mut patch, + prompt_data_patch + .as_object() + .expect("prompt data patch is an object"), + ); + } + + let parser_patch = resolve_parser_patch(args)?; + if let Some((function_type, parser)) = parser_patch { + if let Some(function_type) = function_type { + patch.insert( + "function_type".to_string(), + Value::String(function_type.to_string()), + ); + } + let prompt_data_patch = json!({ "prompt_data": { "parser": parser } }); + merge_json_objects( + &mut patch, + prompt_data_patch + .as_object() + .expect("prompt data patch is an object"), + ); + } + + let prompt_config = args + .prompt_config + .build_prompt_data_patch(args.model.as_deref())?; + if !prompt_config.is_empty() { + let prompt_data_patch = json!({ "prompt_data": prompt_config }); + merge_json_objects( + &mut patch, + prompt_data_patch + .as_object() + .expect("prompt data patch is an object"), + ); + } + + let extra = resolve_extra_patch(args)?; + if let Some(extra_obj) = extra { + merge_json_objects(&mut patch, &extra_obj); + } + + if patch.is_empty() { + bail!("no updates requested. Pass an update flag; see `bt scorers update --help`"); + } + + Ok(Value::Object(patch)) +} + +fn resolve_metadata(args: &UpdateArgs) -> Result> { + if args.classifications.is_some() && args.pass_threshold.is_some() { + bail!("--pass-threshold applies to score output and cannot be used with --classifications"); + } + + let mut metadata = match args.metadata.as_deref() { + Some(source) => read_yaml_object_source(source, "function metadata")?, + None => Map::new(), + }; + if let Some(pass_threshold) = args.pass_threshold { + validate_unit_interval(pass_threshold, "--pass-threshold")?; + metadata.insert("__pass_threshold".to_string(), json!(pass_threshold)); + } + Ok(metadata) +} + +fn resolve_parser_patch(args: &UpdateArgs) -> Result, Value)>> { + if args.choice_scores.is_some() && args.allow_no_match.is_some() { + bail!("--allow-no-match applies to classification output, not --choice-scores"); + } + + let mut parser = Map::new(); + let mut function_type = None; + + if let Some(source) = args.choice_scores.as_deref() { + parser.insert( + "type".to_string(), + Value::String("llm_classifier".to_string()), + ); + parser.insert( + "choice_scores".to_string(), + Value::Object(parse_choice_scores_source(source)?), + ); + function_type = Some("scorer"); + } + if let Some(source) = args.classifications.as_deref() { + parser.insert( + "type".to_string(), + Value::String("llm_classifier".to_string()), + ); + parser.insert( + "choice".to_string(), + Value::Array(parse_classifications_source(source)?), + ); + function_type = Some("classifier"); + } + if let Some(use_cot) = args.use_cot { + parser.insert("use_cot".to_string(), Value::Bool(use_cot)); + } + if let Some(allow_no_match) = args.allow_no_match { + parser.insert("allow_no_match".to_string(), Value::Bool(allow_no_match)); + } + + if parser.is_empty() { + Ok(None) + } else { + Ok(Some((function_type, Value::Object(parser)))) + } +} + +fn resolve_messages(args: &UpdateArgs) -> Result> { + match args.messages.as_deref() { + Some(source) => { + let raw = read_text_source(source, "messages")?; + let parsed: Value = serde_json::from_str(&raw).context("invalid JSON in --messages")?; + match parsed { + Value::Array(_) => Ok(Some(parsed)), + _ => bail!("--messages must be a JSON array of chat messages"), + } + } + None => Ok(None), + } +} + +fn resolve_extra_patch(args: &UpdateArgs) -> Result>> { + let Some(source) = args.patch.as_deref() else { + return Ok(None); + }; + let raw = read_text_source(source, "patch")?; + parse_patch_object(&raw).map(Some) +} + +fn parse_patch_object(raw: &str) -> Result> { + let value: Value = serde_json::from_str(raw).context("invalid JSON in --patch")?; + match value { + Value::Object(map) => Ok(map), + _ => bail!("--patch must be a JSON object"), + } +} + +#[cfg(test)] +mod tests { + use clap::Parser; + + use super::*; + + #[derive(Debug, Parser)] + struct UpdateArgsHarness { + #[command(flatten)] + args: UpdateArgs, + } + + fn args(model: Option<&str>, description: Option<&str>) -> UpdateArgs { + UpdateArgs { + slug: super::super::SlugArgs { + slug_positional: Some("test-slug".to_string()), + slug_flag: None, + }, + id: None, + messages: None, + model: model.map(ToOwned::to_owned), + prompt_config: PromptConfigArgs::default(), + choice_scores: None, + classifications: None, + use_cot: None, + allow_no_match: None, + pass_threshold: None, + metadata: None, + description: description.map(ToOwned::to_owned), + patch: None, + yes: true, + } + } + + #[test] + fn scorer_output_flags_reported_only_when_set() { + let base = args(None, None); + assert!(base.scorer_output_flags().is_empty()); + + let mut scored = args(None, None); + scored.choice_scores = Some(r#"{"pass":1}"#.to_string()); + scored.pass_threshold = Some(0.5); + assert_eq!( + scored.scorer_output_flags(), + vec!["--choice-scores", "--pass-threshold"] + ); + + let mut labeled = args(None, None); + labeled.classifications = Some(r#"["a"]"#.to_string()); + labeled.allow_no_match = Some(true); + labeled.use_cot = Some(false); + assert_eq!( + labeled.scorer_output_flags(), + vec!["--classifications", "--allow-no-match", "--use-cot"] + ); + } + + #[test] + fn build_patch_body_messages_writes_chat_block() { + let mut args = args(None, None); + args.messages = Some(r#"[{"role":"user","content":"hi"}]"#.to_string()); + let body = build_patch_body(&args).expect("patch body"); + assert_eq!( + body["prompt_data"]["prompt"]["type"], + serde_json::json!("chat") + ); + assert_eq!( + body["prompt_data"]["prompt"]["messages"], + serde_json::json!([{"role":"user","content":"hi"}]) + ); + } + + #[test] + fn build_patch_body_model_merges_into_prompt_data() { + let args = args(Some("gpt-4o-mini"), None); + let body = build_patch_body(&args).expect("patch body"); + assert_eq!( + body["prompt_data"]["options"]["model"], + serde_json::json!("gpt-4o-mini") + ); + } + + #[test] + fn build_patch_body_messages_and_model_combine() { + let mut args = args(Some("gpt-4o-mini"), None); + args.messages = Some(r#"[{"role":"user","content":"Grade it."}]"#.to_string()); + let body = build_patch_body(&args).expect("patch body"); + assert_eq!( + body["prompt_data"]["prompt"]["messages"], + json!([{"role": "user", "content": "Grade it."}]) + ); + assert_eq!( + body["prompt_data"]["options"]["model"], + serde_json::json!("gpt-4o-mini") + ); + } + + #[test] + fn build_patch_body_updates_all_llm_configuration() { + let parsed = UpdateArgsHarness::try_parse_from([ + "test", + "test-scorer", + "--model", + "gpt-test", + "--temperature", + "0.2", + "--max-tokens", + "128", + "--top-p", + "0.9", + "--frequency-penalty", + "0.5", + "--presence-penalty", + "0.25", + "--stop-sequence", + "END", + "--tool-choice", + "test_tool", + "--reasoning-effort", + "low", + "--verbosity", + "high", + "--template-format", + "none", + "--use-cot=false", + ]) + .expect("parse update"); + + let body = build_patch_body(&parsed.args).expect("patch body"); + let params = &body["prompt_data"]["options"]["params"]; + assert_eq!(body["prompt_data"]["options"]["model"], "gpt-test"); + assert_eq!(params["temperature"], 0.2); + assert_eq!(params["max_tokens"], 128); + assert_eq!(params["top_p"], 0.9); + assert_eq!(params["frequency_penalty"], 0.5); + assert_eq!(params["presence_penalty"], 0.25); + assert_eq!(params["stop"], json!(["END"])); + assert_eq!( + params["tool_choice"], + json!({"type": "function", "function": {"name": "test_tool"}}) + ); + assert_eq!(params["reasoning_effort"], "low"); + assert_eq!(params["verbosity"], "high"); + assert_eq!(body["prompt_data"]["template_format"], "none"); + assert_eq!(body["prompt_data"]["parser"]["use_cot"], false); + } + + #[test] + fn build_patch_body_switches_to_classification_output() { + let mut args = args(None, None); + args.classifications = Some(r#"["safe","unsafe"]"#.to_string()); + args.allow_no_match = Some(true); + args.metadata = Some("owner: test-team".to_string()); + + let body = build_patch_body(&args).expect("patch body"); + assert_eq!(body["function_type"], "classifier"); + assert_eq!( + body["prompt_data"]["parser"]["choice"], + json!(["safe", "unsafe"]) + ); + assert_eq!(body["prompt_data"]["parser"]["allow_no_match"], true); + assert_eq!(body["metadata"]["owner"], "test-team"); + } + + #[test] + fn build_patch_body_updates_scores_and_pass_threshold() { + let mut args = args(None, None); + args.choice_scores = Some(r#"{"pass":1,"fail":0}"#.to_string()); + args.pass_threshold = Some(0.8); + + let body = build_patch_body(&args).expect("patch body"); + assert_eq!(body["function_type"], "scorer"); + assert_eq!( + body["prompt_data"]["parser"]["choice_scores"], + json!({"pass": 1, "fail": 0}) + ); + assert_eq!(body["metadata"]["__pass_threshold"], 0.8); + } + + #[test] + fn build_patch_body_description_is_top_level() { + let args = args(None, Some("Helpfulness judge")); + let body = build_patch_body(&args).expect("patch body"); + assert_eq!(body["description"], serde_json::json!("Helpfulness judge")); + } + + #[test] + fn build_patch_body_reads_at_prefixed_messages_file() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("messages.json"); + std::fs::write(&path, r#"[{"role":"user","content":"Grade from a file."}]"#) + .expect("write messages"); + let source = format!("@{}", path.display()); + + let mut args = args(None, None); + args.messages = Some(source); + let body = build_patch_body(&args).expect("patch body"); + assert_eq!( + body["prompt_data"]["prompt"]["messages"], + json!([{"role": "user", "content": "Grade from a file."}]) + ); + } + + #[test] + fn build_patch_body_reads_at_prefixed_patch_file() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("patch.json"); + std::fs::write(&path, r#"{"description":"From a file"}"#).expect("write patch"); + let source = format!("@{}", path.display()); + + let mut args = args(None, None); + args.patch = Some(source); + let body = build_patch_body(&args).expect("patch body"); + assert_eq!(body["description"], "From a file"); + } + + #[test] + fn build_patch_body_rejects_empty_update() { + let args = args(None, None); + let err = build_patch_body(&args).expect_err("should reject empty"); + assert!(err.to_string().contains("no updates requested")); + } + + #[test] + fn build_patch_body_extra_patch_merges_into_prompt_data() { + let mut args = args(None, None); + args.patch = Some(r#"{"prompt_data":{"parser":{"type":"llm_classifier","use_cot":true,"choice_scores":{"A":1.0,"B":0.0}}}}"#.to_string()); + let body = build_patch_body(&args).expect("patch body"); + assert_eq!( + body["prompt_data"]["parser"]["choice_scores"], + serde_json::json!({"A": 1.0, "B": 0.0}) + ); + } + + #[test] + fn parse_patch_object_rejects_non_object() { + let err = parse_patch_object("[1,2,3]").expect_err("should reject"); + assert!(err.to_string().contains("JSON object")); + } + + #[test] + fn merge_objects_deep_merges_nested_maps() { + let mut target = serde_json::json!({ + "prompt_data": { "options": { "model": "gpt-4o" } } + }) + .as_object() + .expect("object") + .clone(); + let source = serde_json::json!({ + "prompt_data": { "options": { "temperature": 0 } } + }) + .as_object() + .expect("object") + .clone(); + + merge_json_objects(&mut target, &source); + + assert_eq!( + target["prompt_data"]["options"]["model"], + serde_json::json!("gpt-4o") + ); + assert_eq!( + target["prompt_data"]["options"]["temperature"], + serde_json::json!(0) + ); + } +} diff --git a/src/scorers.rs b/src/scorers.rs index d2485a6e..e9b6a167 100644 --- a/src/scorers.rs +++ b/src/scorers.rs @@ -11,6 +11,7 @@ Examples: bt scorers view my-scorer bt scorers create \"Helpfulness\" --model gpt-5.4-nano --messages @messages.json \\ --choice-scores '{\"A\":1,\"B\":0}' + bt scorers update my-scorer --messages @messages.json bt scorers delete my-scorer TypeScript and Python code scorers: @@ -47,7 +48,6 @@ mod tests { use clap::Parser; use super::*; - use crate::args::CLIArgs; #[derive(Debug, Parser)] struct ScorersArgsHarness { @@ -55,24 +55,6 @@ mod tests { args: ScorersArgs, } - #[test] - fn invoke_accepts_global_json_flag() { - #[derive(Debug, Parser)] - struct Harness { - #[command(flatten)] - command: CLIArgs, - } - - let parsed = Harness::try_parse_from(["bt-scorers", "invoke", "test-scorer", "--json"]) - .expect("parse scorer invoke with global JSON output"); - - assert!(parsed.command.base.json); - assert!(matches!( - parsed.command.args.command, - Some(ScorersCommands::Function(FunctionCommands::Invoke(_))) - )); - } - #[test] fn parses_create_scorer() { let parsed = ScorersArgsHarness::try_parse_from([ @@ -96,4 +78,44 @@ mod tests { Some(ScorersCommands::Create(_)) )); } + + #[test] + fn parses_create_classifier() { + let parsed = ScorersArgsHarness::try_parse_from([ + "bt-scorers", + "create", + "Test classifier", + "--model", + "gpt-test", + "--messages", + r#"[{"role":"user","content":"Classify {{output}}"}]"#, + "--classifications", + r#"["safe","unsafe"]"#, + "--allow-no-match", + ]) + .expect("parse create classifier"); + + assert!(matches!( + parsed.args.command, + Some(ScorersCommands::Create(_)) + )); + } + + #[test] + fn still_parses_shared_scorer_commands() { + let parsed = ScorersArgsHarness::try_parse_from([ + "bt-scorers", + "update", + "test-scorer", + "--model", + "gpt-test", + "--yes", + ]) + .expect("parse update"); + + assert!(matches!( + parsed.args.command, + Some(ScorersCommands::Function(FunctionCommands::Update(_))) + )); + } } diff --git a/tests/cli.rs b/tests/cli.rs index d159d78d..42d30de7 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -972,6 +972,21 @@ fn scorers_create_help_includes_llm_judge_configuration() { .stdout(predicate::str::contains("bt functions push scorer.py")); } +#[test] +fn scorer_update_help_is_conflict_free() { + bt_command() + .args(["scorers", "update", "--help"]) + .assert() + .success() + .stdout(predicate::str::contains("--patch ")) + .stdout(predicate::str::contains("--patch-file").not()) + .stdout(predicate::str::contains("--messages ")) + .stdout(predicate::str::contains("--temperature")) + .stdout(predicate::str::contains("--classifications")) + .stdout(predicate::str::contains("--pass-threshold")) + .stdout(predicate::str::contains("--metadata")); +} + #[test] fn topics_report_help_accepts_global_org_short_conflict_free() { bt_command() From f5172b4575381861d9d374f0c1803af6523395e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Halber?= Date: Thu, 13 Aug 2026 18:36:16 -0700 Subject: [PATCH 11/12] feat(functions): update functions tools and scorers --- README.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/README.md b/README.md index d28feff8..38e9d5e9 100644 --- a/README.md +++ b/README.md @@ -187,6 +187,17 @@ bt tools update my-tool --patch @tool-patch.json The API replaces `prompt_data` rather than merging it, so `bt` reads the current definition and sends a materialized replacement with your changes. A concurrent edit can therefore be overwritten. +Update only the fields you specify, or use `--patch` for fields without dedicated flags: + +```bash +bt scorers update helpfulness --messages @messages.json +bt scorers update helpfulness --model gpt-5.4-nano +bt functions update my-function --description "Updated" +bt tools update my-tool --patch @tool-patch.json +``` + +The API replaces `prompt_data` rather than merging it, so `bt` reads the current definition and sends a materialized replacement with your changes. A concurrent edit can therefore be overwritten. + For TypeScript and Python code scorers, use the Braintrust SDK and `bt functions push`. ## `bt eval` From 28c8207a76b847ac4716b3d8bc1be5888e8004fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Halber?= Date: Thu, 13 Aug 2026 18:37:07 -0700 Subject: [PATCH 12/12] feat(prompts): update prompts --- README.md | 48 ++-- src/functions/create.rs | 49 +---- src/functions/mod.rs | 1 + src/functions/update.rs | 25 +-- src/functions/validation.rs | 104 +++++++++ src/prompts/api.rs | 11 + src/prompts/mod.rs | 28 +++ src/prompts/update.rs | 425 ++++++++++++++++++++++++++++++++++++ tests/cli.rs | 14 ++ 9 files changed, 614 insertions(+), 91 deletions(-) create mode 100644 src/functions/validation.rs create mode 100644 src/prompts/update.rs diff --git a/README.md b/README.md index 38e9d5e9..61961a7f 100644 --- a/README.md +++ b/README.md @@ -135,25 +135,25 @@ Remove-Item -Recurse -Force (Join-Path $env:APPDATA "bt") -ErrorAction SilentlyC ## Commands -| Command | Description | -| ------------- | ------------------------------------------------------------------ | -| `bt init` | Initialize `.bt/` config directory and link to a project | -| `bt login` | Log in to Braintrust or refresh an OAuth login | -| `bt logout` | Remove a saved Braintrust login | -| `bt switch` | Switch org and project context | -| `bt status` | Show current org and project context | -| `bt datasets` | Manage datasets and dataset pipelines | -| `bt eval` | Run eval files (Unix only) | -| `bt sql` | Run SQL queries against Braintrust | -| `bt view` | View logs, traces, and spans | -| `bt projects` | Manage projects (list, create, view, delete) | -| `bt datasets` | Manage remote datasets (list, create, update, view, delete) | -| `bt prompts` | Manage prompts (list, view, delete) | -| `bt functions` | Manage functions (list, view, invoke, update, push, pull, delete) | +| Command | Description | +| -------------- | ------------------------------------------------------------------ | +| `bt init` | Initialize `.bt/` config directory and link to a project | +| `bt login` | Log in to Braintrust or refresh an OAuth login | +| `bt logout` | Remove a saved Braintrust login | +| `bt switch` | Switch org and project context | +| `bt status` | Show current org and project context | +| `bt datasets` | Manage datasets and dataset pipelines | +| `bt eval` | Run eval files (Unix only) | +| `bt sql` | Run SQL queries against Braintrust | +| `bt view` | View logs, traces, and spans | +| `bt projects` | Manage projects (list, create, view, delete) | +| `bt datasets` | Manage remote datasets (list, create, update, view, delete) | +| `bt prompts` | Manage prompts (list, view, update, delete) | +| `bt functions` | Manage functions (list, view, invoke, update, push, pull, delete) | | `bt tools` | Manage tools (list, view, invoke, update, delete) | -| `bt scorers` | Manage scorers (list, create, view, invoke, update, delete) | -| `bt sync` | Synchronize project logs between Braintrust and local NDJSON files | -| `bt update` | Update bt in-place | +| `bt scorers` | Manage scorers (list, create, view, invoke, update, delete) | +| `bt sync` | Synchronize project logs between Braintrust and local NDJSON files | +| `bt update` | Update bt in-place | ## `bt scorers` @@ -183,17 +183,7 @@ bt scorers update helpfulness --messages @messages.json bt scorers update helpfulness --model gpt-5.4-nano bt functions update my-function --description "Updated" bt tools update my-tool --patch @tool-patch.json -``` - -The API replaces `prompt_data` rather than merging it, so `bt` reads the current definition and sends a materialized replacement with your changes. A concurrent edit can therefore be overwritten. - -Update only the fields you specify, or use `--patch` for fields without dedicated flags: - -```bash -bt scorers update helpfulness --messages @messages.json -bt scorers update helpfulness --model gpt-5.4-nano -bt functions update my-function --description "Updated" -bt tools update my-tool --patch @tool-patch.json +bt prompts update my-prompt --messages @messages.json ``` The API replaces `prompt_data` rather than merging it, so `bt` reads the current definition and sends a materialized replacement with your changes. A concurrent edit can therefore be overwritten. diff --git a/src/functions/create.rs b/src/functions/create.rs index 28cce67e..dc10c36a 100644 --- a/src/functions/create.rs +++ b/src/functions/create.rs @@ -126,7 +126,7 @@ pub(crate) async fn run(ctx: &ResolvedContext, args: &CreateArgs, json_output: b api::validate_functions(&ctx.client, std::slice::from_ref(&definition)), ) .await?; - report_validation_issues(&validation).map_err(UserError::from)?; + super::validation::report_issues(&validation, "scorer definition").map_err(UserError::from)?; let result = match with_spinner( "Creating scorer...", @@ -176,53 +176,6 @@ pub(crate) async fn run(ctx: &ResolvedContext, args: &CreateArgs, json_output: b Ok(()) } -fn report_validation_issues(report: &api::FunctionValidationReport) -> Result<()> { - let mut blocking = Vec::new(); - for result in &report.results { - for issue in &result.issues { - let path = issue - .path - .iter() - .map(|part| { - part.as_str() - .map(ToOwned::to_owned) - .unwrap_or_else(|| part.to_string()) - }) - .collect::>() - .join("."); - let location = if path.is_empty() { - issue.code.clone() - } else { - path - }; - let suggestion = issue - .suggestion - .as_ref() - .map( - |suggestion| match (suggestion.action.as_str(), &suggestion.value) { - ("remove", _) => "; suggestion: remove this parameter".to_string(), - ("set", Some(value)) => format!("; suggestion: set it to {value}"), - _ => String::new(), - }, - ) - .unwrap_or_default(); - let message = format!("{location}: {}{suggestion}", issue.message); - if issue.blocking { - blocking.push(message); - } else { - print_command_status(CommandStatus::Warning, &message); - } - } - } - if blocking.is_empty() && report.valid { - Ok(()) - } else if blocking.is_empty() { - bail!("the backend rejected the scorer definition") - } else { - bail!(blocking.join("; ")) - } -} - fn resolve_name(args: &CreateArgs) -> Result { let name = match args.name_positional.as_deref().or(args.name.as_deref()) { Some(name) => name.trim().to_string(), diff --git a/src/functions/mod.rs b/src/functions/mod.rs index aa445272..9a22b6b0 100644 --- a/src/functions/mod.rs +++ b/src/functions/mod.rs @@ -23,6 +23,7 @@ mod pull; mod push; pub(crate) mod report; mod update; +pub(crate) mod validation; mod view; use api::Function; diff --git a/src/functions/update.rs b/src/functions/update.rs index 7be92192..05e28320 100644 --- a/src/functions/update.rs +++ b/src/functions/update.rs @@ -4,6 +4,7 @@ use dialoguer::Confirm; use serde_json::{json, Map, Value}; use crate::{ + error::UserError, ui::{is_interactive, print_command_status, with_spinner, CommandStatus}, utils::{merge_json_objects, read_text_source, read_yaml_object_source}, }; @@ -11,10 +12,11 @@ use crate::{ use super::{api, label, label_plural, select_function_interactive}; use super::{ prompt_config::{ - parse_choice_scores_source, parse_classifications_source, validate_prompt_data_patch, - validate_unit_interval, PromptConfigArgs, + parse_choice_scores_source, parse_classifications_source, validate_unit_interval, + PromptConfigArgs, }, prompt_patch::materialize_prompt_data_patch, + validation::{build_candidate, report_issues}, FunctionTypeFilter, ResolvedContext, }; @@ -195,19 +197,14 @@ pub async fn run( ); } - // Last of the up-front checks because it may hit the network. - with_spinner( - "Validating model parameters...", - validate_prompt_data_patch( - ctx, - function.prompt_data.as_ref(), - &mut body, - args.prompt_config.refresh_models(), - ), - ) - .await? - .warn_if_incomplete(); materialize_prompt_data_patch(&mut body, function.prompt_data.as_ref()); + let candidate = build_candidate(&function, &body)?; + let validation = with_spinner( + "Validating function...", + api::validate_functions(&ctx.client, std::slice::from_ref(&candidate)), + ) + .await?; + report_issues(&validation, "function definition").map_err(UserError::from)?; // Switching output mode updates function_type, but materialization merges // the parser and does not drop the previous mode's keys. Warn so the diff --git a/src/functions/validation.rs b/src/functions/validation.rs new file mode 100644 index 00000000..a2b6c3f0 --- /dev/null +++ b/src/functions/validation.rs @@ -0,0 +1,104 @@ +use anyhow::{bail, Context, Result}; +use serde::Serialize; +use serde_json::Value; + +use crate::ui::{print_command_status, CommandStatus}; + +use super::api::FunctionValidationReport; + +/// Build the complete candidate definition that a partial update would produce. +/// +/// Function and prompt PATCH endpoints replace values at the top level. Callers +/// must materialize replacement objects such as `prompt_data` before calling +/// this helper. +pub(crate) fn build_candidate(existing: &T, patch: &Value) -> Result { + let mut candidate = serde_json::to_value(existing).context("failed to serialize definition")?; + let patch = patch + .as_object() + .context("definition patch must be a JSON object")?; + let candidate_object = candidate + .as_object_mut() + .context("existing definition must be a JSON object")?; + + for (key, value) in patch { + candidate_object.insert(key.clone(), value.clone()); + } + + Ok(candidate) +} + +pub(crate) fn report_issues(report: &FunctionValidationReport, definition: &str) -> Result<()> { + let mut blocking = Vec::new(); + for result in &report.results { + for issue in &result.issues { + let path = issue + .path + .iter() + .map(|part| { + part.as_str() + .map(ToOwned::to_owned) + .unwrap_or_else(|| part.to_string()) + }) + .collect::>() + .join("."); + let location = if path.is_empty() { + issue.code.clone() + } else { + path + }; + let suggestion = issue + .suggestion + .as_ref() + .map( + |suggestion| match (suggestion.action.as_str(), &suggestion.value) { + ("remove", _) => "; suggestion: remove this parameter".to_string(), + ("set", Some(value)) => format!("; suggestion: set it to {value}"), + _ => String::new(), + }, + ) + .unwrap_or_default(); + let message = format!("{location}: {}{suggestion}", issue.message); + if issue.blocking { + blocking.push(message); + } else { + print_command_status(CommandStatus::Warning, &message); + } + } + } + if blocking.is_empty() && report.valid { + Ok(()) + } else if blocking.is_empty() { + bail!("the backend rejected the {definition}") + } else { + bail!(blocking.join("; ")) + } +} + +#[cfg(test)] +mod tests { + use serde_json::json; + + use super::*; + + #[test] + fn candidate_uses_top_level_patch_replacement_semantics() { + let existing = json!({ + "name": "test-function", + "metadata": {"preserved": true}, + "prompt_data": {"options": {"model": "test-model"}} + }); + let patch = json!({ + "metadata": {"replacement": true}, + "prompt_data": {"options": {"model": "test-model-2"}} + }); + + let candidate = build_candidate(&existing, &patch).expect("candidate"); + + assert_eq!(candidate["name"], "test-function"); + assert_eq!(candidate["metadata"], json!({"replacement": true})); + assert_eq!( + candidate["prompt_data"], + json!({"options": {"model": "test-model-2"}}) + ); + } +} diff --git a/src/prompts/api.rs b/src/prompts/api.rs index 5a40a8e7..fe1e6507 100644 --- a/src/prompts/api.rs +++ b/src/prompts/api.rs @@ -1,5 +1,6 @@ use anyhow::Result; use serde::{Deserialize, Serialize}; +use serde_json::Value; use urlencoding::encode; use crate::http::ApiClient; @@ -51,3 +52,13 @@ pub async fn delete_prompt(client: &ApiClient, prompt_id: &str) -> Result<()> { let path = format!("/v1/prompt/{}", encode(prompt_id)); client.delete(&path).await } + +/// Partially update a prompt by id via `PATCH /v1/prompt/{id}`. +/// +/// Top-level fields are patched, but object-valued fields such as `prompt_data` +/// are replaced wholesale. Callers updating `prompt_data` must materialize the +/// complete value before sending the request. +pub async fn patch_prompt(client: &ApiClient, prompt_id: &str, body: &Value) -> Result { + let path = format!("/v1/prompt/{}", encode(prompt_id)); + client.patch(&path, body).await +} diff --git a/src/prompts/mod.rs b/src/prompts/mod.rs index 440ac341..bfe6bf41 100644 --- a/src/prompts/mod.rs +++ b/src/prompts/mod.rs @@ -8,6 +8,7 @@ pub(crate) use crate::project_context::ProjectContext as ResolvedContext; mod api; mod delete; mod list; +mod update; mod view; #[derive(Debug, Clone, Args)] @@ -16,6 +17,7 @@ Examples: bt prompts list bt prompts view my-prompt bt prompts delete my-prompt + bt prompts update my-prompt --messages @messages.json ")] pub struct PromptsArgs { #[command(subcommand)] @@ -28,6 +30,8 @@ enum PromptsCommands { List, /// View a prompt's content View(ViewArgs), + /// Update a prompt in place (prompt configuration, metadata, or arbitrary patch) + Update(Box), /// Delete a prompt Delete(DeleteArgs), } @@ -87,6 +91,7 @@ pub async fn run(base: BaseArgs, args: PromptsArgs) -> Result<()> { Some(PromptsCommands::View(p)) => { view::run(&ctx, p.slug(), base.json, p.web, base.verbose).await } + Some(PromptsCommands::Update(p)) => update::run(&ctx, &p, base.json).await, Some(PromptsCommands::Delete(p)) => delete::run(&ctx, p.slug(), p.force).await, } } @@ -100,8 +105,16 @@ fn prompts_command_is_read_only(command: Option<&PromptsCommands>) -> bool { #[cfg(test)] mod tests { + use clap::Parser; + use super::*; + #[derive(Debug, Parser)] + struct PromptsArgsHarness { + #[command(flatten)] + args: PromptsArgs, + } + #[test] fn prompts_routes_list_and_view_to_read_only_auth() { assert!(prompts_command_is_read_only(None)); @@ -125,4 +138,19 @@ mod tests { }) ))); } + + #[test] + fn prompts_routes_update_to_validated_auth() { + let parsed = PromptsArgsHarness::try_parse_from([ + "bt-prompts", + "update", + "my-prompt", + "--description", + "updated", + "--yes", + ]) + .expect("parse update"); + + assert!(!prompts_command_is_read_only(parsed.args.command.as_ref())); + } } diff --git a/src/prompts/update.rs b/src/prompts/update.rs new file mode 100644 index 00000000..e145bed3 --- /dev/null +++ b/src/prompts/update.rs @@ -0,0 +1,425 @@ +use anyhow::{anyhow, bail, Context, Result}; +use clap::Args; +use dialoguer::Confirm; +use serde_json::{json, Map, Value}; + +use crate::{ + error::UserError, + functions::{ + api as function_api, + prompt_config::PromptConfigArgs, + prompt_patch::materialize_prompt_data_patch, + validation::{build_candidate, report_issues}, + }, + ui::{is_interactive, print_command_status, with_spinner, CommandStatus}, + utils::{merge_json_objects, read_text_source, read_yaml_object_source}, +}; + +use super::{api, ResolvedContext}; + +/// Update a prompt's configuration or metadata in place. +/// +/// The endpoint replaces `prompt_data` wholesale, so the command reads the +/// current prompt and materializes a complete replacement while changing only +/// the fields requested by the user. +#[derive(Debug, Clone, Args)] +#[command(after_help = "\ +Examples: + bt prompts update my-prompt --messages @messages.json + bt prompts update my-prompt --model gpt-5.4-nano + bt prompts update my-prompt --description \"Customer support prompt\" + bt prompts update my-prompt --patch '{\"prompt_data\":{\"options\":{\"model\":\"gpt-5.4-nano\"}}}' + bt prompts update my-prompt --patch @prompt-patch.json +")] +pub struct UpdateArgs { + /// Prompt slug (positional) + #[arg(value_name = "SLUG", conflicts_with = "slug_flag")] + slug_positional: Option, + + /// Prompt slug (flag) + #[arg(long = "slug", short = 's')] + slug_flag: Option, + + /// Replacement chat messages source: inline JSON, @PATH to read from a + /// file, or - for stdin. + #[arg(long, value_name = "SOURCE")] + messages: Option, + + /// Update the model used by the prompt. + #[arg(long, short = 'm', value_name = "MODEL")] + model: Option, + + #[command(flatten)] + prompt_config: PromptConfigArgs, + + /// Deep-merge metadata from inline YAML, @PATH, or stdin (-). + #[arg(long, value_name = "SOURCE")] + metadata: Option, + + /// Update the prompt description. + #[arg(long, short = 'd', value_name = "TEXT")] + description: Option, + + /// Arbitrary JSON object deep-merged into the prompt. Accepts inline JSON, + /// @PATH to read JSON from a file, or - for stdin. + #[arg(long, value_name = "SOURCE")] + patch: Option, + + /// Skip the confirmation prompt. + #[arg(long, short = 'y')] + yes: bool, +} + +impl UpdateArgs { + fn slug(&self) -> Option<&str> { + self.slug_positional + .as_deref() + .or(self.slug_flag.as_deref()) + } +} + +pub async fn run(ctx: &ResolvedContext, args: &UpdateArgs, json_output: bool) -> Result<()> { + let project_name = &ctx.project.name; + let mut body = build_patch_body(args)?; + + let prompt = match args.slug() { + Some(slug) => with_spinner( + "Loading prompt...", + api::get_prompt_by_slug(&ctx.client, project_name, slug), + ) + .await? + .ok_or_else(|| anyhow!("prompt with slug '{slug}' not found"))?, + None => { + if !is_interactive() { + bail!("prompt slug required. Use: bt prompts update [--patch ...]"); + } + super::delete::select_prompt_interactive(&ctx.client, project_name).await? + } + }; + + materialize_prompt_data_patch(&mut body, prompt.prompt_data.as_ref()); + let candidate = build_candidate(&prompt, &body)?; + let validation = with_spinner( + "Validating prompt...", + function_api::validate_functions(&ctx.client, std::slice::from_ref(&candidate)), + ) + .await?; + report_issues(&validation, "prompt definition").map_err(UserError::from)?; + + if !args.yes && is_interactive() { + let confirm = Confirm::new() + .with_prompt(format!( + "Update prompt '{}' in {}?", + prompt.name, project_name + )) + .default(false) + .interact()?; + if !confirm { + return Ok(()); + } + } + + let updated = match with_spinner( + "Updating prompt...", + api::patch_prompt(&ctx.client, &prompt.id, &body), + ) + .await + { + Ok(value) => { + print_command_status( + CommandStatus::Success, + &format!("Updated '{}'", prompt.name), + ); + value + } + Err(error) => { + print_command_status( + CommandStatus::Error, + &format!("Failed to update '{}'", prompt.name), + ); + return Err(error); + } + }; + + if json_output { + println!("{}", serde_json::to_string(&updated)?); + } else if !crate::ui::is_quiet() { + eprintln!( + "Run `bt prompts view {}` to inspect the updated prompt.", + prompt.slug + ); + } + + Ok(()) +} + +fn build_patch_body(args: &UpdateArgs) -> Result { + let mut patch: Map = Map::new(); + + if let Some(description) = args.description.as_deref() { + patch.insert( + "description".to_string(), + Value::String(description.to_string()), + ); + } + + if let Some(source) = args.metadata.as_deref() { + patch.insert( + "metadata".to_string(), + Value::Object(read_yaml_object_source(source, "prompt metadata")?), + ); + } + + if let Some(messages) = resolve_messages(args)? { + let prompt_data_patch = json!({ + "prompt_data": { + "prompt": { "type": "chat", "messages": messages }, + }, + }); + merge_json_objects( + &mut patch, + prompt_data_patch + .as_object() + .expect("prompt data patch is an object"), + ); + } + + let prompt_config = args + .prompt_config + .build_prompt_data_patch(args.model.as_deref())?; + if !prompt_config.is_empty() { + let prompt_data_patch = json!({ "prompt_data": prompt_config }); + merge_json_objects( + &mut patch, + prompt_data_patch + .as_object() + .expect("prompt data patch is an object"), + ); + } + + let extra = resolve_extra_patch(args)?; + if let Some(extra_obj) = extra { + merge_json_objects(&mut patch, &extra_obj); + } + + if patch.is_empty() { + bail!("no updates requested. Pass an update flag; see `bt prompts update --help`"); + } + + Ok(Value::Object(patch)) +} + +fn resolve_messages(args: &UpdateArgs) -> Result> { + match args.messages.as_deref() { + Some(source) => { + let raw = read_text_source(source, "messages")?; + let parsed: Value = serde_json::from_str(&raw).context("invalid JSON in --messages")?; + match parsed { + Value::Array(_) => Ok(Some(parsed)), + _ => bail!("--messages must be a JSON array of chat messages"), + } + } + None => Ok(None), + } +} + +fn resolve_extra_patch(args: &UpdateArgs) -> Result>> { + let Some(source) = args.patch.as_deref() else { + return Ok(None); + }; + let raw = read_text_source(source, "patch")?; + parse_patch_object(&raw).map(Some) +} + +fn parse_patch_object(raw: &str) -> Result> { + let value: Value = serde_json::from_str(raw).context("invalid JSON in --patch")?; + match value { + Value::Object(map) => Ok(map), + _ => bail!("--patch must be a JSON object"), + } +} + +#[cfg(test)] +mod tests { + use clap::Parser; + + use super::*; + + #[derive(Debug, Parser)] + struct UpdateArgsHarness { + #[command(flatten)] + args: UpdateArgs, + } + + fn args(model: Option<&str>, description: Option<&str>) -> UpdateArgs { + UpdateArgs { + slug_positional: Some("test-prompt".to_string()), + slug_flag: None, + messages: None, + model: model.map(ToOwned::to_owned), + prompt_config: PromptConfigArgs::default(), + metadata: None, + description: description.map(ToOwned::to_owned), + patch: None, + yes: true, + } + } + + #[test] + fn build_patch_body_messages_writes_chat_block() { + let mut args = args(None, None); + args.messages = Some(r#"[{"role":"user","content":"hi"}]"#.to_string()); + let body = build_patch_body(&args).expect("patch body"); + assert_eq!( + body["prompt_data"]["prompt"]["type"], + serde_json::json!("chat") + ); + assert_eq!( + body["prompt_data"]["prompt"]["messages"], + serde_json::json!([{"role":"user","content":"hi"}]) + ); + } + + #[test] + fn build_patch_body_model_merges_into_prompt_data() { + let args = args(Some("gpt-4o-mini"), None); + let body = build_patch_body(&args).expect("patch body"); + assert_eq!( + body["prompt_data"]["options"]["model"], + serde_json::json!("gpt-4o-mini") + ); + } + + #[test] + fn build_patch_body_messages_and_model_combine() { + let mut args = args(Some("gpt-4o-mini"), None); + args.messages = Some(r#"[{"role":"user","content":"Answer it."}]"#.to_string()); + let body = build_patch_body(&args).expect("patch body"); + assert_eq!( + body["prompt_data"]["prompt"]["messages"], + json!([{"role": "user", "content": "Answer it."}]) + ); + assert_eq!( + body["prompt_data"]["options"]["model"], + serde_json::json!("gpt-4o-mini") + ); + } + + #[test] + fn build_patch_body_updates_prompt_configuration_and_metadata() { + let parsed = UpdateArgsHarness::try_parse_from([ + "test", + "test-prompt", + "--temperature", + "0.3", + "--max-tokens", + "100", + "--template-format", + "mustache", + "--metadata", + "owner: test-team", + ]) + .expect("parse update"); + + let body = build_patch_body(&parsed.args).expect("patch body"); + assert_eq!(body["prompt_data"]["options"]["params"]["temperature"], 0.3); + assert_eq!(body["prompt_data"]["options"]["params"]["max_tokens"], 100); + assert_eq!(body["prompt_data"]["template_format"], "mustache"); + assert_eq!(body["metadata"]["owner"], "test-team"); + } + + #[test] + fn build_patch_body_description_is_top_level() { + let args = args(None, Some("Customer support prompt")); + let body = build_patch_body(&args).expect("patch body"); + assert_eq!( + body["description"], + serde_json::json!("Customer support prompt") + ); + } + + #[test] + fn build_patch_body_reads_at_prefixed_messages_file() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("messages.json"); + std::fs::write( + &path, + r#"[{"role":"user","content":"Answer from a file."}]"#, + ) + .expect("write messages"); + let source = format!("@{}", path.display()); + + let mut args = args(None, None); + args.messages = Some(source); + let body = build_patch_body(&args).expect("patch body"); + assert_eq!( + body["prompt_data"]["prompt"]["messages"], + json!([{"role": "user", "content": "Answer from a file."}]) + ); + } + + #[test] + fn build_patch_body_reads_at_prefixed_patch_file() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("patch.json"); + std::fs::write(&path, r#"{"description":"From a file"}"#).expect("write patch"); + let source = format!("@{}", path.display()); + + let mut args = args(None, None); + args.patch = Some(source); + let body = build_patch_body(&args).expect("patch body"); + assert_eq!(body["description"], "From a file"); + } + + #[test] + fn build_patch_body_rejects_empty_update() { + let args = args(None, None); + let err = build_patch_body(&args).expect_err("should reject empty"); + assert!(err.to_string().contains("no updates requested")); + } + + #[test] + fn build_patch_body_extra_patch_merges_into_prompt_data() { + let mut args = args(None, None); + args.patch = + Some(r#"{"prompt_data":{"options":{"params":{"temperature":0}}}}"#.to_string()); + let body = build_patch_body(&args).expect("patch body"); + assert_eq!( + body["prompt_data"]["options"]["params"]["temperature"], + serde_json::json!(0) + ); + } + + #[test] + fn parse_patch_object_rejects_non_object() { + let err = parse_patch_object("[1,2,3]").expect_err("should reject"); + assert!(err.to_string().contains("JSON object")); + } + + #[test] + fn merge_objects_deep_merges_nested_maps() { + let mut target = serde_json::json!({ + "prompt_data": { "options": { "model": "gpt-4o" } } + }) + .as_object() + .expect("object") + .clone(); + let source = serde_json::json!({ + "prompt_data": { "options": { "temperature": 0 } } + }) + .as_object() + .expect("object") + .clone(); + + merge_json_objects(&mut target, &source); + + assert_eq!( + target["prompt_data"]["options"]["model"], + serde_json::json!("gpt-4o") + ); + assert_eq!( + target["prompt_data"]["options"]["temperature"], + serde_json::json!(0) + ); + } +} diff --git a/tests/cli.rs b/tests/cli.rs index 42d30de7..bb058748 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -987,6 +987,20 @@ fn scorer_update_help_is_conflict_free() { .stdout(predicate::str::contains("--metadata")); } +#[test] +fn prompt_update_help_is_conflict_free() { + bt_command() + .args(["prompts", "update", "--help"]) + .assert() + .success() + .stdout(predicate::str::contains("--patch ")) + .stdout(predicate::str::contains("--patch-file").not()) + .stdout(predicate::str::contains("--messages ")) + .stdout(predicate::str::contains("--temperature")) + .stdout(predicate::str::contains("--template-format")) + .stdout(predicate::str::contains("--metadata")); +} + #[test] fn topics_report_help_accepts_global_org_short_conflict_free() { bt_command()