diff --git a/README.md b/README.md index f17dd1eb..6a82f544 100644 --- a/README.md +++ b/README.md @@ -174,6 +174,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`. +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`. ## `bt eval` diff --git a/src/error.rs b/src/error.rs new file mode 100644 index 00000000..0776a741 --- /dev/null +++ b/src/error.rs @@ -0,0 +1,20 @@ +use std::fmt; + +/// An expected error caused by command input rather than an internal failure. +#[derive(Debug)] +pub(crate) struct UserError { + message: String, +} + +impl fmt::Display for UserError { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str(&self.message) + } +} + +impl std::error::Error for UserError {} + +pub(crate) fn user_error(error: anyhow::Error) -> anyhow::Error { + let message = error.to_string(); + error.context(UserError { message }) +} diff --git a/src/functions/api.rs b/src/functions/api.rs index ea5d6bd9..0e526cfd 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::user_error, + http::{ApiClient, HttpError}, +}; fn escape_sql(s: &str) -> String { s.replace('\'', "''") @@ -66,6 +69,47 @@ 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)] +pub struct FunctionValidationError { + pub report: FunctionValidationReport, +} + +impl std::fmt::Display for FunctionValidationError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("the backend rejected the function definition") + } +} + +impl std::error::Error for FunctionValidationError {} + #[derive(Debug, Clone)] pub struct InsertFunctionsResult { pub ignored_entries: Option, @@ -164,9 +208,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(user_error(error)) + } + 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<()> { @@ -283,10 +361,20 @@ pub async fn insert_functions( functions: &[Value], ) -> Result { let body = insert_functions_body(functions); - let raw: Value = client - .post("/insert-functions", &body) - .await - .context("failed to insert functions")?; + let raw: Value = match client.post("/insert-functions", &body).await { + Ok(raw) => raw, + Err(error) => { + let Some(http_error) = error.downcast_ref::() else { + return Err(error).context("failed to insert functions"); + }; + if http_error.status != reqwest::StatusCode::UNPROCESSABLE_ENTITY { + return Err(error).context("failed to insert functions"); + } + let report = serde_json::from_str(&http_error.body) + .context("unexpected insert-functions validation error response shape")?; + return Err(FunctionValidationError { report }.into()); + } + }; let response: InsertFunctionsResponse = serde_json::from_value(raw.clone()) .context("unexpected insert-functions response shape")?; @@ -333,6 +421,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..e9f3576e 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::user_error, ui::{is_interactive, print_command_status, with_spinner, CommandStatus}, utils::{merge_json_objects, read_text_source, read_yaml_object_source}, }; @@ -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,10 @@ 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(user_error)?; + let slug = resolve_slug(args, &name).map_err(user_error)?; + let definition = + build_scorer_definition(args, &ctx.project.id, &name, &slug).map_err(user_error)?; let result = match with_spinner( "Creating scorer...", @@ -128,6 +130,9 @@ pub(crate) async fn run(ctx: &ResolvedContext, args: &CreateArgs, json_output: b { Ok(result) => result, Err(error) => { + if let Some(validation_error) = error.downcast_ref::() { + return report_validation_issues(&validation_error.report).map_err(user_error); + } print_command_status(CommandStatus::Error, &format!("Failed to create '{name}'")); return Err(error); } @@ -168,16 +173,62 @@ 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, &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 +445,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 +509,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..03c5301a 100644 --- a/src/functions/mod.rs +++ b/src/functions/mod.rs @@ -185,11 +185,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/prompt_config.rs b/src/functions/prompt_config.rs index 2a120a1b..f77cb095 100644 --- a/src/functions/prompt_config.rs +++ b/src/functions/prompt_config.rs @@ -8,23 +8,29 @@ use crate::utils::read_text_source; #[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 +46,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, @@ -139,20 +159,13 @@ 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())); } insert_optional_number(&mut params, "frequency_penalty", self.frequency_penalty)?; insert_optional_number(&mut params, "presence_penalty", self.presence_penalty)?; @@ -190,6 +203,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(), @@ -380,7 +402,7 @@ mod tests { "--top-p", "0.9", "--frequency-penalty", - "-0.5", + "0.5", "--presence-penalty", "0.25", "--stop-sequence", @@ -409,7 +431,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!( @@ -434,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!( diff --git a/src/main.rs b/src/main.rs index 2b17cb17..4060a8be 100644 --- a/src/main.rs +++ b/src/main.rs @@ -9,6 +9,7 @@ mod config; mod datasets; mod env; mod environments; +mod error; #[cfg(unix)] mod eval; mod experiments; @@ -472,9 +473,13 @@ fn classify_error(err: &anyhow::Error, missing_credential: bool) -> ExitCode { return ExitCode::from_process_code(err.1); } + 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) { @@ -509,22 +514,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() @@ -568,6 +557,10 @@ fn has_io_error(err: &anyhow::Error) -> bool { .any(|source| source.downcast_ref::().is_some()) } +fn has_user_error(err: &anyhow::Error) -> bool { + err.downcast_ref::().is_some() +} + fn looks_like_user_error(err: &anyhow::Error) -> bool { let message = err.to_string().to_lowercase(); message.contains("required") @@ -577,9 +570,31 @@ 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) { @@ -734,9 +749,17 @@ mod tests { parts.iter().map(OsString::from).collect() } + #[test] + fn typed_user_errors_use_the_user_exit_code() { + let err = + crate::error::user_error(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 { + let err = crate::error::user_error(anyhow::Error::new(crate::http::HttpError { status: reqwest::StatusCode::UNAUTHORIZED, body: serde_json::json!({ "error": { @@ -747,20 +770,53 @@ mod tests { "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/text_source.rs b/src/utils/text_source.rs index 1f2ce754..7c90b96d 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,13 +20,18 @@ fn read_text_source_with_stdin_guard( stdin_reader: &Mutex>, ) -> Result { if value == "-" { - // The second reader would otherwise see "" and call it malformed input. + // Check this first so a duplicate source reports the actual conflict, + // even when the process's stdin is an interactive terminal. 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 '-'"); } + + ensure_stdin_is_piped(label, std::io::stdin().is_terminal())?; + + // The second reader would otherwise see "" and call it malformed input. *reader = Some(label.to_string()); drop(reader); @@ -52,6 +57,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 +105,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/cli.rs b/tests/cli.rs index cdcbd80d..5430134a 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -10,6 +10,8 @@ fn bt_command() -> Command { fn clear_braintrust_auth_env(cmd: &mut Command) { for key in [ "BRAINTRUST_API_KEY", + "BRAINTRUST_API_URL", + "BRAINTRUST_APP_URL", "BRAINTRUST_PROFILE", "BRAINTRUST_ORG_NAME", "BRAINTRUST_DEFAULT_PROJECT", diff --git a/tests/functions.rs b/tests/functions.rs index 1ae13634..664c2a13 100644 --- a/tests/functions.rs +++ b/tests/functions.rs @@ -130,15 +130,7 @@ fn find_tsc() -> Option { } else { repo_root().join("node_modules").join(".bin").join("tsc") }; - if local.is_file() { - return Some(local); - } - - if command_exists("tsc") { - return Some(PathBuf::from("tsc")); - } - - None + local.is_file().then_some(local) } fn compile_functions_runner(tsc: &Path, root: &Path, runner_dir: &Path) { @@ -763,12 +755,22 @@ fn root_login_refresh_uses_selected_profile() { .env("APPDATA", config_dir.path()) .env("BRAINTRUST_NO_COLOR", "1") .env_remove("BRAINTRUST_API_KEY") + .env_remove("BRAINTRUST_API_URL") + .env_remove("BRAINTRUST_APP_URL") .env_remove("BRAINTRUST_ORG_NAME"); let output = cmd.output().expect("run bt login --refresh"); assert!(!output.status.success()); - assert!(String::from_utf8_lossy(&output.stdout) - .contains("`bt login --refresh` only applies to oauth profiles")); + let payload: Value = serde_json::from_slice(&output.stdout) + .expect("JSON failures should emit a machine-readable payload on stdout"); + assert!(payload["error"]["message"].as_str().is_some_and( + |message| message.contains("`bt login --refresh` only applies to oauth profiles") + )); + assert!( + output.stderr.is_empty(), + "JSON failure should not be emitted on stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); } #[test]