diff --git a/Cargo.lock b/Cargo.lock index 570383a03..d9defbe85 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1523,6 +1523,7 @@ dependencies = [ "fabro-test", "fabro-types", "fabro-util", + "fabro-vault", "futures", "glob", "htmd", @@ -1887,6 +1888,7 @@ dependencies = [ "tokio-stream", "tokio-util", "tracing", + "trybuild", "uuid", ] @@ -2307,6 +2309,7 @@ dependencies = [ "futures", "git2", "hex", + "httpmock", "md5", "mime_guess", "object_store", @@ -6428,6 +6431,12 @@ dependencies = [ "xattr", ] +[[package]] +name = "target-triple" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "591ef38edfb78ca4771ee32cf494cb8771944bee237a9b91fc9c1424ac4b777b" + [[package]] name = "temp-env" version = "0.3.6" @@ -6715,6 +6724,21 @@ dependencies = [ "winnow 0.7.14", ] +[[package]] +name = "toml" +version = "1.0.6+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "399b1124a3c9e16766831c6bba21e50192572cdd98706ea114f9502509686ffc" +dependencies = [ + "indexmap 2.13.0", + "serde_core", + "serde_spanned 1.0.4", + "toml_datetime 1.1.1+spec-1.1.0", + "toml_parser", + "toml_writer", + "winnow 0.7.14", +] + [[package]] name = "toml_datetime" version = "0.6.11" @@ -6733,6 +6757,15 @@ dependencies = [ "serde_core", ] +[[package]] +name = "toml_datetime" +version = "1.1.1+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3165f65f62e28e0115a00b2ebdd37eb6f3b641855f9d636d3cd4103767159ad7" +dependencies = [ + "serde_core", +] + [[package]] name = "toml_edit" version = "0.22.27" @@ -6895,6 +6928,21 @@ version = "0.2.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e421abadd41a4225275504ea4d6566923418b7f05506fbc9c0fe86ba7396114b" +[[package]] +name = "trybuild" +version = "1.0.116" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "47c635f0191bd3a2941013e5062667100969f8c4e9cd787c14f977265d73616e" +dependencies = [ + "glob", + "serde", + "serde_derive", + "serde_json", + "target-triple", + "termcolor", + "toml 1.0.6+spec-1.1.0", +] + [[package]] name = "tungstenite" version = "0.26.2" diff --git a/lib/crates/fabro-agent/Cargo.toml b/lib/crates/fabro-agent/Cargo.toml index ea575134a..2a1334a31 100644 --- a/lib/crates/fabro-agent/Cargo.toml +++ b/lib/crates/fabro-agent/Cargo.toml @@ -32,6 +32,7 @@ fabro-model = { path = "../fabro-model" } fabro-mcp = { path = "../fabro-mcp" } fabro-sandbox = { path = "../fabro-sandbox" } fabro-util = { path = "../fabro-util" } +fabro-vault = { path = "../fabro-vault" } fabro-http.workspace = true thiserror.workspace = true serde.workspace = true diff --git a/lib/crates/fabro-agent/src/cli.rs b/lib/crates/fabro-agent/src/cli.rs index 0798e070a..8a957159a 100644 --- a/lib/crates/fabro-agent/src/cli.rs +++ b/lib/crates/fabro-agent/src/cli.rs @@ -8,7 +8,8 @@ use std::path::PathBuf; use std::sync::{Arc, Mutex}; use clap::{Args, Parser}; -use fabro_auth::EnvCredentialSource; +use fabro_auth::{CredentialSource, EnvCredentialSource, VaultCredentialSource}; +use fabro_config::{Storage, load_settings_user, resolve_storage_root}; use fabro_llm::Error as LlmError; use fabro_llm::client::Client; use fabro_llm::middleware::{Middleware, NextFn, NextStreamFn}; @@ -17,9 +18,10 @@ use fabro_llm::types::{Request, Response}; use fabro_mcp::config::McpServerSettings; use fabro_model::{Catalog, ModelHandle, Provider}; use fabro_util::terminal::Styles; +use fabro_vault::Vault; use tokio::io::{AsyncWriteExt, stdout}; use tokio::signal; -use tokio::sync::Mutex as AsyncMutex; +use tokio::sync::{Mutex as AsyncMutex, RwLock as AsyncRwLock}; use crate::config::{ToolApprovalAdapter, ToolApprovalFn, ToolHookCallback}; use crate::error::InterruptReason; @@ -216,20 +218,18 @@ fn summarizer_model_id(provider: Provider) -> ModelHandle { } } -fn build_summarizer(provider: Provider, llm_client: Option) -> Option { - let client = llm_client?; - Some(WebFetchSummarizer { - client, +fn build_summarizer(provider: Provider, llm_client: Client) -> WebFetchSummarizer { + WebFetchSummarizer { + client: llm_client, model_id: summarizer_model_id(provider), - }) + } } fn build_profile( provider: Provider, model: &str, - llm_client: Option, + summarizer: Option, ) -> Box { - let summarizer = build_summarizer(provider, llm_client); match provider { Provider::OpenAi => Box::new(OpenAiProfile::with_summarizer(model, summarizer)), Provider::Kimi @@ -244,6 +244,45 @@ fn build_profile( } } +fn parse_provider(args: &AgentArgs) -> anyhow::Result { + let provider_str = args.provider.as_deref().unwrap_or("anthropic"); + provider_str + .parse() + .map_err(|_| anyhow::anyhow!("unknown provider: {provider_str}")) +} + +fn standalone_llm_source() -> anyhow::Result> { + fn env_lookup(name: &str) -> Option { + std::env::var(name).ok() + } + + let settings = load_settings_user()?; + let storage_root = resolve_storage_root(&settings); + + let storage_dir = match storage_root.resolve(&env_lookup) { + Ok(resolved) => PathBuf::from(resolved.value), + Err(_) => return Ok(Arc::new(EnvCredentialSource::new())), + }; + + let vault = Vault::load(Storage::new(&storage_dir).secrets_path()) + .map_err(|err| anyhow::anyhow!("Failed to load vault for LLM credentials: {err}"))?; + Ok(Arc::new(VaultCredentialSource::new(Arc::new( + AsyncRwLock::new(vault), + )))) +} + +fn ensure_provider_registered(client: &Client, provider: Provider) -> anyhow::Result<()> { + if client + .provider_names() + .iter() + .any(|name| *name == provider.as_str()) + { + return Ok(()); + } + + anyhow::bail!("LLM credentials not configured for provider '{provider}'"); +} + fn format_tool_args(args: &serde_json::Value, cwd: &str) -> String { let cwd_prefix = if cwd.ends_with('/') { cwd.to_string() @@ -403,7 +442,27 @@ pub async fn run_with_args( args: AgentArgs, mcp_servers: Vec, ) -> anyhow::Result<()> { - run_with_args_and_client(args, None, mcp_servers).await + let llm_source = standalone_llm_source()?; + run_with_args_and_source(args, llm_source, mcp_servers).await +} + +#[allow( + clippy::print_stdout, + clippy::print_stderr, + reason = "Assistant output stays on stdout while prompts and diagnostics use stderr." +)] +pub async fn run_with_args_and_source( + args: AgentArgs, + llm_source: Arc, + mcp_servers: Vec, +) -> anyhow::Result<()> { + let provider = parse_provider(&args)?; + let client = Client::from_source(llm_source.as_ref()) + .await + .map(|client| (*client).clone()) + .map_err(|e| anyhow::anyhow!("Failed to create LLM client: {e}"))?; + ensure_provider_registered(&client, provider)?; + run_with_args_and_client(args, client, mcp_servers).await } #[allow( @@ -413,33 +472,15 @@ pub async fn run_with_args( )] pub async fn run_with_args_and_client( args: AgentArgs, - llm_client: Option, + mut client: Client, mcp_servers: Vec, ) -> anyhow::Result<()> { // Resolve color support once, leak to get 'static lifetime for use across // threads let styles: &'static Styles = Box::leak(Box::new(Styles::detect_stderr())); - // Parse provider string to enum early for compile-time safety - let provider_str = args.provider.as_deref().unwrap_or("anthropic"); - let provider: Provider = provider_str - .parse() - .map_err(|_| anyhow::anyhow!("unknown provider: {provider_str}"))?; - - // Build LLM client — use provided client or create from env - let mut client = if let Some(c) = llm_client { - c - } else { - // Validate provider API key only in standalone mode - if !provider.has_api_key() { - anyhow::bail!("API key not set for provider '{provider}'"); - } - let source = EnvCredentialSource::new(); - Client::from_source(&source) - .await - .map(|client| (*client).clone()) - .map_err(|e| anyhow::anyhow!("Failed to create LLM client: {e}"))? - }; + let provider = parse_provider(&args)?; + ensure_provider_registered(&client, provider)?; if args.verbose { client.add_middleware(Arc::new(VerboseMiddleware { styles })); @@ -461,7 +502,11 @@ pub async fn run_with_args_and_client( })? }; eprintln!("{}", styles.dim.apply_to(format!("Using model: {model}"))); - let mut profile = build_profile(provider, &model, Some(client.clone())); + let mut profile = build_profile( + provider, + &model, + Some(build_summarizer(provider, client.clone())), + ); // Build sandbox let cwd = std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")); @@ -497,7 +542,7 @@ pub async fn run_with_args_and_client( let factory_env = Arc::clone(&env); let factory_hooks = config.tool_hooks.clone(); let factory: SessionFactory = Arc::new(move || { - let child_summarizer = build_summarizer(provider, Some(factory_client.clone())); + let child_summarizer = Some(build_summarizer(provider, factory_client.clone())); let child_profile: Arc = match provider { Provider::OpenAi => Arc::new(OpenAiProfile::with_summarizer( &factory_model, diff --git a/lib/crates/fabro-cli/src/commands/exec.rs b/lib/crates/fabro-cli/src/commands/exec.rs index c8b218daf..84ddb0b61 100644 --- a/lib/crates/fabro-cli/src/commands/exec.rs +++ b/lib/crates/fabro-cli/src/commands/exec.rs @@ -2,7 +2,7 @@ use std::collections::HashMap; use std::sync::Arc; use anyhow::Result as AnyResult; -use fabro_agent::cli::{OutputFormat, run_with_args, run_with_args_and_client}; +use fabro_agent::cli::{OutputFormat, run_with_args_and_client, run_with_args_and_source}; use fabro_llm::client::Client; use fabro_llm::error::{ Error as LlmError, ProviderErrorDetail, ProviderErrorKind, error_from_status_code, @@ -447,12 +447,13 @@ pub(crate) async fn execute(mut args: ExecArgs, ctx: &CommandContext) -> AnyResu .register_provider(adapter) .await .map_err(|e| anyhow::anyhow!("Failed to register fabro server adapter: {e}"))?; - run_with_args_and_client(args.agent, Some(client), mcp_servers) + run_with_args_and_client(args.agent, client, mcp_servers) .await .map_err(classify_server_agent_auth)?; } else { tracing::info!(transport = "direct", "Agent session starting"); - run_with_args(args.agent, mcp_servers).await?; + let llm_source = ctx.llm_source().await?; + run_with_args_and_source(args.agent, llm_source, mcp_servers).await?; } Ok(()) diff --git a/lib/crates/fabro-cli/tests/it/cmd/exec.rs b/lib/crates/fabro-cli/tests/it/cmd/exec.rs index 0bba4e677..87a2fbce5 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/exec.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/exec.rs @@ -103,7 +103,7 @@ fn exec_missing_api_key_exits_with_error() { exit_code: 1 ----- stdout ----- ----- stderr ----- - error: API key not set for provider 'anthropic' + error: LLM credentials not configured for provider 'anthropic' "); } @@ -129,7 +129,7 @@ fn exec_uses_user_config_defaults() { exit_code: 1 ----- stdout ----- ----- stderr ----- - error: API key not set for provider 'openai' + error: LLM credentials not configured for provider 'openai' "); } @@ -205,8 +205,8 @@ fn exec_configured_server_target_alone_does_not_reroute_exec() { let output = cmd.assert().failure().get_output().clone(); let stderr = String::from_utf8(output.stderr).expect("valid utf8"); assert!( - stderr.contains("API key not set for provider 'openai'"), - "expected local API key validation failure, got: {stderr}" + stderr.contains("LLM credentials not configured for provider 'openai'"), + "expected local credential resolution failure, got: {stderr}" ); assert!( !stderr.contains("config-should-not-be-used"), diff --git a/lib/crates/fabro-llm/Cargo.toml b/lib/crates/fabro-llm/Cargo.toml index 648b1880b..6f2064d1c 100644 --- a/lib/crates/fabro-llm/Cargo.toml +++ b/lib/crates/fabro-llm/Cargo.toml @@ -42,6 +42,7 @@ http = "1" insta = { workspace = true } tokio = { workspace = true, features = ["test-util", "macros"] } httpmock = "0.8" +trybuild = "1" serde_json.workspace = true fabro-macros = { path = "../fabro-macros" } fabro-test = { workspace = true } diff --git a/lib/crates/fabro-llm/tests/compile_fail.rs b/lib/crates/fabro-llm/tests/compile_fail.rs new file mode 100644 index 000000000..d7273b80a --- /dev/null +++ b/lib/crates/fabro-llm/tests/compile_fail.rs @@ -0,0 +1,5 @@ +#[test] +fn generate_params_requires_client() { + let cases = trybuild::TestCases::new(); + cases.compile_fail("tests/ui/generate_params_requires_client.rs"); +} diff --git a/lib/crates/fabro-llm/tests/ui/generate_params_requires_client.rs b/lib/crates/fabro-llm/tests/ui/generate_params_requires_client.rs new file mode 100644 index 000000000..1f5002505 --- /dev/null +++ b/lib/crates/fabro-llm/tests/ui/generate_params_requires_client.rs @@ -0,0 +1,5 @@ +use fabro_llm::generate::GenerateParams; + +fn main() { + let _ = GenerateParams::new("claude-sonnet-4-5"); +} diff --git a/lib/crates/fabro-llm/tests/ui/generate_params_requires_client.stderr b/lib/crates/fabro-llm/tests/ui/generate_params_requires_client.stderr new file mode 100644 index 000000000..04d9963ed --- /dev/null +++ b/lib/crates/fabro-llm/tests/ui/generate_params_requires_client.stderr @@ -0,0 +1,15 @@ +error[E0061]: this function takes 2 arguments but 1 argument was supplied + --> tests/ui/generate_params_requires_client.rs:4:13 + | +4 | let _ = GenerateParams::new("claude-sonnet-4-5"); + | ^^^^^^^^^^^^^^^^^^^--------------------- argument #2 of type `Arc` is missing + | +note: associated function defined here + --> src/generate.rs + | + | pub fn new(model: impl Into, client: Arc) -> Self { + | ^^^ +help: provide the argument + | +4 | let _ = GenerateParams::new("claude-sonnet-4-5", /* Arc */); + | +++++++++++++++++++ diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 416df3085..221803332 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -552,17 +552,17 @@ pub struct AppState { /// proceed in parallel. See `crate::run_files` for semantics. pub(crate) files_in_flight: FilesInFlight, - pub(crate) vault: Arc>, - pub(super) server_secrets: ServerSecrets, - pub(crate) llm_source: Arc, - pub(crate) settings: Arc>, - pub(crate) server_settings: RwLock>, - pub(crate) env_lookup: EnvLookup, - http_client: Option, - shutting_down: AtomicBool, - registry_factory_override: Option>, - slack_service: Option>, - slack_started: AtomicBool, + pub(crate) vault: Arc>, + pub(super) server_secrets: ServerSecrets, + pub(crate) llm_source: Arc, + pub(crate) settings: Arc>, + pub(crate) server_settings: RwLock>, + pub(crate) env_lookup: EnvLookup, + http_client: Option, + shutting_down: AtomicBool, + registry_factory_override: Option>, + slack_service: Option>, + slack_started: AtomicBool, } pub(crate) struct AppStateConfig { @@ -2514,9 +2514,9 @@ pub(crate) fn build_app_state(config: AppStateConfig) -> anyhow::Result = Arc::new(VaultCredentialSource::with_env_lookup( Arc::clone(&vault), { - let env_lookup = Arc::clone(&env_lookup); - move |name| env_lookup(name) - }, + let env_lookup = Arc::clone(&env_lookup); + move |name| env_lookup(name) + }, )); let (global_event_tx, _) = broadcast::channel(4096); let current_server_settings = { @@ -7173,13 +7173,17 @@ mod tests { use axum::body::Body; use axum::http::{Method, Request, header}; use chrono::{Duration as ChronoDuration, Utc}; + use fabro_auth::{AuthCredential, AuthDetails}; use fabro_config::bind::Bind; use fabro_interview::{AnswerValue, ControlInterviewer, Interviewer, Question, QuestionType}; + use fabro_llm::types::{Message as LlmMessage, Request as LlmRequest}; use fabro_model::Provider; use fabro_types::settings::ServerAuthMethod; use fabro_types::{ InterviewQuestionRecord, InterviewQuestionType, RunAuthMethod, RunBlobId, RunId, fixtures, }; + use httpmock::Method::POST; + use httpmock::MockServer; use serde_json::json; use tokio_stream::StreamExt as _; use tower::ServiceExt; @@ -7217,6 +7221,39 @@ mod tests { serde_json::from_slice(&bytes).unwrap() } + fn openai_api_key_credential(key: &str) -> AuthCredential { + AuthCredential { + provider: Provider::OpenAi, + details: AuthDetails::ApiKey { + key: key.to_string(), + }, + } + } + + fn openai_responses_payload(text: &str) -> serde_json::Value { + json!({ + "id": "resp_1", + "model": "gpt-5.4", + "output": [ + { + "type": "message", + "role": "assistant", + "content": [ + { + "type": "output_text", + "text": text + } + ] + } + ], + "status": "completed", + "usage": { + "input_tokens": 10, + "output_tokens": 20 + } + }) + } + macro_rules! assert_status { ($response:expr, $expected:expr) => { fabro_test::assert_axum_status($response, $expected, concat!(file!(), ":", line!())) @@ -7625,6 +7662,106 @@ root = "/srv/new" assert!(state.vault.read().await.get("openai_codex").is_some()); } + #[tokio::test] + async fn resolve_llm_client_reads_openai_codex_credential_from_vault() { + let state = create_app_state_with_env_lookup(SettingsLayer::default(), 5, |_| None); + state + .vault + .write() + .await + .set( + "openai_codex", + &serde_json::to_string(&openai_api_key_credential("vault-openai-key")).unwrap(), + SecretType::Credential, + None, + ) + .unwrap(); + + let llm_result = state.resolve_llm_client().await.unwrap(); + + assert_eq!(llm_result.client.provider_names(), vec!["openai"]); + assert!(llm_result.auth_issues.is_empty()); + } + + #[tokio::test] + async fn llm_source_configured_providers_reads_openai_codex_from_vault() { + let state = create_app_state_with_env_lookup(SettingsLayer::default(), 5, |_| None); + state + .vault + .write() + .await + .set( + "openai_codex", + &serde_json::to_string(&openai_api_key_credential("vault-openai-key")).unwrap(), + SecretType::Credential, + None, + ) + .unwrap(); + + assert_eq!(state.llm_source.configured_providers().await, vec![ + Provider::OpenAi + ]); + } + + #[tokio::test] + async fn resolve_llm_client_uses_env_lookup_for_openai_settings() { + let server = MockServer::start_async().await; + let response_mock = server + .mock_async(|when, then| { + when.method(POST) + .path("/v1/responses") + .header("authorization", "Bearer vault-openai-key") + .header("OpenAI-Organization", "env-org"); + then.status(200) + .header("content-type", "application/json") + .json_body(openai_responses_payload("hello from env lookup")); + }) + .await; + let base_url = server.url("/v1"); + let state = + create_app_state_with_env_lookup(SettingsLayer::default(), 5, move |name| match name { + "OPENAI_BASE_URL" => Some(base_url.clone()), + "OPENAI_ORG_ID" => Some("env-org".to_string()), + _ => None, + }); + state + .vault + .write() + .await + .set( + "openai_codex", + &serde_json::to_string(&openai_api_key_credential("vault-openai-key")).unwrap(), + SecretType::Credential, + None, + ) + .unwrap(); + + let llm_result = state.resolve_llm_client().await.unwrap(); + let response = llm_result + .client + .complete(&LlmRequest { + model: "gpt-5.4".to_string(), + messages: vec![LlmMessage::user("Hello")], + provider: Some("openai".to_string()), + tools: None, + tool_choice: None, + response_format: None, + temperature: None, + top_p: None, + max_tokens: None, + stop_sequences: None, + reasoning_effort: None, + speed: None, + metadata: None, + provider_options: None, + }) + .await + .unwrap(); + + assert_eq!(response.text(), "hello from env lookup"); + response_mock.assert_async().await; + } + #[tokio::test] async fn list_secrets_includes_credential_metadata() { let state = create_app_state(); diff --git a/lib/crates/fabro-workflow/Cargo.toml b/lib/crates/fabro-workflow/Cargo.toml index 1611a7bdd..b04d92e8d 100644 --- a/lib/crates/fabro-workflow/Cargo.toml +++ b/lib/crates/fabro-workflow/Cargo.toml @@ -73,6 +73,7 @@ tokio = { workspace = true, features = ["test-util", "macros"] } object_store.workspace = true assert_cmd = "2" predicates = "3" +httpmock = "0.8" fabro-macros = { path = "../fabro-macros" } fabro-test = { workspace = true } fabro-types = { path = "../fabro-types", features = ["test-support"] } diff --git a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs index fbfa29b51..c35b8a38b 100644 --- a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs +++ b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs @@ -530,8 +530,8 @@ pub async fn pull_request(concluded: Concluded, options: &PullRequestOptions) -> } else if let Ok(ref result) = outcome { if matches!( result.status, - StageStatus::Success | StageStatus::PartialSuccess - ) { + StageStatus::Success | StageStatus::PartialSuccess + ) { let diff = load_pull_request_diff(&services.run_store).await; if let (Some(base_branch), Some(run_branch), Some(creds), Some(origin)) = ( &run_options.base_branch, @@ -610,20 +610,26 @@ mod tests { use std::time::Duration; use chrono::Utc; - use fabro_auth::{CredentialSource, EnvCredentialSource}; + use fabro_auth::{ + AuthCredential, AuthDetails, CredentialSource, EnvCredentialSource, VaultCredentialSource, + }; use fabro_graphviz::graph::Graph; + use fabro_llm::Error as LlmError; use fabro_llm::client::Client; use fabro_llm::provider::{ProviderAdapter, StreamEventStream}; use fabro_llm::types::{FinishReason, Message, Request, Response, StreamEvent, TokenCounts}; - use fabro_llm::Error as LlmError; use fabro_retro::retro::{ AggregateStats, FrictionKind, FrictionPoint, OpenItem, OpenItemKind, StageRetro, }; use fabro_store::Database; use fabro_types::settings::SettingsLayer; use fabro_types::{BilledTokenCounts, RunSpec, SuccessReason, fixtures}; + use fabro_vault::{SecretType, Vault}; use futures::stream; + use httpmock::Method::POST; + use httpmock::MockServer; use object_store::memory::InMemory; + use tokio::sync::RwLock as AsyncRwLock; use super::*; use crate::event::{Event, append_event}; @@ -725,6 +731,39 @@ mod tests { Arc::new(EnvCredentialSource::new()) } + fn openai_api_key_credential(key: &str) -> AuthCredential { + AuthCredential { + provider: fabro_model::Provider::OpenAi, + details: AuthDetails::ApiKey { + key: key.to_string(), + }, + } + } + + fn openai_responses_payload(text: &str) -> serde_json::Value { + serde_json::json!({ + "id": "resp_1", + "model": "gpt-5.4", + "output": [ + { + "type": "message", + "role": "assistant", + "content": [ + { + "type": "output_text", + "text": text + } + ] + } + ], + "status": "completed", + "usage": { + "input_tokens": 10, + "output_tokens": 20 + } + }) + } + fn make_test_conclusion() -> Conclusion { Conclusion { timestamp: Utc::now(), @@ -1267,6 +1306,58 @@ mod tests { assert!(!body.contains("Narrative from mock.")); } + #[tokio::test] + async fn build_pr_body_uses_vault_only_openai_codex_source() { + let server = MockServer::start_async().await; + let response_mock = server + .mock_async(|when, then| { + when.method(POST) + .path("/v1/responses") + .header("authorization", "Bearer vault-openai-key"); + then.status(200) + .header("content-type", "application/json") + .json_body(openai_responses_payload("Narrative from vault source.")); + }) + .await; + + let dir = tempfile::tempdir().unwrap(); + let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); + vault + .set( + "openai_codex", + &serde_json::to_string(&openai_api_key_credential("vault-openai-key")).unwrap(), + SecretType::Credential, + None, + ) + .unwrap(); + let base_url = server.url("/v1"); + let llm_source: Arc = + Arc::new(VaultCredentialSource::with_env_lookup( + Arc::new(AsyncRwLock::new(vault)), + move |name| match name { + "OPENAI_BASE_URL" => Some(base_url.clone()), + _ => None, + }, + )); + + let store = test_store(); + let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); + let services = RunServices::for_cli(run_store.into(), llm_source); + + let body = build_pr_body( + "diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n", + "Implement feature", + "gpt-5.4", + services.as_ref(), + Some(&make_test_conclusion()), + ) + .await + .unwrap(); + + assert!(body.contains("Narrative from vault source.")); + response_mock.assert_async().await; + } + // ── parse_dot_summary tests ───────────────────────────────────────── #[test] diff --git a/lib/crates/fabro-workflow/src/services.rs b/lib/crates/fabro-workflow/src/services.rs index d074b960e..1751212ad 100644 --- a/lib/crates/fabro-workflow/src/services.rs +++ b/lib/crates/fabro-workflow/src/services.rs @@ -7,7 +7,7 @@ use std::time::Duration; use fabro_agent::Sandbox; use fabro_auth::CredentialSource; #[cfg(test)] -use fabro_auth::EnvCredentialSource; +use fabro_auth::ResolvedCredentials; use fabro_hooks::{HookContext, HookDecision, HookRunner}; use fabro_model::Provider; #[cfg(test)] @@ -25,16 +25,35 @@ use crate::runtime_store::RunStoreHandle; use crate::sandbox_git::GitState; use crate::workflow_bundle::WorkflowBundle; +#[cfg(test)] +#[derive(Debug, Default)] +struct StubCredentialSource; + +#[cfg(test)] +#[async_trait::async_trait] +impl CredentialSource for StubCredentialSource { + async fn resolve(&self) -> anyhow::Result { + Ok(ResolvedCredentials { + credentials: Vec::new(), + auth_issues: Vec::new(), + }) + } + + async fn configured_providers(&self) -> Vec { + Vec::new() + } +} + /// Services shared across workflow phases. #[derive(Clone)] pub struct RunServices { - pub run_store: RunStoreHandle, - pub emitter: Arc, - pub sandbox: Arc, - pub hook_runner: Option>, - pub cancel_requested: Option>, - pub provider: Provider, - pub llm_source: Arc, + pub run_store: RunStoreHandle, + pub emitter: Arc, + pub sandbox: Arc, + pub hook_runner: Option>, + pub cancel_requested: Option>, + pub provider: Provider, + pub llm_source: Arc, } impl RunServices { @@ -71,16 +90,15 @@ impl RunServices { let Some(ref runner) = self.hook_runner else { return HookDecision::Proceed; }; - runner.run(hook_context, Arc::clone(&self.sandbox), None).await + runner + .run(hook_context, Arc::clone(&self.sandbox), None) + .await } /// CLI helper: minimal cross-phase services for PR generation and similar /// source-backed operations outside the workflow executor. #[must_use] - pub fn for_cli( - run_store: RunStoreHandle, - llm_source: Arc, - ) -> Arc { + pub fn for_cli(run_store: RunStoreHandle, llm_source: Arc) -> Arc { Self::new( run_store, Arc::new(Emitter::default()), @@ -129,7 +147,7 @@ impl RunServices { }) } - /// Test-only default: local sandbox at cwd, empty run store, env source. + /// Test-only default: local sandbox at cwd, empty run store, stub source. #[cfg(test)] #[expect( clippy::disallowed_methods, @@ -165,30 +183,30 @@ impl RunServices { None, None, Provider::Anthropic, - Arc::new(EnvCredentialSource::new()), + Arc::new(StubCredentialSource), ) } } /// Services available only while executing workflow nodes. pub struct EngineServices { - pub run: Arc, - pub registry: Arc, + pub run: Arc, + pub registry: Arc, /// Git state for the current run. Set via `set_git_state` at the start of /// `execute` and read by parallel/fan-in handlers. pub(crate) git_state: std::sync::RwLock>>, /// Environment variables from `[sandbox.env]` config, injected into command /// nodes. - pub env: HashMap, + pub env: HashMap, /// Typed values from `[run.inputs]`, available to prompt templates. - pub inputs: HashMap, + pub inputs: HashMap, /// When true, handlers should skip real execution and return simulated /// results. - pub dry_run: bool, + pub dry_run: bool, /// Logical path of the current workflow when running from a bundle. - pub workflow_path: Option, + pub workflow_path: Option, /// Bundled workflows available for child-workflow resolution. - pub workflow_bundle: Option>, + pub workflow_bundle: Option>, } impl EngineServices { @@ -245,3 +263,15 @@ pub(crate) fn sandbox_cancel_token( Some(token) } + +#[cfg(test)] +mod tests { + use super::RunServices; + + #[tokio::test] + async fn for_test_uses_stub_credential_source() { + let services = RunServices::for_test(); + + assert!(services.llm_source.configured_providers().await.is_empty()); + } +} diff --git a/lib/crates/fabro-workflow/src/test_support.rs b/lib/crates/fabro-workflow/src/test_support.rs index 092178343..85c50fc02 100644 --- a/lib/crates/fabro-workflow/src/test_support.rs +++ b/lib/crates/fabro-workflow/src/test_support.rs @@ -5,7 +5,7 @@ use std::sync::Arc; use std::time::Duration; use fabro_agent::Sandbox; -use fabro_auth::EnvCredentialSource; +use fabro_auth::{CredentialSource, EnvCredentialSource}; use fabro_graphviz::graph::Graph as GvGraph; use fabro_store::{ArtifactStore, Database, RunProjection}; use object_store::local::LocalFileSystem; @@ -34,6 +34,7 @@ struct InitializedOptions { hook_runner: Option>, env: HashMap, checkpoint: Option, + llm_source: Option>, } struct InitializedState { @@ -115,38 +116,40 @@ async fn initialized( ); InitializedState { initialized: Initialized { - graph: graph.clone(), - source: String::new(), - run_options: run_options.clone(), - checkpoint: options.checkpoint, - seed_context: None, - on_node: None, + graph: graph.clone(), + source: String::new(), + run_options: run_options.clone(), + checkpoint: options.checkpoint, + seed_context: None, + on_node: None, artifact_sink: Some(ArtifactSink::Store(artifact_store)), - run_control: None, - engine: Arc::new(EngineServices { - run: RunServices::new( + run_control: None, + engine: Arc::new(EngineServices { + run: RunServices::new( run_store.into(), emitter, sandbox, options.hook_runner, run_options.cancel_token.clone(), fabro_llm::Provider::Anthropic, - Arc::new(EnvCredentialSource::new()), + options + .llm_source + .unwrap_or_else(|| Arc::new(EnvCredentialSource::new())), ), - registry: Arc::new(registry), - git_state: std::sync::RwLock::new(None), - env: options.env, - inputs: run_options + registry: Arc::new(registry), + git_state: std::sync::RwLock::new(None), + env: options.env, + inputs: run_options .settings .run .as_ref() .and_then(|run| run.inputs.clone()) .unwrap_or_default(), - dry_run: run_options.dry_run_enabled(), - workflow_path: None, + dry_run: run_options.dry_run_enabled(), + workflow_path: None, workflow_bundle: None, }), - model: String::new(), + model: String::new(), }, store_logger, } @@ -169,6 +172,7 @@ pub async fn run_graph( hook_runner: None, env: HashMap::new(), checkpoint: None, + llm_source: None, }, ) .await; @@ -196,6 +200,7 @@ pub async fn run_graph_with_state( hook_runner: None, env: HashMap::new(), checkpoint: None, + llm_source: None, }, ) .await; @@ -231,6 +236,7 @@ pub async fn run_graph_with_hooks( hook_runner: Some(hook_runner), env: env.unwrap_or_default(), checkpoint: None, + llm_source: None, }, ) .await; @@ -258,6 +264,7 @@ pub async fn run_graph_with_hooks_and_state( hook_runner: Some(hook_runner), env: env.unwrap_or_default(), checkpoint: None, + llm_source: None, }, ) .await; @@ -292,6 +299,7 @@ pub async fn run_graph_from_checkpoint( hook_runner: None, env: HashMap::new(), checkpoint: Some(checkpoint.clone()), + llm_source: None, }, ) .await; @@ -318,6 +326,42 @@ pub async fn run_graph_from_checkpoint_with_state( hook_runner: None, env: HashMap::new(), checkpoint: Some(checkpoint.clone()), + llm_source: None, + }, + ) + .await; + let executed = pipeline::execute(initialized.initialized).await; + initialized.store_logger.flush().await; + let outcome = executed.outcome?; + let state = executed + .engine + .run + .run_store + .state() + .await + .map_err(|err| Error::engine(err.to_string()))?; + Ok((outcome, state)) +} + +pub async fn run_graph_with_state_and_llm_source( + registry: HandlerRegistry, + emitter: Arc, + sandbox: Arc, + graph: &GvGraph, + run_options: &RunOptions, + llm_source: Arc, +) -> Result<(Outcome, RunProjection)> { + let initialized = initialized( + registry, + emitter, + sandbox, + graph, + run_options, + InitializedOptions { + hook_runner: None, + env: HashMap::new(), + checkpoint: None, + llm_source: Some(llm_source), }, ) .await; @@ -392,6 +436,29 @@ impl WorkflowRunner { .await } + pub async fn run_with_state_and_llm_source( + &self, + graph: &GvGraph, + run_options: &RunOptions, + llm_source: Arc, + ) -> Result<(Outcome, RunProjection)> { + let registry = self + .registry + .lock() + .unwrap() + .take() + .expect("WorkflowRunner may only be used once"); + Box::pin(run_graph_with_state_and_llm_source( + registry, + Arc::clone(&self.emitter), + Arc::clone(&self.sandbox), + graph, + run_options, + llm_source, + )) + .await + } + pub async fn run_from_checkpoint( &self, graph: &GvGraph, diff --git a/lib/crates/fabro-workflow/tests/it/integration.rs b/lib/crates/fabro-workflow/tests/it/integration.rs index 16ea4e69e..781e1e70d 100644 --- a/lib/crates/fabro-workflow/tests/it/integration.rs +++ b/lib/crates/fabro-workflow/tests/it/integration.rs @@ -6605,10 +6605,164 @@ mod real_llm { } } +fn openai_api_key_credential(key: &str) -> fabro_auth::AuthCredential { + fabro_auth::AuthCredential { + provider: fabro_model::Provider::OpenAi, + details: fabro_auth::AuthDetails::ApiKey { + key: key.to_string(), + }, + } +} + +fn openai_responses_payload(text: &str) -> serde_json::Value { + serde_json::json!({ + "id": "resp_1", + "model": "gpt-5.4", + "output": [ + { + "type": "message", + "role": "assistant", + "content": [ + { + "type": "output_text", + "text": text + } + ] + } + ], + "status": "completed", + "usage": { + "input_tokens": 10, + "output_tokens": 20 + } + }) +} + // --------------------------------------------------------------------------- // Wait.human freeform edge integration tests (Section 4.6) // --------------------------------------------------------------------------- +#[tokio::test] +async fn workflow_run_with_vault_only_openai_codex_builds_pr_body() { + use chrono::Utc; + use fabro_auth::{CredentialSource, VaultCredentialSource}; + use fabro_types::Conclusion; + use fabro_vault::{SecretType, Vault}; + use httpmock::Method::POST; + use httpmock::MockServer; + use tokio::sync::RwLock as AsyncRwLock; + + let server = MockServer::start_async().await; + let response_mock = server + .mock_async(|when, then| { + when.method(POST) + .path("/v1/responses") + .header("authorization", "Bearer vault-openai-key"); + then.status(200) + .header("content-type", "application/json") + .json_body(openai_responses_payload("Narrative from vault source.")); + }) + .await; + + let mut graph = Graph::new("VaultOpenAiCodexPrBody"); + graph.attrs.insert( + "goal".to_string(), + AttrValue::String("Verify PR body generation uses vault credentials".to_string()), + ); + + let mut start = Node::new("start"); + start.attrs.insert( + "shape".to_string(), + AttrValue::String("Mdiamond".to_string()), + ); + graph.nodes.insert("start".to_string(), start); + + let mut exit = Node::new("exit"); + exit.attrs.insert( + "shape".to_string(), + AttrValue::String("Msquare".to_string()), + ); + graph.nodes.insert("exit".to_string(), exit); + + graph.edges.push(Edge::new("start", "exit")); + + let vault_dir = tempfile::tempdir().unwrap(); + let mut vault = Vault::load(vault_dir.path().join("secrets.json")).unwrap(); + vault + .set( + "openai_codex", + &serde_json::to_string(&openai_api_key_credential("vault-openai-key")).unwrap(), + SecretType::Credential, + None, + ) + .unwrap(); + let base_url = server.url("/v1"); + let llm_source: Arc = Arc::new(VaultCredentialSource::with_env_lookup( + Arc::new(AsyncRwLock::new(vault)), + move |name| match name { + "OPENAI_BASE_URL" => Some(base_url.clone()), + _ => None, + }, + )); + + let dir = tempfile::tempdir().unwrap(); + let mut registry = HandlerRegistry::new(Box::new(StartHandler)); + registry.register("start", Box::new(StartHandler)); + registry.register("exit", Box::new(ExitHandler)); + + let engine = WorkflowRunner::new(registry, Arc::new(Emitter::default()), local_env()); + let run_options = RunOptions { + settings: SettingsLayer::default(), + run_dir: dir.path().to_path_buf(), + cancel_token: None, + run_id: test_run_id("vault-only-openai-codex-pr-body"), + labels: std::collections::HashMap::new(), + workflow_slug: None, + github_app: None, + base_branch: None, + display_base_sha: None, + host_repo_path: None, + git: None, + }; + let (outcome, _) = engine + .run_with_state_and_llm_source(&graph, &run_options, Arc::clone(&llm_source)) + .await + .expect("workflow run should succeed"); + assert_eq!(outcome.status, StageStatus::Success); + + let store_dir = test_store_dir(&run_options.run_dir); + let store = Arc::new(Database::new( + Arc::new(LocalFileSystem::new_with_prefix(&store_dir).unwrap()), + "", + Duration::from_millis(1), + None, + )); + let run_store = store.open_run_reader(&run_options.run_id).await.unwrap(); + let services = fabro_workflow::services::RunServices::for_cli(run_store.into(), llm_source); + + let body = fabro_workflow::pull_request::build_pr_body( + "diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n", + "Implement feature", + "gpt-5.4", + services.as_ref(), + Some(&Conclusion { + timestamp: Utc::now(), + status: StageStatus::Success, + duration_ms: 1, + failure_reason: None, + final_git_commit_sha: None, + stages: Vec::new(), + billing: None, + total_retries: 0, + }), + ) + .await + .expect("PR body should build from vault-only credentials"); + + assert!(body.contains("Narrative from vault source.")); + response_mock.assert_async().await; +} + /// Freeform-only human gate: free-text input routes through the freeform edge /// and stores the text in human.gate.text context variable. #[tokio::test]