diff --git a/Cargo.lock b/Cargo.lock index 0cc3f4846..d142e8df9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1512,6 +1512,7 @@ dependencies = [ "chrono", "clap", "dirs", + "fabro-auth", "fabro-config", "fabro-http", "fabro-llm", @@ -1522,6 +1523,7 @@ dependencies = [ "fabro-test", "fabro-types", "fabro-util", + "fabro-vault", "futures", "glob", "htmd", @@ -1570,6 +1572,7 @@ dependencies = [ "fabro-http", "fabro-model", "fabro-oauth", + "fabro-util", "fabro-vault", "httpmock", "serde", @@ -1805,6 +1808,7 @@ version = "0.213.0-nightly.0" dependencies = [ "async-trait", "fabro-agent", + "fabro-auth", "fabro-config", "fabro-http", "fabro-llm", @@ -2305,6 +2309,7 @@ dependencies = [ "futures", "git2", "hex", + "httpmock", "md5", "mime_guess", "object_store", diff --git a/docs-internal/llm-client-resolution.md b/docs-internal/llm-client-resolution.md new file mode 100644 index 000000000..22f58d6cd --- /dev/null +++ b/docs-internal/llm-client-resolution.md @@ -0,0 +1,32 @@ +# LLM Client Resolution + +This document defines how Fabro resolves LLM credentials and constructs `fabro-llm` clients. + +## Core Rules + +- `fabro_auth::CredentialSource` is the credential authority. +- Long-lived runtime contexts store `Arc`, not `Client`. +- Call `fabro_llm::client::Client::from_source(&source).await?` at the point of use. +- `GenerateParams::new(model, client)` always receives an explicit `Arc`. +- When a caller needs diagnostics, call `source.resolve()` directly and consume both `credentials` and `auth_issues`. +- `EnvCredentialSource` is the env-backed source for env-only or no-vault contexts. +- `VaultCredentialSource` is the normal source for vault-backed runtime contexts. + +## Why + +- Rebuilding a client from the source at point of use preserves OAuth refresh behavior on long-running processes. +- Holding the source on contexts avoids process-global installs and cross-context leakage. +- Requiring an explicit client on `GenerateParams` makes the old silent fallback bug unrepresentable. + +## Application + +- Workflow state lives on `RunServices.llm_source`. +- Server state lives on `AppState.llm_source`. +- Hooks and other long-lived executors receive a source and derive clients when they actually generate. +- One-shot CLI commands may resolve a source locally, then derive a client once for that operation. + +## Enforcement + +- Do not add new `Client::from_env`-style shortcuts in production paths. +- Do not cache a long-lived `Client` where OAuth refresh or storage-dir rebinding matters. +- Mirror [server-secrets-strategy.md](/Users/bhelmkamp/p/fabro-sh/fabro-6/docs-internal/server-secrets-strategy.md): production credential resolution should be explicit about where secrets come from and how they flow into subprocesses. diff --git a/docs/plans/2026-04-23-003-refactor-llm-client-resolution-and-run-services-plan.md b/docs/plans/2026-04-23-003-refactor-llm-client-resolution-and-run-services-plan.md new file mode 100644 index 000000000..83c5c0fbe --- /dev/null +++ b/docs/plans/2026-04-23-003-refactor-llm-client-resolution-and-run-services-plan.md @@ -0,0 +1,569 @@ +--- +title: "refactor: Source-based LLM client resolution + RunServices split" +type: refactor +status: completed +date: 2026-04-23 +deepened: 2026-04-23 +--- + +# refactor: Source-based LLM client resolution + RunServices split + +## Overview + +PR #168 revealed that `fabro-llm::generate::generate()` silently falls back to `Client::from_env()` when no explicit client is passed. Vault-configured credentials (`openai_codex`) get bypassed. PR #168 is a point fix that threads `Option` through the pipeline. This plan replaces that with the correct architecture. + +**Design in one line:** credentials are the long-lived authority; clients are short-lived derivations built at the point of use. + +**Greenfield context.** No production installs, no backwards-compat, no migration. The bar is elegance. + +**Two changes:** + +1. Replace the silent env fallback with `CredentialSource` as the credential authority and `Client::from_source(&source)` as the idiomatic constructor. Every `generate()` requires an explicit `Arc` (compile-time enforcement). Delete `DEFAULT_CLIENT`, `set_default_client`, `get_default_client`, and `Client::from_env`. + +2. Split `EngineServices` into cross-phase `RunServices` (holds `llm_source: Arc`) + execute-only `EngineServices` (holds `Arc` + execute-only state). Phase structs carry `Arc` or `Arc`, not individual service fields. PR #168's `Option` plumbing unwinds. + +**Prerequisite.** PR #168 must merge to main before Phase 2. Phase 1 is independent of PR #168's merge state. + +## Problem Frame + +Two smells, one architectural root: + +- **Silent env fallback.** `generate()` without an explicit client silently uses `Client::from_env()` — env-only, vault-unaware. Every future call site that forgets a client reintroduces the class bug. +- **`Option` threading.** PR #168 plumbed `Option` through phase structs to fix one instance. In production the client is always present; the `Option` is a domain lie. + +**Root cause:** `Client` plays two roles today — a short-lived RPC handle AND the point of credential resolution. When a caller forgets to pass one, the "resolution" role silently falls through to env. Separating the two roles — `CredentialSource` is the long-lived authority, `Client` is always derived from one — makes the class bug unrepresentable. + +## Requirements Trace + +- **R1.** `fabro_llm::generate::generate()` fails to compile when no client is supplied. No runtime fallback of any kind. +- **R2.** Every long-lived runtime context that needs LLM access (`RunServices`, `AppState`, `HookExecutor`) holds `Arc`, not a pinned `Client`. CLI subcommands resolve sources on demand via a `CommandContext::llm_source()` helper, not a held field. +- **R3.** OAuth refresh behavior is preserved — clients get fresh tokens at point of use, not at process start. +- **R4.** `RunServices` (cross-phase) and `EngineServices` (execute-only) have distinct types. Retro/finalize/PR-body code cannot reach execute-only state. +- **R5.** PR #168's production bug stays fixed, covered by a pipeline-level regression test (run with vault-only `openai_codex`, auto-PR body succeeds). +- **R6.** Duplicate `build_llm_client` in `fabro-workflow/handler/llm/api.rs` and `fabro-server/server_secrets.rs` collapses to one implementation. + +## Scope Boundaries + +- Don't rework `Client` internals, provider adapters, `from_credentials`, or any `fabro-llm` provider registration logic. +- Don't redesign vault/credential storage — `VaultCredentialSource` adapts existing `CredentialResolver` logic. +- Don't change phase ordering or per-phase behavior — only where shared state lives. +- Don't touch the `fabro-client` crate extraction (`docs/plans/2026-04-20-002`). +- Don't create `docs/solutions/` (separate opportunity). + +## Context & Research + +### Relevant Code + +**`fabro-llm`:** +- `lib/crates/fabro-llm/src/client.rs` — `Client::new`, `Client::from_env`, `Client::from_credentials`, `provider_names`. `Client` derives `Clone`. +- `lib/crates/fabro-llm/src/generate.rs` — `DEFAULT_CLIENT: OnceCell>`, `set_default_client`, `get_default_client`, `generate`, `stream_with_tool_loop`, `stream_generate`, `generate_object`. `GenerateParams.client: Option>`. + +**`fabro-auth`:** +- `lib/crates/fabro-auth/src/resolve.rs` — `CredentialResolver { vault: Arc>, env_lookup: EnvLookup }`. `resolve(provider, usage)` returns `ResolvedCredential::Api(ApiCredential)` or error. Handles OAuth refresh and writes back to vault. +- `ApiCredential`, `ApiKeyHeader` — already imported by `fabro-llm::client::Client::from_env`. + +**`fabro-workflow`:** +- `lib/crates/fabro-workflow/src/handler/mod.rs` — `EngineServices` (13 fields today). +- `lib/crates/fabro-workflow/src/handler/llm/api.rs` — `build_llm_client(resolver: Option<&CredentialResolver>)`; `AgentApiBackend::create_session_for` and `one_shot` rebuild client per session via `build_llm_client(self.resolver.as_ref()).await?.client` at `:268` and `:349`. **This per-session rebuild is load-bearing for OAuth refresh.** +- `lib/crates/fabro-workflow/src/handler/llm/cli.rs` — parallel `AgentCliBackend` with its own `new_from_env`. +- `lib/crates/fabro-workflow/src/pipeline/types.rs` — phase structs and options. +- `lib/crates/fabro-workflow/src/pipeline/initialize.rs:555-566` — `SandboxReady` hook fires BEFORE `build_registry` at `:587` builds the LLM client. Any design that requires a client at hook time regresses this. +- `lib/crates/fabro-workflow/src/pipeline/{execute,retro,finalize,pull_request}.rs` — phase implementations. +- `lib/crates/fabro-workflow/src/operations/start.rs` — `RunSession`, `StartServices`. `RunSession.vault: Option>>` threads to `InitOptions.vault` at `:714`. + +**Consumers:** +- `lib/crates/fabro-cli/src/main.rs` — CLI entry. +- `lib/crates/fabro-cli/src/command_context.rs:61-72` — `CommandContext::with_target` and `with_connection` re-derive context per subcommand with different `storage_dir_override`. **A process-global source install is not correct here.** +- `lib/crates/fabro-cli/src/commands/pr/create.rs` — loads vault at `:103`, calls `maybe_open_pull_request(..., None)`. +- `lib/crates/fabro-cli/src/shared/provider_auth.rs` — already passes explicit client. +- `lib/crates/fabro-hooks/src/executor.rs` — `execute_prompt` (default client today), `execute_agent` (`Client::from_env()` at `:358`). +- `lib/crates/fabro-agent/src/cli.rs:436` — `Client::from_env()`. +- `lib/crates/fabro-server/src/server.rs:2488` — `build_app_state`; `Vault::load` at `:2500`. +- `lib/crates/fabro-server/src/server_secrets.rs:87-112` — duplicate `ProviderCredentials::build_llm_client`. + +### Institutional Learnings + +- **`docs/plans/2026-04-20-002-refactor-extract-fabro-client-crate-plan.md`** — `CredentialFallback` named-trait precedent. Same rationale applies here: named trait makes the role obvious at the call site. +- **`docs/plans/2026-04-05-server-canonical-secrets-doctor-repo-plan.md:171,485`** — Already flagged `from_env()` as a smell; this plan resolves the deferred follow-up. +- **`docs/plans/2026-04-22-003-refactor-lock-down-server-secrets-plan.md:74-78`** — Credential taxonomy. LLM provider credentials live on the Vault + `ProviderCredentials` track. +- **`docs/plans/2026-04-08-cli-services-command-context-refactor-plan.md:59-61`** and **`docs/plans/2026-04-23-001-refactor-command-context-alignment-plan.md:273-277`** — Repo has twice rejected adding a peer-wrapper alongside an existing context. This plan's `RunServices`/`EngineServices` split is **composition** (`EngineServices` contains `Arc`), not a peer wrapper — and is about the pipeline (`fabro-workflow`), not CLI (`fabro-cli`). The rejections do not apply. + +## Key Technical Decisions + +### 1. Credentials are the authority; clients are derived + +`CredentialSource` trait lives in `fabro-auth`: + +```rust +pub struct ResolvedCredentials { + pub credentials: Vec, + pub auth_issues: Vec<(Provider, ResolveError)>, +} + +pub trait CredentialSource: Send + Sync { + /// Full resolution with OAuth refresh. Expensive. Returns credentials + auth issues. + async fn resolve(&self) -> Result; + + /// Cheap preflight: which providers have credentials configured at all? + /// No OAuth refresh. Used for model selection and manifest building. + async fn configured_providers(&self) -> Vec; +} +``` + +The richer return type preserves today's partial-resolution diagnostics. `auth_issues` exists because `CredentialResolver::resolve` can fail for one provider (e.g. "OpenAI OAuth refresh failed") while another succeeds — callers produce user-facing messages like "OpenAI requires re-authentication; Anthropic is not configured." This is load-bearing at four sites: `pipeline/initialize.rs:301-318` (error message on "no usable providers"), `server/src/server.rs:6652` (diagnostics endpoint), `server/src/server.rs:6821-6833` (`create_completion` logs `warn!` per issue), `server/src/diagnostics.rs:90`. + +Natural home: `fabro-auth` already owns `ApiCredential`, `CredentialResolver`, and `ResolveError`. No new dep cycles. `fabro-llm` exposes `Client::from_source(&dyn CredentialSource) -> Result, Error>` — a convenience that discards `auth_issues` and returns only the Client. Callers that need diagnostics call `source.resolve()` directly and inspect both halves. + +**Implementors:** +- `VaultCredentialSource` in `fabro-auth` — wraps `CredentialResolver`. Iterates `Provider::ALL` calling `resolver.resolve(provider, CredentialUsage::ApiRequest)`, collecting credentials and auth issues. Two constructors: + - `VaultCredentialSource::new(Arc>)` — default env lookup (used by workflow path). + - `VaultCredentialSource::with_env_lookup(Arc>, env_lookup)` — server uses this with its own env policy (preserves today's `ProviderCredentials::with_env_lookup` behavior at `server_secrets.rs:67`). Replaces `build_llm_client` in both `fabro-workflow/handler/llm/api.rs` and `fabro-server/server_secrets.rs`. +- `EnvCredentialSource` in `fabro-auth` — reads env vars for each provider, emits `ApiCredential`s. Replaces `Client::from_env`. `auth_issues` is typically empty for this source. + +**Server-side implication:** `ProviderCredentials` collapses into a `VaultCredentialSource` constructed via `with_env_lookup`. The server doesn't need its own `CredentialSource` impl — the env-lookup policy is a constructor argument, not a type-level distinction. This is the key insight that lets the consolidation actually happen rather than just moving duplication behind a trait. + +### 2. No process-global state + +No `DEFAULT_CLIENT`, no `defaults::install`, no `Client::available_default`. Every long-lived runtime context holds its own `Arc`: + +- `RunServices.llm_source` — built from `InitOptions.vault` at initialize (or `EnvCredentialSource` if vault is `None`; see §3). +- `AppState.llm_source` — built from `Vault::load` at `build_app_state`. +- `HookExecutor.llm_source` — received on construction from the invoker. +- Standalone agent CLI — builds source at startup from resolved vault or env. +- Tests — build stubs directly, no install ceremony. + +**CLI subcommands resolve sources on demand.** `CommandContext` does not hold a source as a field. Instead it exposes a lazy helper: + +```rust +impl CommandContext { + pub async fn llm_source(&self) -> Result> { ... } +} +``` + +This mirrors the existing `CommandContext::server()` pattern (`command_context.rs:106-134`). The helper is re-derived per `with_target`/`with_connection` call, so each subcommand gets the right source for its resolved storage dir — no binding hazard. + +This design eliminates: CLI re-derivation hazard (`CommandContext::with_connection` can change storage_dir), test cross-contamination on shared statics, double-install policy questions, feature-gate hazards, and the question "which install site wins." + +### 3. `RunServices.llm_source` is always `Some` — no `Option` + +Today's `RunSession.vault: Option>>` is optional (workflows can run without a vault, e.g. dry-run or env-only). The plan keeps `RunServices.llm_source` **non-optional** to preserve the elegance of "every context has a source." Rule: + +- **Vault is `Some`:** build `VaultCredentialSource` from it. +- **Vault is `None`:** build `EnvCredentialSource::new()` — reads env vars; may resolve empty. Same semantics as today's `build_llm_client` with `resolver: None` branch. +- **Dry-run:** `EngineServices.dry_run` (separate flag, as today) is the authoritative signal. When true, handlers skip LLM stages entirely; `llm_source` is constructed anyway but unused. +- **No credentials anywhere:** `source.resolve()` returns empty `Vec`. `Client::from_source` returns a Client with `provider_names().is_empty()`. Consumers that need to `generate()` detect this via the same check as today (`initialize.rs:302`) and error with a helpful message built from `auth_issues`. + +The error "No usable LLM providers configured" becomes a point-of-use error rather than an initialize-time error, **except** for graphs where `graph::needs_llm_handler_type` is true — for those, `initialize` preflights `source.resolve()` once and errors early with the same message today produces. Preserves UX. + +### 4. Clients are built at point of use + +`AgentApiBackend` today rebuilds `Client` per session via `build_llm_client(resolver)` — load-bearing for OAuth refresh on long runs. The new design preserves this pattern: every caller that wants a client calls `Client::from_source(&source).await?` at the point of use. Under the hood, `source.resolve()` calls `resolver.resolve()` which refreshes OAuth tokens and writes back to the vault. + +No pinned per-run `Client`. No stale-token regression. + +### 5. `GenerateParams.client: Arc` is required + +Not `Option`. `GenerateParams::new(model, client)` takes both. Compile error if a future caller forgets. + +### 6. Delete `Client::from_env` + +`Client::from_env` was the silent-fallback gateway. It's replaced by `Client::from_source(&EnvCredentialSource::new())` — explicit, one-line, same result, and doesn't leave a footgun for future contributors. + +`Client::from_credentials` stays as the lowest-level constructor (used internally by `from_source`). + +### 7. `SandboxReady` hooks get a source, not a client + +Hooks run at `pipeline/initialize.rs:555`, before `build_registry` at `:587`. In the source-on-context design this is fine: `InitOptions.vault` already exists at hook time; `initialize` builds a `VaultCredentialSource` eagerly (or `EnvCredentialSource` if vault is `None`) and passes it to the hook runner. A `SandboxReady` hook that wants to `generate()` calls `Client::from_source(&source).await?` at its point of use. + +### 8. `RunServices` / `EngineServices` split is composition, not peer wrapping + +Today's single `EngineServices` (13 fields) mixes two lifetimes: + +- **Cross-phase** (live past execute into retro/finalize/PR): `run_store`, `emitter`, `sandbox`, `hook_runner`, `cancel_requested`, `provider`, and the new `llm_source`. +- **Execute-only** (die at the execute→retro boundary): `registry`, `inputs`, `workflow_bundle`, `workflow_path`, `dry_run`, `env`, `git_state`. + +Split into two structs with `EngineServices { run: Arc, ...execute_only_fields }`. Retroed/Concluded/PullRequestOptions carry `Arc` only — they can't reach execute-only state by construction. + +This is composition (`EngineServices` contains `Arc`), not the peer-wrapper pattern the CLI plans rejected. Handlers read `services.run.emitter` for cross-phase state and `services.registry` for execute-only state. One access shape, two concerns. + +### 9. Server's `ProviderCredentials` collapses into `VaultCredentialSource::with_env_lookup` + +`fabro-server/src/server_secrets.rs` today owns two capabilities: +- `build_llm_client` (`:87-112`) — full resolution with OAuth refresh, plus `auth_issues`. +- `configured_providers` (`:114-119`) — cheap preflight used for model selection at `server.rs:3929` and `run_manifest.rs:355`. + +Both collapse into `VaultCredentialSource`: `build_llm_client` → `source.resolve()`; `configured_providers` → `source.configured_providers()` (the second trait method). With `VaultCredentialSource::with_env_lookup` covering the server's env policy (§1), `ProviderCredentials` disappears — `build_app_state` constructs `VaultCredentialSource::with_env_lookup(vault, env_lookup)` directly and stores it as `Arc` on `AppState`. Callers at `server.rs:3929` and `run_manifest.rs:355` swap `state.provider_credentials.configured_providers().await` → `state.llm_source.configured_providers().await`. One trait, one type, two capabilities. + +## Open Questions + +### Resolved During Planning + +- **Trait home?** `fabro-auth` (owns `ApiCredential`, `CredentialResolver`). Avoids `fabro-auth → fabro-llm` cycle. +- **Keep `Client::from_env`?** No. Replaced by `EnvCredentialSource` + `Client::from_source`. One construction path. +- **Process-global static?** No. Every context holds its own source. +- **`Client::available_default`?** Not needed. Callers use `Client::from_source(&ctx.llm_source)` where `ctx` is whatever context they hold. +- **Per-run client caching?** No. `Client::from_source` is called at point of use. Cost is acceptable (one resolver iteration + `from_credentials`), and this is where OAuth refresh happens. +- **OAuth refresh preservation?** Automatic. `resolver.resolve()` handles refresh; it runs every time a caller builds a client from a source. +- **CLI install site?** None. Each `CommandContext`-derived command that needs LLM builds its own source from the context's storage dir. +- **Server install site?** None. `build_app_state` constructs `VaultCredentialSource` once and stores it on `AppState`. +- **`SandboxReady` hook ordering?** No change. `InitOptions.vault` already exists at hook time; source is built before the hook runs. +- **Duplicate `build_llm_client` in server?** Delete. Server uses `VaultCredentialSource`. Parity test in Unit 1.2 guards the deletion. +- **`install_mock_llm` test helper?** Delete. Tests construct sources and clients directly. +- **Partial resolution diagnostics (`auth_issues`)?** Preserved. Trait returns `ResolvedCredentials { credentials, auth_issues }`. Four diagnostic call sites (`initialize.rs:301`, `server.rs:6652`, `server.rs:6821-6833` per-issue `warn!`, `diagnostics.rs:90`) consume `auth_issues` directly via `source.resolve()`. `Client::from_source` is the convenience path for callers that don't need diagnostics. +- **What source exists when `InitOptions.vault` is `None`?** `EnvCredentialSource::new()`. `RunServices.llm_source` is never `Option`. Graph-needs-LLM preflight inside `initialize` produces the same user-facing error today produces. +- **CLI source ownership?** `CommandContext::llm_source(&self) -> Result>` — lazy helper, re-derived per `with_target`/`with_connection`. Mirrors existing `CommandContext::server()`. + +### Deferred to Implementation + +- **Exact error type on the trait.** `Result` — use `fabro-auth::Error` or introduce a trait-level `CredentialSourceError`. Decide while writing the trait. +- **Where `CommandContext::llm_source` caches.** `OnceCell>` on the context like `server: OnceCell>` today, or freshly built per call. Decide by measuring: if CLI subcommands only call it once per invocation anyway, a `OnceCell` is overkill. +- **`HookExecutor` construction signature.** Takes `Arc`. Workflow callers pass `services.run.llm_source.clone()`; standalone callers construct their own. +- **Test harness fixture.** `RunServices::for_test()` builds a stub source + minimal services. Exact surface decided during Unit 2.1. + +## High-Level Technical Design + +> *Directional guidance for review, not implementation specification.* + +### Client resolution flow + +``` +Caller has long-lived Arc + +When a generate() is needed: + let client = Client::from_source(&source).await?; + generate(GenerateParams::new(model, client).prompt("...")) + +Under the hood: + Client::from_source(&source) + └── source.resolve() → ResolvedCredentials { credentials, auth_issues } + └── VaultCredentialSource: iterate Provider::ALL, resolver.resolve() (with OAuth refresh) + or EnvCredentialSource: iterate Provider::ALL, read env + └── Client::from_credentials(resolved.credentials) // Client::from_source discards auth_issues + +For diagnostics-aware callers (initialize preflight, server diag endpoint): + let resolved = source.resolve().await?; + if resolved.credentials.is_empty() && graph_needs_llm { + // build detailed error from resolved.auth_issues + } +``` + +### Services topology + +``` +┌────────────────────── RunServices (Arc) ────────────────────────┐ +│ run_store, emitter, sandbox, hook_runner, cancel_requested, │ +│ provider, llm_source: Arc │ +└───────────────────────────────┬─────────────────────────────────┘ + │ Arc + ▼ +┌──────────────────── EngineServices (Arc) ───────────────────────┐ +│ run: Arc, │ +│ registry, inputs, workflow_bundle, workflow_path, │ +│ dry_run, env, git_state │ +└─────────────────────────────────────────────────────────────────┘ + +Phase data flow: + Persisted → Initialized { engine: Arc } + → Executed { engine: Arc } + → Retroed { services: Arc } // drops execute-only state + → Concluded { services: Arc } + → Finalized { /* no services — terminal */ } +``` + +### Where sources come from + +| Context | Source construction | +|---|---| +| Workflow run | `pipeline/initialize.rs` — `VaultCredentialSource::new(vault)` if `InitOptions.vault` is `Some`, else `EnvCredentialSource::new()` | +| CLI subcommand | `CommandContext::llm_source().await?` — lazy helper built from the context's resolved storage dir | +| Server | `build_app_state` — `VaultCredentialSource::with_env_lookup(vault, env_lookup)` from `Vault::load` (preserves today's server env-lookup policy) | +| Standalone `fabro agent` | CLI startup — resolved vault or `EnvCredentialSource` | +| Tests | Inline stub impl | + +## Implementation Units + +### Phase 1 — Source-based client resolution + +- [x] **Unit 1.1: Add `CredentialSource` trait + `VaultCredentialSource` + `EnvCredentialSource` + `Client::from_source`** + + **Goal:** Pure addition of the new abstractions. No consumer changes. Existing code compiles and runs unchanged. + + **Requirements:** R2, R3 (groundwork). + + **Dependencies:** None. + + **Files:** + - Modify: `lib/crates/fabro-auth/src/lib.rs` (re-exports) + - Create: `lib/crates/fabro-auth/src/credential_source.rs` (trait) + - Create: `lib/crates/fabro-auth/src/vault_source.rs` (`VaultCredentialSource`) + - Create: `lib/crates/fabro-auth/src/env_source.rs` (`EnvCredentialSource`) + - Modify: `lib/crates/fabro-llm/src/client.rs` (add `Client::from_source`) + - Test: inline unit tests in each new file; `lib/crates/fabro-llm/src/client.rs` test for `from_source` + + **Approach:** + - `struct ResolvedCredentials { credentials: Vec, auth_issues: Vec<(Provider, ResolveError)> }`. + - `trait CredentialSource: Send + Sync` with `async fn resolve(&self) -> Result`. Use `async_trait::async_trait`. + - `VaultCredentialSource::new(Arc>) -> Self` wraps `CredentialResolver::new` (default env lookup). + - `VaultCredentialSource::with_env_lookup(Arc>, F) -> Self` where `F: Fn(&str) -> Option + Send + Sync + 'static` wraps `CredentialResolver::with_env_lookup` (preserves server env policy). + - `resolve()` iterates `Provider::ALL`, matching today's logic at `handler/llm/api.rs:43-78` byte-for-byte — both credentials accumulation and `auth_issues` accumulation. + - `configured_providers()` delegates to `CredentialResolver::configured_providers(&vault)` at `fabro-auth/src/resolve.rs:140`. Cheap vault read; no OAuth refresh. Matches today's `ProviderCredentials::configured_providers` at `server_secrets.rs:114-119` byte-for-byte. + - For `EnvCredentialSource`, `configured_providers()` iterates `Provider::ALL` checking env-var presence for each (returns providers whose env vars are set). + - `EnvCredentialSource::new()` reads env for each provider in `Provider::ALL`, emitting `ApiCredential`s. `auth_issues` is always empty. Replaces the body of today's `Client::from_env`. + - `Client::from_source(source: &dyn CredentialSource) -> Result, Error>` — calls `source.resolve()`, discards `auth_issues`, calls `Client::from_credentials`, wraps in `Arc`. Callers that need diagnostics call `source.resolve()` directly. + - Move `auth_issue_message` helper (today duplicated in `fabro-workflow/handler/llm/api.rs:82` and `fabro-server/server_secrets.rs:127`) to `fabro-auth` alongside the trait. + - `ApiCredential` must have a redacting `Debug` impl (verify/add using `fabro-util::redaction`). Add unit test asserting `format!("{:?}", credential)` does not echo key material. + + **Patterns to follow:** + - `CredentialFallback` trait shape in `docs/plans/2026-04-20-002-refactor-extract-fabro-client-crate-plan.md`. + - Existing `CredentialResolver` iteration in `lib/crates/fabro-workflow/src/handler/llm/api.rs:43-78` (preserve semantics exactly, including partial-resolution `auth_issues` accumulation). + + **Test scenarios:** + - Happy path: `VaultCredentialSource::resolve()` with a vault containing `openai_codex` returns `ResolvedCredentials` with the OpenAI entry in `.credentials` and empty `.auth_issues`. + - Happy path: `VaultCredentialSource::configured_providers()` with a vault containing OpenAI + Anthropic returns `[OpenAi, Anthropic]`. No OAuth refresh happens (verified via a stub vault that panics on write). + - Happy path: `EnvCredentialSource::configured_providers()` with `ANTHROPIC_API_KEY` set returns `[Anthropic]`; with nothing set, returns empty. + - Happy path (partial resolution): `VaultCredentialSource::resolve()` with a vault containing `openai_codex` whose refresh token has expired **and** a working Anthropic `ApiKey` returns both — Anthropic in `.credentials`, `(OpenAi, ResolveError::RefreshFailed{...})` in `.auth_issues`. Matches real auth shapes (`CodexOAuth` is OpenAI-only per `fabro-auth/src/credential.rs:69-73`). + - Happy path: `EnvCredentialSource` with `ANTHROPIC_API_KEY` set returns one entry; with nothing set, returns an empty `ResolvedCredentials` (not an error). + - Happy path: `Client::from_source(&stub)` where stub returns a single-provider `ResolvedCredentials` yields a Client with that provider registered. + - Happy path: `Client::from_source(&VaultCredentialSource::new(vault))` with OAuth credential calls through `resolver.resolve` (verified via a stub vault that records calls). + - Edge case: source returning empty credentials yields a Client with `provider_names().is_empty()` — no error. + - Redaction: `format!("{:?}", credential)` does not contain the key/token. + + **Verification:** + - `cargo nextest run -p fabro-auth -p fabro-llm` passes. + - `cargo +nightly-2026-04-14 clippy -p fabro-auth -p fabro-llm --all-targets -- -D warnings` clean. + +- [x] **Unit 1.2: Migrate every LLM consumer to hold/pass sources and build clients explicitly** + + **Goal:** Every `generate()`/`generate_object()`/`stream_generate()` call site constructs its `Arc` explicitly (via `Client::from_source`). Every long-lived backend/executor/context holds `Arc`. `DEFAULT_CLIENT` and the default-client code path remain temporarily so each migration step compiles/tests cleanly. + + **Requirements:** R2, R3, R6. + + **Dependencies:** Unit 1.1. + + **Files:** + - Modify: `lib/crates/fabro-workflow/src/handler/llm/api.rs` — `AgentApiBackend` holds `Arc`; `create_session_for` and `one_shot` call `Client::from_source(&self.source)` per call (matching today's per-session rebuild). Delete `build_llm_client` and the local `auth_issue_message` helper (both moved to `fabro-auth` in Unit 1.1). + - Modify: `lib/crates/fabro-workflow/src/handler/llm/cli.rs` — parallel treatment for `AgentCliBackend`. + - Modify: `lib/crates/fabro-workflow/src/pipeline/initialize.rs` — `build_registry` takes/holds `Arc`. Builds source from `InitOptions.vault` when `Some`, else `EnvCredentialSource::new()`. Passes source to backends. For `graph_needs_llm` paths, preflights `source.resolve()` once at initialize and produces the same "No usable LLM providers configured: ..." error at `:301-318` by consuming `ResolvedCredentials.auth_issues`. Phase 2 moves the source onto `RunServices`; for this unit, hold it alongside the registry return value. + - Modify: `lib/crates/fabro-workflow/src/pipeline/initialize.rs:555-566` — `SandboxReady` hook invocation passes the already-built source into the hook runner. + - Modify: `lib/crates/fabro-workflow/src/pipeline/pull_request.rs` — `build_pr_body`/`maybe_open_pull_request` accept an explicit client (PR #168 already added this; here it becomes non-Option). + - Modify: `lib/crates/fabro-hooks/src/executor.rs` — `HookExecutor::new(..., source: Arc)`. `execute_prompt` and `execute_agent` call `Client::from_source(&self.source)` at point of use. Remove `Client::from_env()` at `:358`. + - Modify: `lib/crates/fabro-hooks/src/runner.rs` — `HookRunner` carries source, forwards to executor. + - Modify: `lib/crates/fabro-cli/src/main.rs` — no process-global install. + - Modify: `lib/crates/fabro-cli/src/command_context.rs` — add lazy helper `pub async fn llm_source(&self) -> Result>`. Resolves from the context's current storage dir; builds `VaultCredentialSource` when a vault is present, `EnvCredentialSource` otherwise. Follow the existing `CommandContext::server()` pattern at `:106-134`. + - Modify: `lib/crates/fabro-cli/src/commands/pr/create.rs` — replace today's direct `Vault::load` at `:103` with `ctx.llm_source().await?`. Call `Client::from_source(&source)`, pass into `maybe_open_pull_request`. + - Modify: `lib/crates/fabro-cli/src/shared/provider_auth.rs` — already passes an explicit client. No change. + - Modify: `lib/crates/fabro-agent/src/cli.rs` — replace `Client::from_env()` with source-based resolution (either via `CommandContext::llm_source` or an equivalent startup helper). + - Modify/delete: `lib/crates/fabro-server/src/server_secrets.rs` — delete `ProviderCredentials` entirely (struct at `:61-75`, `build_llm_client` at `:87-112`, `configured_providers` at `:114-119`, local `auth_issue_message` at `:127`). Both capabilities move to `VaultCredentialSource` via the `CredentialSource` trait. Diagnostic callers at `server.rs:6652`, `server.rs:6821`, and `diagnostics.rs:90` consume `ResolvedCredentials.auth_issues` from `source.resolve()`. + - Modify: `lib/crates/fabro-server/src/server.rs` — `build_app_state` (`:2488`) constructs `VaultCredentialSource::with_env_lookup(vault, env_lookup)` and stores it as `Arc` on `AppState`. `create_completion` (`:6821-6833`) calls `state.llm_source.resolve()` directly (not `Client::from_source`), logs each `auth_issue` via `warn!` (preserving today's observability at `:6831-6833`), then builds the client via `Client::from_credentials(resolved.credentials)`. Server diagnostics endpoint (`:6652`) same pattern. Site at `:3929` swaps `state.provider_credentials.configured_providers().await` → `state.llm_source.configured_providers().await`. + - Modify: `lib/crates/fabro-server/src/run_manifest.rs:355` — same swap: `state.provider_credentials.configured_providers().await` → `state.llm_source.configured_providers().await`. + - Test: per-crate unit tests; a workflow-level integration test that runs a minimal pipeline with a vault-only `openai_codex` credential and asserts agent stages use a fresh client each session. + + **Approach:** + - Mechanical migration. Each consumer that previously used the default client now constructs explicitly from a source it already has access to. + - `AgentApiBackend` today: `build_llm_client(self.resolver.as_ref()).await?.client` per session. New: `Client::from_source(&self.source).await?` per session. Functionally equivalent; OAuth refresh still happens because `source.resolve()` calls `resolver.resolve()` which refreshes. + - `build_registry` no longer needs to return a client — it returns the registry and the source (passed to backends). Clients are built per session inside the backends. + - Diagnostic sites that previously called `build_llm_client(...).auth_issues` now call `source.resolve()` and consume `.auth_issues` directly. Four sites: `initialize.rs:301` (error message), `server.rs:6652` (diagnostics endpoint), `server.rs:6821-6833` (`create_completion` per-issue `warn!`), `diagnostics.rs:90`. Each of these stays on the `resolve()` path rather than using the `Client::from_source` convenience — they need both halves. + + **Patterns to follow:** + - `AgentApiBackend::create_session_for:268` (today's per-session pattern) is the model. + - `CommandContext::server()` (`command_context.rs:106-134`) is the model for the new `llm_source()` helper — lazy, re-derived per context, cached via `OnceCell` per context. + + **Test scenarios:** + - Happy path (workflow, vault): pipeline run with vault-configured `openai_codex` produces a Client per session with `openai` registered. Verified via a test that wraps a stub source and counts `resolve()` calls (should equal session count + 1 for retro/PR + 1 for initialize preflight). + - Happy path (workflow, no vault): `InitOptions.vault = None`, graph has no LLM handlers — initialize succeeds; `llm_source` is an `EnvCredentialSource` and is never queried. + - Error path (workflow, no vault, graph needs LLM): `InitOptions.vault = None`, graph has LLM handlers, no env credentials — initialize preflight `source.resolve()` returns empty credentials; error produced is the same "No usable LLM providers configured: ..." message today builds from `auth_issues` (empty for env source → plain "No LLM providers configured" fallback). + - Error path (workflow, vault with one bad credential + one good): partial resolution — `openai_codex` with expired refresh token + working Anthropic `ApiKey`. Initialize preflight succeeds (Anthropic is usable); `auth_issues` is logged or surfaced per today's behavior. + - Happy path (CLI `pr create`): command calls `ctx.llm_source().await?`, builds client, generates PR body. No `None` parameter anywhere. + - Happy path (CLI context re-derivation): `ctx.with_connection(args)` produces a new context with its own `llm_source` bound to the new storage dir. Verified by calling `llm_source` on two derived contexts with different storage dirs. + - Happy path (hooks, workflow-invoked): `SandboxReady` hook that wants to generate receives source from `initialize`; builds a client and calls `generate`. + - Happy path (hooks, standalone agent): `fabro agent` CLI builds source at startup, passes to `HookExecutor`; hooks build clients as needed. + - Regression: server `create_completion` request generates successfully after `build_app_state` stores the source on `AppState`. + - Regression: server diagnostic endpoint at `server.rs:6652` still reports auth issues for misconfigured providers. + - Regression: server `create_completion` at `server.rs:6821-6833` still emits `warn!` per auth issue before building the client. Verified via a test with a partial-resolution fixture source (one working provider, one failing) that captures `warn!` events and asserts the expected log output. + - Regression: `AgentCliBackend` (CLI-mode agent) works with source-based resolution. + - Parity (resolve): `VaultCredentialSource::with_env_lookup(vault, env_lookup).resolve()` output (credentials + auth_issues) matches today's `ProviderCredentials::build_llm_client()` output for the same fixture vault + env policy. + - Parity (configured_providers): `VaultCredentialSource::with_env_lookup(vault, env_lookup).configured_providers()` output matches today's `ProviderCredentials::configured_providers()` output for the same fixture. Required before deleting the old path. + + **Verification:** + - `cargo nextest run --workspace` passes. + - `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings` clean. + - `rg 'Client::from_env' lib/crates/` — only `client.rs` (constructor definition), `EnvCredentialSource` (if it reuses the body), and test files. + - `rg 'build_llm_client' lib/crates/` — zero hits. + +- [x] **Unit 1.3: Delete the default-client machinery and `Client::from_env`; require `GenerateParams.client`** + + **Goal:** Atomic cleanup. Every consumer already passes an explicit client (Unit 1.2). Now remove the fallback paths. + + **Requirements:** R1, R6. + + **Dependencies:** Unit 1.2. + + **Files:** + - Modify: `lib/crates/fabro-llm/src/generate.rs` — delete `DEFAULT_CLIENT`, `set_default_client`, `get_default_client`, the `Client::from_env()` fallback inside `get_default_client`. `GenerateParams.client: Arc` (not `Option`). `GenerateParams::new(model, client)` takes both. Drop the `.client(...)` builder method. + - Modify: `lib/crates/fabro-llm/src/lib.rs` — drop `set_default_client` re-export. + - Modify: `lib/crates/fabro-llm/src/client.rs` — delete `Client::from_env`. Keep `Client::from_credentials` (used internally by `EnvCredentialSource` and `Client::from_source`). + - Modify: `lib/crates/fabro-workflow/src/pipeline/pull_request.rs` — delete `install_mock_llm` helper and the three `install_mock_llm()` call sites. Tests construct clients directly via a mock source. + - Modify: ~30 test sites in `lib/crates/fabro-llm/src/generate.rs` — use a helper `#[cfg(test)] fn test_params(model: &str) -> GenerateParams` that constructs a mock client. + - Test: new negative test — attempting to build `GenerateParams` without a client is a compile error (trybuild or inspection). + + **Approach:** + - Introduce the test helper first, then swap sites mechanically. + - `GenerateParams` spread patterns (`GenerateParams { response_format: ..., ..params }`) continue to work since `client` is a field with a value being propagated. + - `MockProvider` in `pull_request.rs` keeps its `name` field (added by PR #168) — useful for the regression test where one mock is registered under the "openai" provider name. + + **Test scenarios:** + - Compile-time enforcement: `GenerateParams::new(model)` without a client arg fails to compile (by the struct shape). + - All existing `fabro-llm` generate tests pass with `test_params(model)` swap. + - Regression (pipeline-level): workflow with vault-only `openai_codex` runs to completion and generates a PR body using vault-resolved credentials. (This is the class-bug fix from PR #168, now type-enforced.) + + **Verification:** + - `cargo nextest run --workspace` passes. + - `rg 'DEFAULT_CLIENT|set_default_client|get_default_client|install_mock_llm|Client::from_env' lib/crates/` — zero hits. + - `rg 'Option>' lib/crates/` — zero hits in production code. + +### Phase 2 — `RunServices` split + PR #168 `Option` unwind + +**Prerequisite:** PR #168 must have merged to main. + +- [x] **Unit 2.1: Introduce `RunServices` + `EngineServices` split** + + **Goal:** Today's `EngineServices` (13 fields, mixed lifetimes) partitions into cross-phase `RunServices` + execute-only `EngineServices`. Handlers read cross-phase state as `services.run.xxx`. + + **Requirements:** R4. + + **Dependencies:** Phase 1 complete. + + **Files:** + - Create: `lib/crates/fabro-workflow/src/services.rs` (both structs + `RunServices::for_test`) + - Modify: `lib/crates/fabro-workflow/src/handler/mod.rs` — delete old `EngineServices`; re-export from `services`. + - Modify: `lib/crates/fabro-workflow/src/handler/{parallel,manager_loop,prompt,agent,command,fan_in,human}.rs` — mechanical rewrite: `services.run_store` → `services.run.run_store`, etc. + - Modify: `lib/crates/fabro-workflow/src/node_handler.rs` — same rewrites. + - Modify: `lib/crates/fabro-workflow/src/pipeline/initialize.rs` — `build_registry` now returns `Arc` (holding `Arc` with `llm_source`). Dry-run still produces a valid services pair. + - Modify: `lib/crates/fabro-workflow/src/pipeline/execute.rs` — use `Arc` directly instead of reconstructing it. + - Modify: `lib/crates/fabro-workflow/src/test_support.rs` — test fixture adapts. + - Test: handler unit tests and `fabro-workflow/tests/it/integration.rs` pipeline tests. + + **Approach:** + + | Field | Lives on | + |---|---| + | `run_store` | `RunServices` | + | `emitter` | `RunServices` | + | `sandbox` | `RunServices` | + | `hook_runner` | `RunServices` | + | `cancel_requested` | `RunServices` | + | `provider` | `RunServices` | + | `llm_source` | `RunServices` (**new**) | + | `registry` | `EngineServices` | + | `inputs` | `EngineServices` | + | `workflow_bundle` | `EngineServices` | + | `workflow_path` | `EngineServices` | + | `dry_run` | `EngineServices` | + | `env` | `EngineServices` | + | `git_state` | `EngineServices` | + + - `RunServices::for_test() -> Arc` builds a minimal services bundle with a stub source. Used by handler and pipeline tests. + - No delegation methods on `EngineServices`; handlers read through `services.run.xxx` explicitly. More lines but clearer boundary. + + **Patterns to follow:** + - Existing `Arc` construction at `pipeline/execute.rs:84`. + + **Test scenarios:** + - Happy path: `RunServices::for_test()` constructs cleanly and supports all handler unit tests. + - Happy path: end-to-end pipeline test runs without regression. + - Type-level: a retro-phase test asserting `services.run.emitter` compiles but `services.registry` does not (via a compile-fail test or a comment marker). + - Edge case: `services.run_hooks(&HookContext)` method lives on `RunServices` since `hook_runner` moved there. + - Edge case: `services.git_state()` / `set_git_state(...)` stays on `EngineServices` (execute-only). + + **Verification:** + - `cargo nextest run -p fabro-workflow` passes. + - `cargo clippy -p fabro-workflow --all-targets -- -D warnings` clean. + - `rg 'services\.run_store' lib/crates/fabro-workflow/src/handler/` — zero hits (all now `services.run.run_store`). + +- [x] **Unit 2.2: Shrink phase structs; unwind PR #168 `Option`** + + **Goal:** `Initialized`/`Executed` carry `Arc`; `Retroed`/`Concluded` carry `Arc`. Delete `Option` threading introduced by PR #168. + + **Requirements:** R4, R5. + + **Dependencies:** Unit 2.1 + PR #168 merged. + + **Files:** + - Modify: `lib/crates/fabro-workflow/src/pipeline/types.rs` — shrink phase structs (below), shrink Options structs. + - Modify: `lib/crates/fabro-workflow/src/pipeline/execute.rs` — destructure updates (carry `engine`). + - Modify: `lib/crates/fabro-workflow/src/pipeline/retro.rs` — read `executed.engine.run`; drop `options.llm_client`. + - Modify: `lib/crates/fabro-workflow/src/pipeline/finalize.rs` — destructure updates. + - Modify: `lib/crates/fabro-workflow/src/pipeline/pull_request.rs` — drop `Option` parameter from `build_pr_body`/`maybe_open_pull_request`/`pull_request`; build via `Client::from_source(&services.run.llm_source)`. + - Modify: `lib/crates/fabro-workflow/src/operations/start.rs` — `RunSession::run` threads services Arc through phase calls (no longer cloning `executed.llm_client`). + - Modify: `lib/crates/fabro-cli/src/commands/pr/create.rs` — build `RunServices::for_cli(source, run_store, ...)` and pass to `maybe_open_pull_request`. + - Test: `lib/crates/fabro-workflow/tests/it/integration.rs` pipeline-level regression; update phase-level tests. + + **Approach — phase structs after the split:** + + - `Initialized { graph, source, inputs, run_options, workflow_path, workflow_bundle, checkpoint, seed_context, engine: Arc, model }` + - `Executed { graph, outcome, run_options, duration_ms, final_context, engine: Arc, model }` + - `Retroed { graph, outcome, run_options, duration_ms, retro, services: Arc }` + - `Concluded { run_id, outcome, conclusion, pushed_branch, graph, run_options, services: Arc }` + - `Finalized { run_id, outcome, conclusion, pushed_branch, pr_url }` (unchanged, terminal) + - `RetroOptions` drops `llm_client`, `provider`, `sandbox`, `run_store`, `emitter` (read from services); keeps `run_id`, `workflow_name`, `goal`, `run_dir`, `failed`, `run_duration_ms`, `enabled`, `model`. + - `PullRequestOptions` drops `run_store`; keeps `run_dir`, `pr_config`, `github_app`, `origin_url`, `model`. + - `build_pr_body(services: &RunServices, diff, goal, model, conclusion) -> Result` — builds client via `Client::from_source(&services.llm_source)`. + + All phase structs retain `#[non_exhaustive]` (already present). + + **Patterns to follow:** + - Unit 2.1's `Arc` shape. + + **Test scenarios:** + - Regression (the R5 pipeline-level test): workflow run with vault-configured `openai_codex`, no `OPENAI_API_KEY` env, generates a PR body successfully. Previously failed with "Provider 'openai' not registered." + - Happy path: CLI `fabro pr create` resolves vault, builds source, builds `RunServices::for_cli`, calls `maybe_open_pull_request`. + - Edge case: retro when disabled still no-ops (doesn't call `from_source`). + - Edge case: workflow with no LLM stages and no vault initializes cleanly (source built from vault; `Client::from_source` is called lazily by consumers, not eagerly). + + **Verification:** + - `cargo nextest run --workspace` passes. + - `rg 'llm_client:' lib/crates/fabro-workflow/` — zero hits in non-test code. + - `rg 'Option' lib/crates/` — zero production hits. + +## System-Wide Impact + +- **Interaction graph:** Every `generate`/`generate_object`/`stream_generate` consumer is affected — `fabro-workflow`, `fabro-hooks`, `fabro-agent`, `fabro-cli`, `fabro-server`. Each now builds its client from a source it already holds. +- **Error propagation:** Source-resolution errors (vault lookup failure, OAuth refresh failure) surface at `Client::from_source` call sites, which is the same point they surface today at `build_llm_client` call sites. Error semantics preserved. +- **State lifecycle risks:** `Client::from_source` builds a fresh client per call. For most contexts this runs once per long-running operation (per session, per PR body, per hook invocation). The cost is identical to today's `build_llm_client(resolver)` pattern — unchanged. +- **API surface parity:** `fabro_llm::set_default_client` re-export deletes. `GenerateParams::new` signature changes. `Client::from_env` deletes. Internal-only API shape changes; no external consumers in this monorepo. +- **Integration coverage:** R5's pipeline-level regression test (Unit 2.2) is the critical insurance — proves the class bug is closed at the pipeline shape level, not just at one unit. +- **Credential trust boundary:** `RunServices.llm_source` is shared across all phases by design. Workflow stages run against the same vault as retro/PR body — which matches today's behavior. Callers deploying for multi-tenant workflows must not mix trust levels in a single run. Per-phase credential scoping is out of scope. +- **Unchanged invariants:** `Vault`, `CredentialResolver`, `ApiCredential`, `Provider::ALL`, `ResolvedCredential` enum, phase ordering, `#[non_exhaustive]` on all phase structs, OAuth refresh behavior. + +## Risks & Dependencies + +| Risk | Mitigation | +|---|---| +| PR #168 lands with `Option` threading that Phase 2 needs to unwind. If PR #168 stalls, Phase 2 blocks. | Phase 1 is independent of PR #168 and can land first. Phase 2 starts when PR #168 merges. | +| 30+ `GenerateParams::new("mock-model")` test sites in `fabro-llm/src/generate.rs`. | Introduce `test_params(model)` helper before migrating sites; one-line edits thereafter (Unit 1.3). | +| Phase struct destructure patterns in `pipeline/execute.rs`, `pipeline/retro.rs`, `pipeline/finalize.rs` break when struct shape changes. | Unit 2.2 updates them explicitly. `#[non_exhaustive]` protects external crates; in-crate destructures adjust. | +| Server `ProviderCredentials` owns two capabilities (`build_llm_client` + `configured_providers`). Consolidation must preserve both. | Unit 1.2 parity tests on both paths: `resolve()` vs today's `build_llm_client()` (credentials + auth_issues), and `configured_providers()` vs today's `configured_providers()`, against the same fixture vault + env policy. Reconcile any divergence before deletion. | +| `SandboxReady` hooks today have no client; tomorrow they call `Client::from_source(&source)` on demand. Construction cost per hook. | Source already holds the resolver; `source.resolve()` is the same cost as today's `build_llm_client` call that would happen anyway if a hook wanted to generate. No net regression. | +| `AgentApiBackend` per-session client rebuild is load-bearing for OAuth refresh. | Preserved verbatim: backend holds source, calls `Client::from_source(&source)` per session. `source.resolve()` → `resolver.resolve()` → OAuth refresh. Identical behavior. | + +## Documentation / Operational Notes + +- Add a short strategy doc at `docs-internal/llm-client-resolution.md` describing the source-on-context rule: `fabro-auth::CredentialSource` is the authority, `Client` is always derived via `Client::from_source`, `generate()` requires an explicit client. Reference `docs-internal/server-secrets-strategy.md` for the adjacent "no `from_env` in production paths" enforcement model. +- A future follow-up should create a `docs/solutions/` entry capturing "pipeline boundary drop" as a pattern learning. Out of scope for this plan. + +## Sources & References + +- **Related PR:** #168 (`fix(workflow): reuse resolved llm client for auto-pr`) — the point fix this refactor replaces. +- **Related plans:** + - `docs/plans/2026-04-20-002-refactor-extract-fabro-client-crate-plan.md` — `CredentialFallback` named-trait precedent. + - `docs/plans/2026-04-05-server-canonical-secrets-doctor-repo-plan.md` — `from_env()` smell pre-flagged. + - `docs/plans/2026-04-22-003-refactor-lock-down-server-secrets-plan.md` — credential taxonomy. + - `docs/plans/2026-04-08-cli-services-command-context-refactor-plan.md`, `docs/plans/2026-04-23-001-refactor-command-context-alignment-plan.md` — "no peer wrapper" precedent (addressed in Key Technical Decisions). +- **Strategy docs:** + - `docs-internal/server-secrets-strategy.md` — the `set_var`/`remove_var` enforcement model is precedent for enforcing "no `from_env` in production paths." +- **Target files:** `lib/crates/fabro-llm/`, `lib/crates/fabro-auth/`, `lib/crates/fabro-workflow/`, `lib/crates/fabro-cli/`, `lib/crates/fabro-server/`, `lib/crates/fabro-hooks/`, `lib/crates/fabro-agent/`. + +## Unresolved questions + +- **`docs/solutions/` seeding** — out of scope for this plan; flagged as a follow-up. +- **Observability** — should `VaultCredentialSource::resolve` emit a tracing event on successful resolution? Low-value, but worth deciding while implementing. diff --git a/docs/reference/sdk.mdx b/docs/reference/sdk.mdx index e44b7a9fe..c23e2232a 100644 --- a/docs/reference/sdk.mdx +++ b/docs/reference/sdk.mdx @@ -16,6 +16,7 @@ The `fabro-agent` crate provides a session-based AI agent that runs an LLM with ```toml title="Cargo.toml" [dependencies] +fabro-auth = { git = "https://github.com/fabro-sh/fabro" } fabro-agent = { git = "https://github.com/fabro-sh/fabro" } fabro-llm = { git = "https://github.com/fabro-sh/fabro" } tokio = { version = "1", features = ["full"] } @@ -25,18 +26,20 @@ tokio = { version = "1", features = ["full"] } ```rust use fabro_agent::{ - AnthropicProfile, LocalSandbox, Session, SessionConfig, + AnthropicProfile, LocalSandbox, Session, SessionOptions, }; +use fabro_auth::EnvCredentialSource; use fabro_llm::client::Client; use std::path::PathBuf; use std::sync::Arc; #[tokio::main] async fn main() -> Result<(), Box> { - let client = Client::from_env().await?; + let source = EnvCredentialSource::new(); + let client = Client::from_source(&source).await?.as_ref().clone(); let sandbox = Arc::new(LocalSandbox::new(PathBuf::from("."))); let profile = Arc::new(AnthropicProfile::new("claude-sonnet-4-5")); - let config = SessionConfig::default(); + let config = SessionOptions::default(); let mut session = Session::new(client, profile, sandbox, config); session.initialize().await; @@ -66,9 +69,9 @@ async fn main() -> Result<(), Box> { ```rust pub fn new( llm_client: Client, - provider_profile: Arc, + provider_profile: Arc, sandbox: Arc, - config: SessionConfig, + config: SessionOptions, ) -> Self ``` @@ -97,7 +100,7 @@ pub fn new( | `steer(message)` | Injects a system-level guidance message into the next LLM call. | | `follow_up(message)` | Queues a follow-up user message after the current turn completes. | -### SessionConfig +### SessionOptions All fields are public. Key settings with their defaults: @@ -159,10 +162,10 @@ The `DaytonaSandbox` implementation (feature-gated: `daytona`) runs inside a Day ### Provider profiles -The `ProviderProfile` trait encapsulates LLM-specific system prompts, tool definitions, and capability metadata. It controls how the agent presents itself to the model. +The `AgentProfile` trait encapsulates LLM-specific system prompts, tool definitions, and capability metadata. It controls how the agent presents itself to the model. ```rust -pub trait ProviderProfile: Send + Sync { +pub trait AgentProfile: Send + Sync { fn provider(&self) -> Provider; fn model(&self) -> &str; fn tool_registry(&self) -> &ToolRegistry; @@ -250,10 +253,10 @@ impl ToolHookCallback for MyHooks { } ``` -Pass hooks via `SessionConfig`: +Pass hooks via `SessionOptions`: ```rust -let config = SessionConfig { +let config = SessionOptions { tool_hooks: Some(Arc::new(MyHooks)), ..Default::default() }; @@ -265,7 +268,7 @@ For simple sync approval, use `ToolApprovalAdapter` to wrap a closure: use fabro_agent::ToolApprovalAdapter; use std::sync::Arc; -let config = SessionConfig { +let config = SessionOptions { tool_hooks: Some(Arc::new(ToolApprovalAdapter(Arc::new(|tool_name, _args| { if tool_name == "shell" { Err("shell is not allowed".into()) @@ -299,6 +302,7 @@ You can use it independently of Fabro's workflow engine — add it as a dependen ```toml title="Cargo.toml" [dependencies] +fabro-auth = { git = "https://github.com/fabro-sh/fabro" } fabro-llm = { git = "https://github.com/fabro-sh/fabro" } tokio = { version = "1", features = ["full"] } serde_json = "1" @@ -306,20 +310,20 @@ serde_json = "1" ### Quick start -The simplest path is `Client::from_env()`, which auto-registers providers based on environment variables (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, `GEMINI_API_KEY`, etc.): +The simplest path is an environment-backed `CredentialSource`, then `Client::from_source(&source)`. That keeps credential resolution explicit while still auto-reading environment variables such as `ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, and `GEMINI_API_KEY`. ```rust +use fabro_auth::EnvCredentialSource; use fabro_llm::client::Client; use fabro_llm::generate::{generate, GenerateParams}; -use fabro_llm::set_default_client; #[tokio::main] async fn main() -> Result<(), Box> { - let client = Client::from_env().await?; - set_default_client(client); + let source = EnvCredentialSource::new(); + let client = Client::from_source(&source).await?; let result = generate( - GenerateParams::new("claude-sonnet-4-5") + GenerateParams::new("claude-sonnet-4-5", client.clone()) .prompt("Explain ownership in Rust in two sentences.") ).await?; @@ -333,13 +337,17 @@ async fn main() -> Result<(), Box> { `Client` is the core type that holds provider adapters and middleware. It routes each request to the appropriate provider. -#### Creating from environment +#### Creating from a credential source ```rust -let client = Client::from_env().await?; +use fabro_auth::EnvCredentialSource; +use fabro_llm::client::Client; + +let source = EnvCredentialSource::new(); +let client = Client::from_source(&source).await?; ``` -This checks for API key environment variables and registers adapters for each provider found: +For env-backed usage, `EnvCredentialSource` checks for API key environment variables and registers adapters for each provider found: | Environment variable | Provider | |---|---| @@ -351,7 +359,7 @@ This checks for API key environment variables and registers adapters for each pr | `MINIMAX_API_KEY` | Minimax | | `INCEPTION_API_KEY` | Inception | -The first provider registered becomes the default. Optional base URL overrides (e.g. `ANTHROPIC_BASE_URL`) are also read. +The first provider registered becomes the default. Optional base URL overrides (e.g. `ANTHROPIC_BASE_URL`) are also read. For vault-backed usage inside Fabro, use `fabro_auth::VaultCredentialSource` instead. #### Creating manually @@ -394,10 +402,14 @@ The `generate()` function wraps the client with automatic tool execution loops, #### Basic completion ```rust +use fabro_auth::EnvCredentialSource; +use fabro_llm::client::Client; use fabro_llm::generate::{generate, GenerateParams}; +# let source = EnvCredentialSource::new(); +# let client = Client::from_source(&source).await?; let result = generate( - GenerateParams::new("claude-sonnet-4-5") + GenerateParams::new("claude-sonnet-4-5", client.clone()) .system("You are a helpful assistant.") .prompt("What is the capital of France?") .temperature(0.0) @@ -411,10 +423,14 @@ println!("{}", result.text()); Use `.messages()` instead of `.prompt()` to pass a full conversation history: ```rust +use fabro_auth::EnvCredentialSource; +use fabro_llm::client::Client; use fabro_llm::types::Message; +# let source = EnvCredentialSource::new(); +# let client = Client::from_source(&source).await?; let result = generate( - GenerateParams::new("claude-sonnet-4-5") + GenerateParams::new("claude-sonnet-4-5", client.clone()) .messages(vec![ Message::user("My name is Alice."), Message::assistant("Hello Alice! How can I help you?"), @@ -431,7 +447,7 @@ You cannot use both `.prompt()` and `.messages()` on the same request — this r | Method | Type | Description | |---|---|---| -| `new(model)` | `impl Into` | Required. Model ID or alias (e.g. `"opus"`, `"claude-sonnet-4-5"`) | +| `new(model, client)` | `(impl Into, Arc)` | Required. Model ID or alias plus the client to use | | `.prompt(text)` | `impl Into` | Convenience: sends a single user message | | `.messages(msgs)` | `Vec` | Full conversation history | | `.system(text)` | `impl Into` | System prompt | @@ -446,7 +462,6 @@ You cannot use both `.prompt()` and `.messages()` on the same request — this r | `.provider(name)` | `impl Into` | Force a specific provider | | `.max_retries(n)` | `u32` | Retry count for transient errors (default: 2) | | `.timeout(config)` | `TimeoutConfig` | Total and per-step timeouts | -| `.client(client)` | `Arc` | Override the default client | | `.abort_signal(token)` | `CancellationToken` | Cancel generation | | `.stop_when(f)` | `Fn(&[StepResult]) -> bool` | Custom stop condition after each tool round | @@ -480,6 +495,8 @@ Tools let the model call functions during generation. There are two kinds: #### Defining an active tool ```rust +use fabro_auth::EnvCredentialSource; +use fabro_llm::client::Client; use fabro_llm::tools::Tool; use serde_json::json; @@ -503,8 +520,12 @@ let weather = Tool::active( #### Using tools with generate ```rust +# use fabro_auth::EnvCredentialSource; +# use fabro_llm::client::Client; +# let source = EnvCredentialSource::new(); +# let client = Client::from_source(&source).await?; let result = generate( - GenerateParams::new("claude-sonnet-4-5") + GenerateParams::new("claude-sonnet-4-5", client.clone()) .prompt("What's the weather in San Francisco?") .tools(vec![weather]) .max_tool_rounds(5) @@ -527,18 +548,22 @@ Control how the model selects tools: use fabro_llm::types::ToolChoice; // Let the model decide (default) -GenerateParams::new("opus").tool_choice(ToolChoice::Auto); +# use fabro_auth::EnvCredentialSource; +# use fabro_llm::client::Client; +# let source = EnvCredentialSource::new(); +# let client = Client::from_source(&source).await?; +GenerateParams::new("opus", client.clone()).tool_choice(ToolChoice::Auto); // Force a specific tool -GenerateParams::new("opus").tool_choice(ToolChoice::Named { +GenerateParams::new("opus", client.clone()).tool_choice(ToolChoice::Named { tool_name: "get_weather".into() }); // Force the model to use some tool -GenerateParams::new("opus").tool_choice(ToolChoice::Required); +GenerateParams::new("opus", client.clone()).tool_choice(ToolChoice::Required); // Prevent tool use -GenerateParams::new("opus").tool_choice(ToolChoice::None); +GenerateParams::new("opus", client.clone()).tool_choice(ToolChoice::None); ``` #### Passive tools @@ -546,6 +571,10 @@ GenerateParams::new("opus").tool_choice(ToolChoice::None); Passive tools let you handle execution yourself: ```rust +# use fabro_auth::EnvCredentialSource; +# use fabro_llm::client::Client; +# let source = EnvCredentialSource::new(); +# let client = Client::from_source(&source).await?; let search = Tool::passive( "search", "Search the codebase", @@ -559,7 +588,7 @@ let search = Tool::passive( ); let result = generate( - GenerateParams::new("claude-sonnet-4-5") + GenerateParams::new("claude-sonnet-4-5", client.clone()) .prompt("Find all uses of the Config struct") .tools(vec![search]) ).await?; @@ -577,11 +606,15 @@ for call in result.tool_calls() { For simple cases where you only need the text deltas: ```rust +use fabro_auth::EnvCredentialSource; +use fabro_llm::client::Client; use fabro_llm::generate::{stream, GenerateParams}; use futures::StreamExt; +# let source = EnvCredentialSource::new(); +# let client = Client::from_source(&source).await?; let stream_result = stream( - GenerateParams::new("claude-sonnet-4-5") + GenerateParams::new("claude-sonnet-4-5", client.clone()) .prompt("Write a haiku about Rust") ).await?; @@ -596,12 +629,16 @@ while let Some(chunk) = text_stream.next().await { For fine-grained control, consume `StreamEvent` variants directly: ```rust +use fabro_auth::EnvCredentialSource; +use fabro_llm::client::Client; use fabro_llm::generate::{stream, GenerateParams}; use fabro_llm::types::StreamEvent; use futures::StreamExt; +# let source = EnvCredentialSource::new(); +# let client = Client::from_source(&source).await?; let mut stream_result = stream( - GenerateParams::new("claude-sonnet-4-5") + GenerateParams::new("claude-sonnet-4-5", client.clone()) .prompt("Explain monads") ).await?; @@ -646,9 +683,13 @@ while let Some(event) = stream_result.next().await { Generate typed JSON objects that conform to a JSON Schema: ```rust +use fabro_auth::EnvCredentialSource; +use fabro_llm::client::Client; use fabro_llm::generate::{generate_object, GenerateParams}; use serde_json::json; +# let source = EnvCredentialSource::new(); +# let client = Client::from_source(&source).await?; let schema = json!({ "type": "object", "properties": { @@ -663,7 +704,7 @@ let schema = json!({ }); let result = generate_object( - GenerateParams::new("claude-sonnet-4-5") + GenerateParams::new("claude-sonnet-4-5", client.clone()) .prompt("Generate a profile for a fictional character"), schema, ).await?; @@ -712,8 +753,15 @@ impl Middleware for LoggingMiddleware { Add middleware to the client: ```rust -let mut client = Client::from_env().await?; -client.add_middleware(Arc::new(LoggingMiddleware)); +use fabro_auth::EnvCredentialSource; +use fabro_llm::client::Client; +use std::sync::Arc; + +let source = EnvCredentialSource::new(); +let mut client = Client::from_source(&source).await?; +Arc::get_mut(&mut client) + .expect("install middleware before sharing the client") + .add_middleware(Arc::new(LoggingMiddleware)); ``` ### Model catalog @@ -820,8 +868,12 @@ Retry only fires when `error.retryable()` returns `true` and respects `Retry-Aft Pass a `CancellationToken` to interrupt long-running generation: ```rust +use fabro_auth::EnvCredentialSource; +use fabro_llm::client::Client; use tokio_util::sync::CancellationToken; +# let source = EnvCredentialSource::new(); +# let client = Client::from_source(&source).await?; let token = CancellationToken::new(); let token_clone = token.clone(); @@ -832,7 +884,7 @@ tokio::spawn(async move { }); let result = generate( - GenerateParams::new("opus") + GenerateParams::new("opus", client.clone()) .prompt("Write a novel") .abort_signal(token) ).await; diff --git a/lib/crates/fabro-agent/Cargo.toml b/lib/crates/fabro-agent/Cargo.toml index 4fbe1079c..2a1334a31 100644 --- a/lib/crates/fabro-agent/Cargo.toml +++ b/lib/crates/fabro-agent/Cargo.toml @@ -24,6 +24,7 @@ workspace = true [dependencies] clap.workspace = true anyhow.workspace = true +fabro-auth = { path = "../fabro-auth" } fabro-config = { path = "../fabro-config", features = ["clap"] } fabro-types = { path = "../fabro-types" } fabro-llm = { path = "../fabro-llm" } @@ -31,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 4b8c7dd25..1ce8f4870 100644 --- a/lib/crates/fabro-agent/src/cli.rs +++ b/lib/crates/fabro-agent/src/cli.rs @@ -8,6 +8,9 @@ use std::path::PathBuf; use std::sync::{Arc, Mutex}; use clap::{Args, Parser}; +use fabro_auth::{CredentialSource, EnvCredentialSource, VaultCredentialSource}; +use fabro_config::Storage; +use fabro_config::user::default_storage_dir; use fabro_llm::Error as LlmError; use fabro_llm::client::Client; use fabro_llm::middleware::{Middleware, NextFn, NextStreamFn}; @@ -16,9 +19,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; @@ -215,20 +219,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 @@ -243,6 +245,35 @@ 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() -> Arc { + let storage_dir = default_storage_dir(); + match Vault::load(Storage::new(storage_dir).secrets_path()) { + Ok(vault) => Arc::new(VaultCredentialSource::new(Arc::new(AsyncRwLock::new( + vault, + )))), + Err(_) => Arc::new(EnvCredentialSource::new()), + } +} + +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() @@ -402,7 +433,26 @@ 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_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( @@ -412,31 +462,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}'"); - } - Client::from_env() - .await - .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 })); @@ -458,7 +492,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(".")); @@ -494,7 +532,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-agent/src/session.rs b/lib/crates/fabro-agent/src/session.rs index 673b2230c..7a4b846f0 100644 --- a/lib/crates/fabro-agent/src/session.rs +++ b/lib/crates/fabro-agent/src/session.rs @@ -2,6 +2,7 @@ use std::collections::{HashMap, VecDeque}; use std::sync::{Arc, Mutex}; use std::time::SystemTime; +use fabro_auth::CredentialSource; use fabro_llm::client::Client; use fabro_llm::error::ProviderErrorKind; use fabro_llm::generate::StreamAccumulator; @@ -91,6 +92,33 @@ impl Session { } } + /// Build a session from a credential source. Resolves the LLM client + /// once at construction and caches it for the session's lifetime. + /// Sessions are bounded (≤ 1 hour); cached client is fine within that + /// window. For longer-lived contexts (workflow runs) hold a source, + /// not a session. + /// + /// # Errors + /// + /// Returns an error if `Client::from_source` fails (e.g. vault unreachable, + /// OAuth refresh failed). + pub async fn from_source( + source: &dyn CredentialSource, + provider_profile: Arc, + sandbox: Arc, + config: SessionOptions, + subagent_manager: Option>>, + ) -> Result { + let client = Client::from_source(source).await?; + Ok(Self::new( + client, + provider_profile, + sandbox, + config, + subagent_manager, + )) + } + pub fn set_tool_env(&mut self, env: HashMap) { self.tool_env = Some(env); } diff --git a/lib/crates/fabro-agent/tests/it/parity_matrix.rs b/lib/crates/fabro-agent/tests/it/parity_matrix.rs index 77bfe92fa..8b8630e4d 100644 --- a/lib/crates/fabro-agent/tests/it/parity_matrix.rs +++ b/lib/crates/fabro-agent/tests/it/parity_matrix.rs @@ -13,6 +13,7 @@ use fabro_agent::{ AgentProfile, AnthropicProfile, GeminiProfile, LocalSandbox, OpenAiProfile, Session, SessionOptions, SubAgentManager, WebFetchSummarizer, }; +use fabro_auth::EnvCredentialSource; use fabro_llm::client::Client; use fabro_llm::provider::{Provider, ProviderAdapter}; use fabro_llm::providers::OpenAiAdapter; @@ -148,7 +149,10 @@ async fn make_client(provider: Provider, twin: Option<&OpenAiTwinOptions>) -> Cl return make_twin_client(twin.expect("openai twin config should be provided")); } - Client::from_env().await.expect("Client::from_env failed") + let source = EnvCredentialSource::new(); + Client::from_source(&source) + .await + .expect("Client::from_source failed") } fn make_twin_client(twin: &OpenAiTwinOptions) -> Client { diff --git a/lib/crates/fabro-auth/Cargo.toml b/lib/crates/fabro-auth/Cargo.toml index 747e943ba..1b7ebeb0d 100644 --- a/lib/crates/fabro-auth/Cargo.toml +++ b/lib/crates/fabro-auth/Cargo.toml @@ -17,6 +17,7 @@ chrono = { workspace = true, features = ["serde"] } fabro-http.workspace = true fabro-model = { path = "../fabro-model" } fabro-oauth = { path = "../fabro-oauth" } +fabro-util = { path = "../fabro-util" } fabro-vault = { path = "../fabro-vault" } serde.workspace = true serde_json.workspace = true diff --git a/lib/crates/fabro-auth/src/credential.rs b/lib/crates/fabro-auth/src/credential.rs index ac5479a77..96df27c88 100644 --- a/lib/crates/fabro-auth/src/credential.rs +++ b/lib/crates/fabro-auth/src/credential.rs @@ -1,5 +1,6 @@ use chrono::{DateTime, Duration, Utc}; use fabro_model::Provider; +use fabro_util::redact::redact_string; use serde::{Deserialize, Serialize}; #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] @@ -57,12 +58,37 @@ pub struct OAuthConfig { pub use_pkce: bool, } -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Clone, PartialEq, Eq)] pub enum ApiKeyHeader { Bearer(String), Custom { name: String, value: String }, } +fn redact_for_debug(value: &str) -> String { + let redacted = redact_string(value); + if redacted == value && !value.is_empty() { + "REDACTED".to_string() + } else { + redacted + } +} + +impl std::fmt::Debug for ApiKeyHeader { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Bearer(value) => f + .debug_tuple("Bearer") + .field(&redact_for_debug(value)) + .finish(), + Self::Custom { name, value } => f + .debug_struct("Custom") + .field("name", name) + .field("value", &redact_for_debug(value)) + .finish(), + } + } +} + pub fn credential_id_for(credential: &AuthCredential) -> Result { match (&credential.provider, &credential.details) { (Provider::OpenAi, AuthDetails::ApiKey { .. }) => Ok("openai".to_string()), @@ -160,4 +186,14 @@ mod tests { assert!(parse_credential_secret("openai", &json).is_err()); assert!(parse_credential_secret("openai_codex", "{").is_err()); } + + #[test] + fn api_key_header_debug_redacts_secret_values() { + let header = ApiKeyHeader::Bearer("sk-test".to_string()); + + let debug = format!("{header:?}"); + + assert!(!debug.contains("sk-test")); + assert!(debug.contains("REDACTED")); + } } diff --git a/lib/crates/fabro-auth/src/credential_source.rs b/lib/crates/fabro-auth/src/credential_source.rs new file mode 100644 index 000000000..bba9f4600 --- /dev/null +++ b/lib/crates/fabro-auth/src/credential_source.rs @@ -0,0 +1,17 @@ +use async_trait::async_trait; +use fabro_model::Provider; + +use crate::{ApiCredential, ResolveError}; + +#[derive(Debug)] +pub struct ResolvedCredentials { + pub credentials: Vec, + pub auth_issues: Vec<(Provider, ResolveError)>, +} + +#[async_trait] +pub trait CredentialSource: Send + Sync { + async fn resolve(&self) -> anyhow::Result; + + async fn configured_providers(&self) -> Vec; +} diff --git a/lib/crates/fabro-auth/src/env_source.rs b/lib/crates/fabro-auth/src/env_source.rs new file mode 100644 index 000000000..c387a1e84 --- /dev/null +++ b/lib/crates/fabro-auth/src/env_source.rs @@ -0,0 +1,166 @@ +use std::sync::Arc; + +use async_trait::async_trait; +use fabro_model::Provider; + +use crate::credential_source::{CredentialSource, ResolvedCredentials}; +use crate::{ApiCredential, EnvLookup}; + +#[derive(Clone)] +pub struct EnvCredentialSource { + env_lookup: EnvLookup, +} + +impl EnvCredentialSource { + #[must_use] + pub fn new() -> Self { + Self::with_env_lookup(Arc::new(|name| std::env::var(name).ok())) + } + + #[must_use] + pub fn with_env_lookup(env_lookup: EnvLookup) -> Self { + Self { env_lookup } + } + + fn lookup(&self, name: &str) -> Option { + (self.env_lookup)(name) + } + + fn credential_for(&self, provider: Provider) -> Option { + let key = provider + .api_key_env_vars() + .iter() + .find_map(|var| self.lookup(var))?; + + let mut cred = ApiCredential::from_api_key(provider, key); + match provider { + Provider::Anthropic => { + cred.base_url = self.lookup("ANTHROPIC_BASE_URL"); + } + Provider::OpenAi => { + cred.base_url = self.lookup("OPENAI_BASE_URL"); + cred.org_id = self.lookup("OPENAI_ORG_ID"); + cred.project_id = self.lookup("OPENAI_PROJECT_ID"); + if let Some(account_id) = self.lookup("CHATGPT_ACCOUNT_ID") { + cred.base_url = Some("https://chatgpt.com/backend-api/codex".to_string()); + cred.codex_mode = true; + cred.extra_headers + .insert("ChatGPT-Account-Id".to_string(), account_id); + cred.extra_headers + .insert("originator".to_string(), "fabro".to_string()); + } + } + Provider::Gemini => { + cred.base_url = self.lookup("GEMINI_BASE_URL"); + } + Provider::Kimi | Provider::Zai | Provider::Minimax | Provider::Inception => {} + // OpenAiCompatible has no api_key_env_vars, so find_map returned None above. + Provider::OpenAiCompatible => unreachable!(), + } + Some(cred) + } +} + +impl std::fmt::Debug for EnvCredentialSource { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("EnvCredentialSource") + .finish_non_exhaustive() + } +} + +impl Default for EnvCredentialSource { + fn default() -> Self { + Self::new() + } +} + +#[async_trait] +impl CredentialSource for EnvCredentialSource { + async fn resolve(&self) -> anyhow::Result { + let credentials = Provider::ALL + .iter() + .copied() + .filter_map(|provider| self.credential_for(provider)) + .collect(); + + Ok(ResolvedCredentials { + credentials, + auth_issues: Vec::new(), + }) + } + + async fn configured_providers(&self) -> Vec { + Provider::ALL + .iter() + .copied() + .filter(|provider| { + provider + .api_key_env_vars() + .iter() + .any(|env_var| self.lookup(env_var).is_some()) + }) + .collect() + } +} + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + use std::sync::Arc; + + use fabro_model::Provider; + + use super::EnvCredentialSource; + use crate::CredentialSource; + + fn test_source(entries: &[(&str, &str)]) -> EnvCredentialSource { + let entries: HashMap = entries + .iter() + .map(|(key, value)| ((*key).to_string(), (*value).to_string())) + .collect(); + EnvCredentialSource::with_env_lookup(Arc::new(move |name| entries.get(name).cloned())) + } + + #[tokio::test] + async fn configured_providers_reads_injected_env() { + let source = test_source(&[("ANTHROPIC_API_KEY", "anthropic-key")]); + + assert_eq!(source.configured_providers().await, vec![ + Provider::Anthropic + ]); + } + + #[tokio::test] + async fn resolve_returns_empty_when_no_keys_are_configured() { + let source = test_source(&[]); + + let resolved = source.resolve().await.unwrap(); + + assert!(resolved.credentials.is_empty()); + assert!(resolved.auth_issues.is_empty()); + } + + #[tokio::test] + async fn resolve_builds_openai_codex_env_credential() { + let source = test_source(&[ + ("OPENAI_API_KEY", "openai-key"), + ("CHATGPT_ACCOUNT_ID", "acct_123"), + ("OPENAI_PROJECT_ID", "project_123"), + ]); + + let resolved = source.resolve().await.unwrap(); + let credential = resolved.credentials.first().unwrap(); + + assert_eq!(credential.provider, Provider::OpenAi); + assert!(credential.codex_mode); + assert_eq!( + credential.base_url.as_deref(), + Some("https://chatgpt.com/backend-api/codex") + ); + assert_eq!( + credential.extra_headers.get("ChatGPT-Account-Id"), + Some(&"acct_123".to_string()) + ); + assert_eq!(credential.project_id.as_deref(), Some("project_123")); + } +} diff --git a/lib/crates/fabro-auth/src/lib.rs b/lib/crates/fabro-auth/src/lib.rs index a34c65d1f..c845930da 100644 --- a/lib/crates/fabro-auth/src/lib.rs +++ b/lib/crates/fabro-auth/src/lib.rs @@ -1,9 +1,12 @@ mod context; mod credential; +mod credential_source; +mod env_source; mod refresh; mod resolve; mod strategy; mod vault_ext; +mod vault_source; pub mod strategies; @@ -12,13 +15,16 @@ pub use credential::{ ApiKeyHeader, AuthCredential, AuthDetails, OAuthConfig, OAuthTokens, credential_id_for, parse_credential_secret, }; +pub use credential_source::{CredentialSource, ResolvedCredentials}; +pub use env_source::EnvCredentialSource; pub use refresh::refresh_oauth_credential; pub use resolve::{ ApiCredential, CliAgentKind, CliCredential, CredentialResolver, CredentialUsage, EnvLookup, - ResolveError, ResolvedCredential, configured_providers_from_process_env, + ResolveError, ResolvedCredential, auth_issue_message, configured_providers_from_process_env, }; pub use strategy::{ AuthMethod, AuthStrategy, CODEX_AUTH_URL, CODEX_CLIENT_ID, CODEX_TOKEN_URL, codex_oauth_config, strategy_for, }; pub use vault_ext::{vault_credentials_for_provider, vault_get_credential, vault_set_credential}; +pub use vault_source::VaultCredentialSource; diff --git a/lib/crates/fabro-auth/src/resolve.rs b/lib/crates/fabro-auth/src/resolve.rs index 1c4ee1351..03643d6c8 100644 --- a/lib/crates/fabro-auth/src/resolve.rs +++ b/lib/crates/fabro-auth/src/resolve.rs @@ -37,6 +37,32 @@ pub struct ApiCredential { pub project_id: Option, } +impl ApiCredential { + /// Build an `ApiCredential` from just an API key. Picks the right + /// auth header kind for the provider (Anthropic uses `x-api-key`; + /// everyone else uses `Authorization: Bearer`). All other fields + /// default to empty. + #[must_use] + pub fn from_api_key(provider: Provider, key: String) -> Self { + let auth_header = match provider { + Provider::Anthropic => ApiKeyHeader::Custom { + name: "x-api-key".to_string(), + value: key, + }, + _ => ApiKeyHeader::Bearer(key), + }; + Self { + provider, + auth_header, + extra_headers: HashMap::new(), + base_url: None, + codex_mode: false, + org_id: None, + project_id: None, + } + } +} + #[derive(Debug, Clone, PartialEq, Eq)] pub struct CliCredential { pub env_vars: HashMap, @@ -63,6 +89,24 @@ pub enum ResolveError { RefreshTokenMissing(Provider), } +#[must_use] +pub fn auth_issue_message(provider: Provider, err: &ResolveError) -> String { + match err { + ResolveError::NotConfigured(_) => { + format!("{} is not configured", provider.display_name()) + } + ResolveError::RefreshFailed { source, .. } => format!( + "{} requires re-authentication: {}", + provider.display_name(), + source + ), + ResolveError::RefreshTokenMissing(_) => format!( + "{} requires re-authentication: refresh token missing", + provider.display_name() + ), + } +} + #[derive(Clone)] pub struct CredentialResolver { vault: Arc>, @@ -184,7 +228,6 @@ impl CredentialResolver { } fn to_api_credential(&self, vault: &Vault, credential: &AuthCredential) -> ApiCredential { - let mut extra_headers = HashMap::new(); let base_url = match credential.provider { Provider::Anthropic => self.lookup_env_or_vault(vault, "ANTHROPIC_BASE_URL"), Provider::OpenAi => self.lookup_env_or_vault(vault, "OPENAI_BASE_URL"), @@ -195,32 +238,19 @@ impl CredentialResolver { } }; match &credential.details { - AuthDetails::ApiKey { key } => ApiCredential { - provider: credential.provider, - auth_header: match credential.provider { - Provider::Anthropic => ApiKeyHeader::Custom { - name: "x-api-key".to_string(), - value: key.clone(), - }, - _ => ApiKeyHeader::Bearer(key.clone()), - }, - extra_headers, - base_url, - codex_mode: false, - org_id: if credential.provider == Provider::OpenAi { - self.lookup_env_or_vault(vault, "OPENAI_ORG_ID") - } else { - None - }, - project_id: if credential.provider == Provider::OpenAi { - self.lookup_env_or_vault(vault, "OPENAI_PROJECT_ID") - } else { - None - }, - }, + AuthDetails::ApiKey { key } => { + let mut cred = ApiCredential::from_api_key(credential.provider, key.clone()); + cred.base_url = base_url; + if credential.provider == Provider::OpenAi { + cred.org_id = self.lookup_env_or_vault(vault, "OPENAI_ORG_ID"); + cred.project_id = self.lookup_env_or_vault(vault, "OPENAI_PROJECT_ID"); + } + cred + } AuthDetails::CodexOAuth { tokens, account_id, .. } => { + let mut extra_headers = HashMap::new(); if let Some(account_id) = account_id { extra_headers.insert("ChatGPT-Account-Id".to_string(), account_id.clone()); extra_headers.insert("originator".to_string(), "fabro".to_string()); @@ -816,4 +846,35 @@ mod tests { ResolveError::RefreshTokenMissing(Provider::OpenAi) )); } + + #[test] + fn auth_issue_message_formats_refresh_token_missing() { + let message = auth_issue_message( + Provider::OpenAi, + &ResolveError::RefreshTokenMissing(Provider::OpenAi), + ); + + assert_eq!( + message, + "OpenAI requires re-authentication: refresh token missing" + ); + } + + #[test] + fn api_credential_debug_redacts_secret_material() { + let credential = ApiCredential { + provider: Provider::OpenAi, + auth_header: ApiKeyHeader::Bearer("sk-test".to_string()), + extra_headers: HashMap::new(), + base_url: None, + codex_mode: false, + org_id: None, + project_id: None, + }; + + let debug = format!("{credential:?}"); + + assert!(!debug.contains("sk-test")); + assert!(debug.contains("REDACTED")); + } } diff --git a/lib/crates/fabro-auth/src/vault_source.rs b/lib/crates/fabro-auth/src/vault_source.rs new file mode 100644 index 000000000..052171fc1 --- /dev/null +++ b/lib/crates/fabro-auth/src/vault_source.rs @@ -0,0 +1,185 @@ +use std::sync::Arc; + +use async_trait::async_trait; +use fabro_model::Provider; +use fabro_vault::Vault; +use tokio::sync::RwLock as AsyncRwLock; + +use crate::credential_source::{CredentialSource, ResolvedCredentials}; +use crate::{CredentialResolver, CredentialUsage, EnvLookup, ResolveError, ResolvedCredential}; + +#[derive(Clone)] +pub struct VaultCredentialSource { + vault: Arc>, + resolver: CredentialResolver, +} + +impl VaultCredentialSource { + #[must_use] + pub fn new(vault: Arc>) -> Self { + let resolver = CredentialResolver::new(Arc::clone(&vault)); + Self { vault, resolver } + } + + #[must_use] + pub fn with_env_lookup(vault: Arc>, env_lookup: F) -> Self + where + F: Fn(&str) -> Option + Send + Sync + 'static, + { + let env_lookup: EnvLookup = Arc::new(env_lookup); + let resolver = CredentialResolver::with_env_lookup(Arc::clone(&vault), env_lookup); + Self { vault, resolver } + } +} + +impl std::fmt::Debug for VaultCredentialSource { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("VaultCredentialSource") + .finish_non_exhaustive() + } +} + +#[async_trait] +impl CredentialSource for VaultCredentialSource { + async fn resolve(&self) -> anyhow::Result { + let mut credentials = Vec::new(); + let mut auth_issues = Vec::new(); + + for provider in Provider::ALL { + match self + .resolver + .resolve(*provider, CredentialUsage::ApiRequest) + .await + { + Ok(ResolvedCredential::Api(credential)) => credentials.push(credential), + Ok(ResolvedCredential::Cli(_)) | Err(ResolveError::NotConfigured(_)) => {} + Err(err) => auth_issues.push((*provider, err)), + } + } + + Ok(ResolvedCredentials { + credentials, + auth_issues, + }) + } + + async fn configured_providers(&self) -> Vec { + let vault = self.vault.read().await; + self.resolver.configured_providers(&vault) + } +} + +#[cfg(test)] +mod tests { + use std::sync::Arc; + + use chrono::{Duration, Utc}; + use fabro_model::Provider; + use fabro_vault::{SecretType, Vault}; + use tokio::sync::RwLock as AsyncRwLock; + + use super::VaultCredentialSource; + use crate::credential::{AuthCredential, AuthDetails, OAuthConfig, OAuthTokens}; + use crate::{CredentialSource, ResolveError}; + + fn api_key_credential(provider: Provider, key: &str) -> AuthCredential { + AuthCredential { + provider, + details: AuthDetails::ApiKey { + key: key.to_string(), + }, + } + } + + fn expired_openai_credential() -> AuthCredential { + AuthCredential { + provider: Provider::OpenAi, + details: AuthDetails::CodexOAuth { + tokens: OAuthTokens { + access_token: "expired-access".to_string(), + refresh_token: Some("refresh-token".to_string()), + expires_at: Utc::now() - Duration::hours(1), + }, + config: OAuthConfig { + auth_url: "https://auth.openai.com".to_string(), + token_url: "http://127.0.0.1:9/oauth/token".to_string(), + client_id: "client".to_string(), + scopes: vec!["openid".to_string()], + redirect_uri: Some("https://example.com/callback".to_string()), + use_pkce: true, + }, + account_id: Some("acct_123".to_string()), + }, + } + } + + #[tokio::test] + async fn resolve_returns_credentials_and_auth_issues() { + let dir = tempfile::tempdir().unwrap(); + let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); + vault + .set( + "openai_codex", + &serde_json::to_string(&expired_openai_credential()).unwrap(), + SecretType::Credential, + None, + ) + .unwrap(); + vault + .set( + "anthropic", + &serde_json::to_string(&api_key_credential(Provider::Anthropic, "anthropic-key")) + .unwrap(), + SecretType::Credential, + None, + ) + .unwrap(); + + let source = + VaultCredentialSource::with_env_lookup(Arc::new(AsyncRwLock::new(vault)), |_| None); + + let resolved = source.resolve().await.unwrap(); + + assert_eq!(resolved.credentials.len(), 1); + assert_eq!(resolved.credentials[0].provider, Provider::Anthropic); + assert_eq!(resolved.auth_issues.len(), 1); + assert!(matches!( + resolved.auth_issues[0].1, + ResolveError::RefreshFailed { + provider: Provider::OpenAi, + .. + } + )); + } + + #[tokio::test] + async fn configured_providers_reads_from_vault_without_refreshing() { + let dir = tempfile::tempdir().unwrap(); + let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); + vault + .set( + "openai", + &serde_json::to_string(&api_key_credential(Provider::OpenAi, "openai-key")) + .unwrap(), + SecretType::Credential, + None, + ) + .unwrap(); + vault + .set( + "anthropic", + &serde_json::to_string(&api_key_credential(Provider::Anthropic, "anthropic-key")) + .unwrap(), + SecretType::Credential, + None, + ) + .unwrap(); + let source = + VaultCredentialSource::with_env_lookup(Arc::new(AsyncRwLock::new(vault)), |_| None); + + assert_eq!(source.configured_providers().await, vec![ + Provider::Anthropic, + Provider::OpenAi + ]); + } +} diff --git a/lib/crates/fabro-cli/src/command_context.rs b/lib/crates/fabro-cli/src/command_context.rs index 4e79dbf48..2dbde1f21 100644 --- a/lib/crates/fabro-cli/src/command_context.rs +++ b/lib/crates/fabro-cli/src/command_context.rs @@ -2,12 +2,14 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; use anyhow::{Context as _, Result, bail}; -use fabro_config::CliLayer; +use fabro_auth::{CredentialSource, EnvCredentialSource, VaultCredentialSource}; +use fabro_config::{CliLayer, Storage}; use fabro_types::settings::RunNamespace; use fabro_types::settings::cli::{OutputFormat, OutputVerbosity}; use fabro_types::{ServerSettings, UserSettings}; use fabro_util::printer::Printer; -use tokio::sync::OnceCell; +use fabro_vault::Vault; +use tokio::sync::{OnceCell, RwLock as AsyncRwLock}; use crate::args::{ ServerConnectionArgs, ServerTargetArgs, printer_from_verbosity, require_no_json_override, @@ -40,6 +42,7 @@ pub(crate) struct CommandContext { user_settings: UserSettings, server_mode: ServerMode, server: OnceCell>, + llm_source: OnceCell>, } struct ResolvedCommandSettings { @@ -68,6 +71,7 @@ impl CommandContext { user_settings: resolved_settings.user_settings, server_mode: ServerMode::None, server: OnceCell::new(), + llm_source: OnceCell::new(), }) } @@ -160,6 +164,26 @@ impl CommandContext { Ok(Arc::clone(client)) } + pub(crate) async fn llm_source(&self) -> Result> { + let storage_dir = self.storage_dir.clone(); + + let source = self + .llm_source + .get_or_try_init(|| async move { + let source: Arc = + match Vault::load(Storage::new(&storage_dir).secrets_path()) { + Ok(vault) => Arc::new(VaultCredentialSource::new(Arc::new( + AsyncRwLock::new(vault), + ))), + Err(_) => Arc::new(EnvCredentialSource::new()), + }; + Ok::, anyhow::Error>(source) + }) + .await?; + + Ok(Arc::clone(source)) + } + fn with_server_mode(&self, server_mode: ServerMode) -> Result { // Always reload settings for the requested derivation mode so the result // depends only on the requested mode, not on whichever derived context @@ -178,6 +202,7 @@ impl CommandContext { user_settings: resolved_settings.user_settings, server_mode, server: OnceCell::new(), + llm_source: OnceCell::new(), }) } } @@ -251,6 +276,7 @@ mod tests { user_settings: resolved_settings.user_settings, server_mode: ServerMode::None, server: OnceCell::new(), + llm_source: OnceCell::new(), } } diff --git a/lib/crates/fabro-cli/src/commands/exec.rs b/lib/crates/fabro-cli/src/commands/exec.rs index 1404767a4..5d3975421 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, @@ -330,12 +330,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/src/commands/pr/create.rs b/lib/crates/fabro-cli/src/commands/pr/create.rs index 109fe90a9..6cdadbf1e 100644 --- a/lib/crates/fabro-cli/src/commands/pr/create.rs +++ b/lib/crates/fabro-cli/src/commands/pr/create.rs @@ -1,14 +1,8 @@ -use std::sync::Arc; - use anyhow::{Context, Result, bail}; -use fabro_auth::configured_providers_from_process_env; -use fabro_config::Storage; use fabro_model::Catalog; use fabro_sandbox::daytona::detect_repo_info; -use fabro_vault::Vault; use fabro_workflow::outcome::StageStatus; use fabro_workflow::pull_request::maybe_open_pull_request; -use tokio::sync::RwLock as AsyncRwLock; use tracing::info; use crate::args::PrCreateArgs; @@ -97,17 +91,15 @@ pub(super) async fn create_command(args: PrCreateArgs, base_ctx: &CommandContext ); } - let vault = Vault::load(Storage::new(ctx.storage_dir()).secrets_path()) - .ok() - .map(|vault| Arc::new(AsyncRwLock::new(vault))); - let configured = configured_providers_from_process_env(vault.as_ref()).await; + let llm_source = ctx.llm_source().await?; + let configured = llm_source.configured_providers().await; let model = args.model.unwrap_or_else(|| { Catalog::builtin() .default_for_configured(&configured) .id .clone() }); - + let run_store_handle = run_store.clone().into(); let pull_request = maybe_open_pull_request( &creds, &origin_url, @@ -118,7 +110,8 @@ pub(super) async fn create_command(args: PrCreateArgs, base_ctx: &CommandContext &model, true, None, - &run_store.clone().into(), + &run_store_handle, + llm_source.as_ref(), None, ) .await diff --git a/lib/crates/fabro-cli/src/shared/provider_auth.rs b/lib/crates/fabro-cli/src/shared/provider_auth.rs index 296a36234..a3ed147ac 100644 --- a/lib/crates/fabro-cli/src/shared/provider_auth.rs +++ b/lib/crates/fabro-cli/src/shared/provider_auth.rs @@ -15,8 +15,8 @@ use dialoguer::console::Term; use dialoguer::theme::ColorfulTheme; use dialoguer::{Confirm, Password}; use fabro_auth::{ - ApiCredential, ApiKeyHeader, AuthContextRequest, AuthContextResponse, AuthCredential, - AuthMethod, codex_oauth_config, strategy_for, + ApiCredential, AuthContextRequest, AuthContextResponse, AuthCredential, AuthMethod, + codex_oauth_config, strategy_for, }; use fabro_llm::client::Client as LlmClient; use fabro_llm::generate::{GenerateParams, generate}; @@ -78,23 +78,10 @@ pub(crate) enum ApiKeySource { // --------------------------------------------------------------------------- pub(crate) async fn validate_api_key(provider: Provider, api_key: &str) -> Result<(), String> { - let auth_header = if provider == Provider::Anthropic { - ApiKeyHeader::Custom { - name: "x-api-key".to_string(), - value: api_key.to_string(), - } - } else { - ApiKeyHeader::Bearer(api_key.to_string()) - }; - let client = LlmClient::from_credentials(vec![ApiCredential { + let client = LlmClient::from_credentials(vec![ApiCredential::from_api_key( provider, - auth_header, - extra_headers: std::collections::HashMap::new(), - base_url: None, - codex_mode: false, - org_id: None, - project_id: None, - }]) + api_key.to_string(), + )]) .await .map_err(|e| e.to_string())?; @@ -103,11 +90,10 @@ pub(crate) async fn validate_api_key(provider: Provider, api_key: &str) -> Resul |model| model.id.clone(), ); - let params = GenerateParams::new(probe_model) + let params = GenerateParams::new(probe_model, Arc::new(client)) .provider(provider.as_str()) .prompt("Say OK") - .max_tokens(16) - .client(Arc::new(client)); + .max_tokens(16); timeout(std::time::Duration::from_secs(30), generate(params)) .await 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-hooks/Cargo.toml b/lib/crates/fabro-hooks/Cargo.toml index eed88ed19..4875720dc 100644 --- a/lib/crates/fabro-hooks/Cargo.toml +++ b/lib/crates/fabro-hooks/Cargo.toml @@ -14,6 +14,7 @@ workspace = true [dependencies] fabro-agent = { path = "../fabro-agent" } +fabro-auth = { path = "../fabro-auth" } fabro-config = { path = "../fabro-config" } fabro-llm = { path = "../fabro-llm" } fabro-model = { path = "../fabro-model" } diff --git a/lib/crates/fabro-hooks/src/bridge.rs b/lib/crates/fabro-hooks/src/bridge.rs index 0c51172a5..a762a53d7 100644 --- a/lib/crates/fabro-hooks/src/bridge.rs +++ b/lib/crates/fabro-hooks/src/bridge.rs @@ -95,6 +95,7 @@ mod tests { context: &HookContext, _sandbox: Arc, _work_dir: Option<&Path>, + _llm_source: &dyn fabro_auth::CredentialSource, ) -> HookResult { self.captured_contexts.lock().unwrap().push(context.clone()); HookResult { diff --git a/lib/crates/fabro-hooks/src/executor.rs b/lib/crates/fabro-hooks/src/executor.rs index e69ad09ae..dbdc86021 100644 --- a/lib/crates/fabro-hooks/src/executor.rs +++ b/lib/crates/fabro-hooks/src/executor.rs @@ -8,6 +8,7 @@ use std::time::Instant; use async_trait::async_trait; use fabro_agent::Sandbox; use fabro_agent::tool_registry::ToolContext; +use fabro_auth::CredentialSource; use fabro_llm::client::Client as LlmClient; use fabro_llm::generate::{GenerateParams, generate_object}; use fabro_llm::types::{Message, Request, ToolResult}; @@ -48,6 +49,7 @@ pub trait HookExecutor: Send + Sync { context: &HookContext, sandbox: Arc, work_dir: Option<&Path>, + llm_source: &dyn CredentialSource, ) -> HookResult; } @@ -288,6 +290,7 @@ impl HookExecutorImpl { model: Option<&str>, context: &HookContext, env: &E, + llm_source: &dyn CredentialSource, ) -> HookDecision where E: Env + Clone + Send + Sync + fmt::Debug + 'static, @@ -301,7 +304,15 @@ impl HookExecutorImpl { let user_msg = Self::build_hook_user_message(&prompt, context); Self::execute_llm_with_timeout(definition.timeout(), "prompt", || async move { - let params = GenerateParams::new(&resolved_model) + let client = match LlmClient::from_source(llm_source).await { + Ok(client) => Arc::new(client), + Err(e) => { + tracing::warn!(error = %e, "prompt hook client creation failed, proceeding"); + return HookDecision::Proceed; + } + }; + + let params = GenerateParams::new(&resolved_model, client) .system(HOOK_EVALUATOR_SYSTEM_PROMPT) .prompt(user_msg) .max_tokens(1024); @@ -342,6 +353,7 @@ impl HookExecutorImpl { context: &HookContext, sandbox: Arc, env: &E, + llm_source: &dyn CredentialSource, ) -> HookDecision where E: Env + Clone + Send + Sync + fmt::Debug + 'static, @@ -355,7 +367,7 @@ impl HookExecutorImpl { let user_msg = Self::build_hook_user_message(&prompt, context); Self::execute_llm_with_timeout(definition.timeout(), "agent", || async move { - let client = match LlmClient::from_env().await { + let client = match LlmClient::from_source(llm_source).await { Ok(c) => c, Err(e) => { tracing::warn!(error = %e, "agent hook client creation failed, proceeding"); @@ -604,6 +616,7 @@ impl HookExecutor for HookExecutorImpl { context: &HookContext, sandbox: Arc, work_dir: Option<&Path>, + llm_source: &dyn CredentialSource, ) -> HookResult { use std::sync::OnceLock; static HTTP_CLIENTS: OnceLock = OnceLock::new(); @@ -654,7 +667,17 @@ impl HookExecutor for HookExecutorImpl { ref prompt, ref model, }), - ) => Self::execute_prompt(definition, prompt, model.as_deref(), context, &env).await, + ) => { + Self::execute_prompt( + definition, + prompt, + model.as_deref(), + context, + &env, + llm_source, + ) + .await + } Some( Cow::Borrowed(HookType::Agent { ref prompt, @@ -675,6 +698,7 @@ impl HookExecutor for HookExecutorImpl { context, sandbox, &env, + llm_source, ) .await } @@ -694,6 +718,7 @@ impl HookExecutor for HookExecutorImpl { #[cfg(test)] mod tests { + use fabro_auth::{CredentialSource, EnvCredentialSource}; use fabro_types::fixtures; use fabro_util::env::TestEnv; @@ -711,6 +736,10 @@ mod tests { )) } + fn test_llm_source() -> Arc { + Arc::new(EnvCredentialSource::new()) + } + fn test_http_client() -> fabro_http::HttpClient { HookExecutorImpl::build_http_client(TlsMode::Off) } @@ -791,7 +820,10 @@ mod tests { let def = make_definition("exit 0"); let ctx = make_context(); let sandbox = make_sandbox(); - let result = executor.execute(&def, &ctx, sandbox, None).await; + let source = test_llm_source(); + let result = executor + .execute(&def, &ctx, sandbox, None, source.as_ref()) + .await; assert_eq!(result.decision, HookDecision::Proceed); assert_eq!(result.hook_name.as_deref(), Some("test-hook")); } @@ -802,7 +834,10 @@ mod tests { let def = make_definition("exit 1"); let ctx = make_context(); let sandbox = make_sandbox(); - let result = executor.execute(&def, &ctx, sandbox, None).await; + let source = test_llm_source(); + let result = executor + .execute(&def, &ctx, sandbox, None, source.as_ref()) + .await; assert!(matches!(result.decision, HookDecision::Block { .. })); } @@ -812,7 +847,10 @@ mod tests { let def = make_definition("exit 2"); let ctx = make_context(); let sandbox = make_sandbox(); - let result = executor.execute(&def, &ctx, sandbox, None).await; + let source = test_llm_source(); + let result = executor + .execute(&def, &ctx, sandbox, None, source.as_ref()) + .await; assert!(matches!(result.decision, HookDecision::Block { .. })); } @@ -822,7 +860,10 @@ mod tests { let def = make_definition(r#"echo '{"decision": "skip", "reason": "test skip"}'"#); let ctx = make_context(); let sandbox = make_sandbox(); - let result = executor.execute(&def, &ctx, sandbox, None).await; + let source = test_llm_source(); + let result = executor + .execute(&def, &ctx, sandbox, None, source.as_ref()) + .await; assert_eq!(result.decision, HookDecision::Skip { reason: Some("test skip".into()), }); @@ -836,7 +877,10 @@ mod tests { let mut ctx = make_context(); ctx.node_id = Some("plan".into()); let sandbox = make_sandbox(); - let result = executor.execute(&def, &ctx, sandbox, None).await; + let source = test_llm_source(); + let result = executor + .execute(&def, &ctx, sandbox, None, source.as_ref()) + .await; assert_eq!(result.decision, HookDecision::Proceed); } @@ -855,7 +899,10 @@ mod tests { }; let ctx = make_context(); let sandbox = make_sandbox(); - let result = executor.execute(&def, &ctx, sandbox, None).await; + let source = test_llm_source(); + let result = executor + .execute(&def, &ctx, sandbox, None, source.as_ref()) + .await; assert!(matches!(result.decision, HookDecision::Block { .. })); } @@ -1247,7 +1294,10 @@ mod tests { }; let ctx = make_context(); let sandbox = make_sandbox(); - let result = executor.execute(&def, &ctx, sandbox, None).await; + let source = test_llm_source(); + let result = executor + .execute(&def, &ctx, sandbox, None, source.as_ref()) + .await; mock.assert_async().await; assert_eq!(result.decision, HookDecision::Proceed); @@ -1278,6 +1328,7 @@ mod tests { None, &make_context(), &test_env(&[]), + test_llm_source().as_ref(), ) .await; @@ -1294,6 +1345,7 @@ mod tests { &make_context(), make_sandbox(), &test_env(&[]), + test_llm_source().as_ref(), ) .await; diff --git a/lib/crates/fabro-hooks/src/runner.rs b/lib/crates/fabro-hooks/src/runner.rs index 5c617dcab..110b3d58c 100644 --- a/lib/crates/fabro-hooks/src/runner.rs +++ b/lib/crates/fabro-hooks/src/runner.rs @@ -3,6 +3,9 @@ use std::path::Path; use std::sync::Arc; use fabro_agent::Sandbox; +use fabro_auth::CredentialSource; +#[cfg(test)] +use fabro_auth::EnvCredentialSource; use crate::config::{HookDefinition, HookSettings}; use crate::executor::{HookExecutor, HookExecutorImpl}; @@ -13,17 +16,19 @@ use crate::types::{HookContext, HookDecision}; pub struct HookRunner { config: HookSettings, executor: Arc, + llm_source: Arc, /// Pre-compiled regexes keyed by matcher pattern string. compiled_matchers: HashMap, } impl HookRunner { #[must_use] - pub fn new(config: HookSettings) -> Self { + pub fn new(config: HookSettings, llm_source: Arc) -> Self { let compiled_matchers = Self::compile_matchers(&config); Self { config, executor: Arc::new(HookExecutorImpl), + llm_source, compiled_matchers, } } @@ -35,6 +40,7 @@ impl HookRunner { Self { config, executor, + llm_source: Arc::new(EnvCredentialSource::new()), compiled_matchers, } } @@ -140,7 +146,13 @@ impl HookRunner { ); let result = self .executor - .execute(hook, context, sandbox.clone(), work_dir) + .execute( + hook, + context, + sandbox.clone(), + work_dir, + self.llm_source.as_ref(), + ) .await; tracing::debug!( hook = %hook.effective_name(), @@ -188,7 +200,13 @@ impl HookRunner { ); let result = self .executor - .execute(hook, context, sandbox.clone(), work_dir) + .execute( + hook, + context, + sandbox.clone(), + work_dir, + self.llm_source.as_ref(), + ) .await; tracing::debug!( hook = %hook.effective_name(), @@ -211,6 +229,7 @@ impl HookRunner { #[cfg(test)] mod tests { + use fabro_auth::EnvCredentialSource; use fabro_types::fixtures; use super::*; @@ -229,6 +248,7 @@ mod tests { _context: &HookContext, _sandbox: Arc, _work_dir: Option<&Path>, + _llm_source: &dyn CredentialSource, ) -> HookResult { HookResult { hook_name: definition.name.clone(), @@ -248,6 +268,10 @@ mod tests { HookContext::new(event, fixtures::RUN_1, "test-wf".into()) } + fn test_llm_source() -> Arc { + Arc::new(EnvCredentialSource::new()) + } + fn make_hook(event: HookEvent, name: &str) -> HookDefinition { HookDefinition { name: Some(name.into()), @@ -263,7 +287,7 @@ mod tests { #[tokio::test] async fn no_hooks_returns_proceed() { - let runner = HookRunner::new(HookSettings::default()); + let runner = HookRunner::new(HookSettings::default(), test_llm_source()); let ctx = make_context(HookEvent::RunStart); let sandbox = make_sandbox(); let decision = runner.run(&ctx, sandbox.clone(), None).await; @@ -428,7 +452,7 @@ mod tests { h }], }; - let runner = HookRunner::new(config); + let runner = HookRunner::new(config, test_llm_source()); let ctx = make_context(HookEvent::RunStart); let sandbox = make_sandbox(); let decision = runner.run(&ctx, sandbox.clone(), None).await; @@ -444,7 +468,7 @@ mod tests { h }], }; - let runner = HookRunner::new(config); + let runner = HookRunner::new(config, test_llm_source()); let ctx = make_context(HookEvent::RunStart); let sandbox = make_sandbox(); let decision = runner.run(&ctx, sandbox.clone(), None).await; diff --git a/lib/crates/fabro-llm/README.md b/lib/crates/fabro-llm/README.md index cce0c9818..56d08a229 100644 --- a/lib/crates/fabro-llm/README.md +++ b/lib/crates/fabro-llm/README.md @@ -1,10 +1,10 @@ -# unified-llm +# fabro-llm A unified async Rust client library for multiple LLM providers. Write your LLM integration code once and switch between Anthropic, OpenAI, and Google Gemini without changing your application logic. ## Key concepts -- **Client** -- Routes requests to registered provider adapters. Can be created explicitly or auto-configured from environment variables. +- **Client** -- Routes requests to registered provider adapters. Build it from a `CredentialSource` or explicit typed credentials. - **ProviderAdapter** -- The trait every provider implements (`complete` and `stream`). Built-in adapters: `AnthropicAdapter`, `OpenAiAdapter`, `GeminiAdapter`, `OpenAiCompatibleAdapter`. - **Middleware** -- Intercepts requests/responses for logging, caching, or transformation. Supports both blocking and streaming paths. - **generate()** -- High-level function that wraps `Client.complete()` with automatic tool execution loops, retries, timeouts, and cancellation. @@ -24,13 +24,15 @@ All adapters support streaming, tool calling, structured output (`response_forma ## Usage -### Auto-configure from environment +### Create from an environment-backed credential source ```rust -use unified_llm::client::Client; -use unified_llm::types::{Message, Request}; +use fabro_auth::EnvCredentialSource; +use fabro_llm::client::Client; +use fabro_llm::types::{Message, Request}; -let client = Client::from_env().await?; +let source = EnvCredentialSource::new(); +let client = Client::from_source(&source).await?; let request = Request { model: "claude-sonnet-4-5".to_string(), @@ -55,10 +57,14 @@ println!("{}", response.text()); ### High-level generate() ```rust -use unified_llm::generate::{generate, GenerateParams}; +use fabro_auth::EnvCredentialSource; +use fabro_llm::client::Client; +use fabro_llm::generate::{generate, GenerateParams}; +let source = EnvCredentialSource::new(); +let client = Client::from_source(&source).await?; let result = generate( - GenerateParams::new("claude-sonnet-4-5") + GenerateParams::new("claude-sonnet-4-5", client.clone()) .prompt("Explain monads in one sentence") .system("You are a concise programming tutor.") .max_tokens(200) @@ -70,10 +76,14 @@ println!("{}", result.text()); ### Tool calling ```rust -use unified_llm::generate::{generate, GenerateParams}; -use unified_llm::tools::Tool; +use fabro_auth::EnvCredentialSource; +use fabro_llm::client::Client; +use fabro_llm::generate::{generate, GenerateParams}; +use fabro_llm::tools::Tool; use std::sync::Arc; +let source = EnvCredentialSource::new(); +let client = Client::from_source(&source).await?; let weather_tool = Tool::active( "get_weather", "Get the current weather for a city", @@ -91,7 +101,7 @@ let weather_tool = Tool::active( ); let result = generate( - GenerateParams::new("claude-sonnet-4-5") + GenerateParams::new("claude-sonnet-4-5", client.clone()) .prompt("What's the weather in San Francisco?") .tools(vec![weather_tool]) .max_tool_rounds(3) @@ -101,11 +111,13 @@ let result = generate( ### Streaming ```rust -use unified_llm::client::Client; -use unified_llm::types::{Message, Request, StreamEvent}; +use fabro_auth::EnvCredentialSource; +use fabro_llm::client::Client; +use fabro_llm::types::{Message, Request, StreamEvent}; use futures::StreamExt; -let client = Client::from_env().await?; +let source = EnvCredentialSource::new(); +let client = Client::from_source(&source).await?; let request = Request { model: "claude-sonnet-4-5".to_string(), messages: vec![Message::user("Tell me a joke")], @@ -131,10 +143,10 @@ while let Some(event) = stream.next().await { ### Middleware ```rust -use unified_llm::middleware::{Middleware, NextFn, NextStreamFn}; -use unified_llm::types::{Request, Response}; -use unified_llm::provider::StreamEventStream; -use unified_llm::error::SdkError; +use fabro_llm::error::Error; +use fabro_llm::middleware::{Middleware, NextFn, NextStreamFn}; +use fabro_llm::provider::StreamEventStream; +use fabro_llm::types::{Request, Response}; struct LoggingMiddleware; @@ -144,7 +156,7 @@ impl Middleware for LoggingMiddleware { &self, request: Request, next: NextFn, - ) -> Result { + ) -> Result { eprintln!("Request to model: {}", request.model); let response = next(request).await?; eprintln!("Response tokens: {}", response.usage.total_tokens); @@ -155,7 +167,7 @@ impl Middleware for LoggingMiddleware { &self, request: Request, next: NextStreamFn, - ) -> Result { + ) -> Result { next(request).await } } @@ -164,7 +176,7 @@ impl Middleware for LoggingMiddleware { ### OpenAI-compatible providers ```rust -use unified_llm::providers::OpenAiCompatibleAdapter; +use fabro_llm::providers::OpenAiCompatibleAdapter; use std::sync::Arc; let adapter = OpenAiCompatibleAdapter::new("your-api-key", "https://api.groq.com/openai/v1") @@ -174,7 +186,7 @@ let adapter = OpenAiCompatibleAdapter::new("your-api-key", "https://api.groq.com ### Model catalog ```rust -use unified_llm::catalog::{get_model_info, list_models, get_latest_model}; +use fabro_llm::catalog::{get_latest_model, get_model_info, list_models}; let info = get_model_info("claude-opus-4-6"); let anthropic_models = list_models(Some("anthropic")); @@ -213,7 +225,7 @@ The `retry()` function and `generate()` respect `Retry-After` headers and use ex Pass provider-specific parameters via `provider_options` without losing portability: ```rust -use unified_llm::types::Request; +use fabro_llm::types::Request; let request = Request { provider_options: Some(serde_json::json!({ diff --git a/lib/crates/fabro-llm/src/client.rs b/lib/crates/fabro-llm/src/client.rs index f589c47be..865cff207 100644 --- a/lib/crates/fabro-llm/src/client.rs +++ b/lib/crates/fabro-llm/src/client.rs @@ -1,7 +1,7 @@ use std::collections::HashMap; use std::sync::Arc; -use fabro_auth::{ApiCredential, ApiKeyHeader}; +use fabro_auth::{ApiCredential, ApiKeyHeader, CredentialSource}; use tracing::debug; use crate::error::Error; @@ -38,107 +38,18 @@ impl Client { } } - /// Create a Client from environment variables (Section 2.2). - /// Registers providers whose API keys are present in the environment. - /// The first registered provider becomes the default. + /// Create a Client from a credential source. /// /// # Errors /// - /// Returns `Error` if any provider adapter fails to initialize. - pub async fn from_env() -> Result { - let mut credentials = Vec::new(); - if let Ok(key) = std::env::var("ANTHROPIC_API_KEY") { - credentials.push(ApiCredential { - provider: fabro_model::Provider::Anthropic, - auth_header: ApiKeyHeader::Custom { - name: "x-api-key".to_string(), - value: key, - }, - extra_headers: HashMap::new(), - base_url: std::env::var("ANTHROPIC_BASE_URL").ok(), - codex_mode: false, - org_id: None, - project_id: None, - }); - } - if let Ok(key) = std::env::var("OPENAI_API_KEY") { - let mut extra_headers = HashMap::new(); - let mut base_url = std::env::var("OPENAI_BASE_URL").ok(); - let mut codex_mode = false; - if let Ok(account_id) = std::env::var("CHATGPT_ACCOUNT_ID") { - base_url = Some("https://chatgpt.com/backend-api/codex".to_string()); - codex_mode = true; - extra_headers.insert("ChatGPT-Account-Id".to_string(), account_id); - extra_headers.insert("originator".to_string(), "fabro".to_string()); - } - credentials.push(ApiCredential { - provider: fabro_model::Provider::OpenAi, - auth_header: ApiKeyHeader::Bearer(key), - extra_headers, - base_url, - codex_mode, - org_id: std::env::var("OPENAI_ORG_ID").ok(), - project_id: std::env::var("OPENAI_PROJECT_ID").ok(), - }); - } - if let Ok(key) = - std::env::var("GEMINI_API_KEY").or_else(|_| std::env::var("GOOGLE_API_KEY")) - { - credentials.push(ApiCredential { - provider: fabro_model::Provider::Gemini, - auth_header: ApiKeyHeader::Bearer(key), - extra_headers: HashMap::new(), - base_url: std::env::var("GEMINI_BASE_URL").ok(), - codex_mode: false, - org_id: None, - project_id: None, - }); - } - if let Ok(key) = std::env::var("KIMI_API_KEY") { - credentials.push(ApiCredential { - provider: fabro_model::Provider::Kimi, - auth_header: ApiKeyHeader::Bearer(key), - extra_headers: HashMap::new(), - base_url: None, - codex_mode: false, - org_id: None, - project_id: None, - }); - } - if let Ok(key) = std::env::var("ZAI_API_KEY") { - credentials.push(ApiCredential { - provider: fabro_model::Provider::Zai, - auth_header: ApiKeyHeader::Bearer(key), - extra_headers: HashMap::new(), - base_url: None, - codex_mode: false, - org_id: None, - project_id: None, - }); - } - if let Ok(key) = std::env::var("MINIMAX_API_KEY") { - credentials.push(ApiCredential { - provider: fabro_model::Provider::Minimax, - auth_header: ApiKeyHeader::Bearer(key), - extra_headers: HashMap::new(), - base_url: None, - codex_mode: false, - org_id: None, - project_id: None, - }); - } - if let Ok(key) = std::env::var("INCEPTION_API_KEY") { - credentials.push(ApiCredential { - provider: fabro_model::Provider::Inception, - auth_header: ApiKeyHeader::Bearer(key), - extra_headers: HashMap::new(), - base_url: None, - codex_mode: false, - org_id: None, - project_id: None, - }); - } - Self::from_credentials(credentials).await + /// Returns `Error` if the source cannot resolve credentials or any provider + /// adapter fails to initialize. + pub async fn from_source(source: &dyn CredentialSource) -> Result { + let resolved = source.resolve().await.map_err(|err| Error::Configuration { + message: format!("Failed to resolve LLM credentials: {err}"), + source: None, + })?; + Self::from_credentials(resolved.credentials).await } /// Create a Client from typed provider credentials. @@ -418,6 +329,8 @@ fn auth_value(auth_header: &ApiKeyHeader) -> String { #[cfg(test)] mod tests { + use async_trait::async_trait; + use fabro_auth::{CredentialSource, ResolvedCredentials}; use futures::stream; use super::*; @@ -506,6 +419,27 @@ mod tests { } } + struct StubSource { + credentials: Vec, + } + + #[async_trait] + impl CredentialSource for StubSource { + async fn resolve(&self) -> anyhow::Result { + Ok(ResolvedCredentials { + credentials: self.credentials.clone(), + auth_issues: Vec::new(), + }) + } + + async fn configured_providers(&self) -> Vec { + self.credentials + .iter() + .map(|credential| credential.provider) + .collect() + } + } + #[tokio::test] async fn complete_routes_to_default_provider() { let mut client = Client::new(HashMap::new(), None, vec![]); @@ -612,6 +546,39 @@ mod tests { assert_eq!(client.default_provider(), Some("kimi")); } + #[tokio::test] + async fn from_source_registers_provider_from_resolved_credentials() { + let source = StubSource { + credentials: vec![ApiCredential { + provider: fabro_model::Provider::Anthropic, + auth_header: ApiKeyHeader::Custom { + name: "x-api-key".to_string(), + value: "anthropic-key".to_string(), + }, + extra_headers: HashMap::new(), + base_url: None, + codex_mode: false, + org_id: None, + project_id: None, + }], + }; + + let client = Client::from_source(&source).await.unwrap(); + + assert_eq!(client.provider_names(), vec!["anthropic"]); + } + + #[tokio::test] + async fn from_source_supports_empty_credentials() { + let source = StubSource { + credentials: Vec::new(), + }; + + let client = Client::from_source(&source).await.unwrap(); + + assert!(client.provider_names().is_empty()); + } + #[tokio::test] async fn register_sets_first_as_default() { let mut client = Client::new(HashMap::new(), None, vec![]); diff --git a/lib/crates/fabro-llm/src/generate.rs b/lib/crates/fabro-llm/src/generate.rs index f73c42b18..798e434c5 100644 --- a/lib/crates/fabro-llm/src/generate.rs +++ b/lib/crates/fabro-llm/src/generate.rs @@ -4,7 +4,7 @@ use std::task::{Context, Poll}; use fabro_util::backoff::BackoffPolicy; use futures::{Stream, StreamExt, future, stream}; -use tokio::sync::{OnceCell, mpsc}; +use tokio::sync::mpsc; use tokio::time; use tokio_stream::wrappers::ReceiverStream; use tokio_util::sync::CancellationToken; @@ -21,24 +21,6 @@ use crate::types::{ TokenCounts, ToolCall, ToolChoice, ToolDefinition, }; -/// Module-level default client (Section 2.5). -static DEFAULT_CLIENT: OnceCell> = OnceCell::const_new(); - -/// Set the module-level default client. -pub fn set_default_client(client: Client) { - let _ = DEFAULT_CLIENT.set(Arc::new(client)); -} - -/// Get the default client, lazily initialized from env. -async fn get_default_client() -> Result, Error> { - if let Some(client) = DEFAULT_CLIENT.get() { - return Ok(client.clone()); - } - let client = Arc::new(Client::from_env().await?); - let _ = DEFAULT_CLIENT.set(client.clone()); - Ok(client) -} - fn build_initial_messages(params: &GenerateParams) -> Result, Error> { let mut messages = Vec::new(); if let Some(system) = ¶ms.system { @@ -109,10 +91,7 @@ fn build_generate_result(steps: Vec, total_usage: TokenCounts) -> Ge /// Panics if a tool's `execute` handler is `None` when matched during tool /// execution. pub async fn generate(params: GenerateParams) -> Result { - let client = match params.client.clone() { - Some(c) => c, - None => get_default_client().await?, - }; + let client = Arc::clone(¶ms.client); let retry_policy = RetryPolicy { max_retries: params.max_retries, backoff: BackoffPolicy { @@ -307,7 +286,7 @@ pub struct GenerateParams { pub metadata: Option>, pub max_retries: u32, pub timeout: Option, - pub client: Option>, + pub client: Arc, /// Cancellation token to interrupt generation (Section 4.8). pub abort_signal: Option, /// Custom stop condition checked after each tool round (Section 4.3). @@ -317,30 +296,30 @@ pub struct GenerateParams { } impl GenerateParams { - pub fn new(model: impl Into) -> Self { + pub fn new(model: impl Into, client: Arc) -> Self { Self { - model: model.into(), - prompt: None, - messages: None, - system: None, - tools: None, - tool_choice: None, - max_tool_rounds: 1, - response_format: None, - temperature: None, - top_p: None, - max_tokens: None, - stop_sequences: None, + model: model.into(), + prompt: None, + messages: None, + system: None, + tools: None, + tool_choice: None, + max_tool_rounds: 1, + response_format: None, + temperature: None, + top_p: None, + max_tokens: None, + stop_sequences: None, reasoning_effort: None, - speed: None, - provider: None, + speed: None, + provider: None, provider_options: None, - metadata: None, - max_retries: 2, - timeout: None, - client: None, - abort_signal: None, - stop_when: None, + metadata: None, + max_retries: 2, + timeout: None, + client, + abort_signal: None, + stop_when: None, repair_tool_call: None, } } @@ -363,12 +342,6 @@ impl GenerateParams { self } - #[must_use] - pub fn client(mut self, client: Arc) -> Self { - self.client = Some(client); - self - } - #[must_use] pub fn tools(mut self, tools: Vec) -> Self { self.tools = Some(tools.into_iter().map(Arc::new).collect()); @@ -642,10 +615,7 @@ pub async fn stream(params: GenerateParams) -> Result { /// Returns `Error::Configuration` if both `prompt` and `messages` are set, /// or any provider error encountered during streaming setup. async fn stream_with_tool_loop(params: GenerateParams) -> Result { - let client = match params.client.clone() { - Some(c) => c, - None => get_default_client().await?, - }; + let client = Arc::clone(¶ms.client); let mut messages = build_initial_messages(¶ms)?; let tool_definitions: Option> = params .tools @@ -931,10 +901,7 @@ async fn stream_generate_raw( /// Returns `Error::Configuration` if both `prompt` and `messages` are set, /// or any provider error encountered during streaming setup. pub async fn stream_generate(params: GenerateParams) -> Result { - let client = match params.client.clone() { - Some(c) => c, - None => get_default_client().await?, - }; + let client = Arc::clone(¶ms.client); let messages = build_initial_messages(¶ms)?; let tool_definitions: Option> = params .tools @@ -1204,13 +1171,10 @@ mod tests { #[tokio::test] async fn generate_simple_text() { - let result = generate( - GenerateParams::new("mock-model") - .prompt("Hello") - .client(mock_client("Hi there!")), - ) - .await - .unwrap(); + let result = + generate(GenerateParams::new("mock-model", mock_client("Hi there!")).prompt("Hello")) + .await + .unwrap(); assert_eq!(result.text(), "Hi there!"); assert_eq!(result.finish_reason, FinishReason::Stop); @@ -1221,10 +1185,9 @@ mod tests { #[tokio::test] async fn generate_with_system_message() { let result = generate( - GenerateParams::new("mock-model") + GenerateParams::new("mock-model", mock_client("Greetings!")) .system("You are helpful") - .prompt("Hello") - .client(mock_client("Greetings!")), + .prompt("Hello"), ) .await .unwrap(); @@ -1235,13 +1198,11 @@ mod tests { #[tokio::test] async fn generate_with_messages() { let result = generate( - GenerateParams::new("mock-model") - .messages(vec![ - Message::user("Hello"), - Message::assistant("Hi"), - Message::user("How are you?"), - ]) - .client(mock_client("I'm doing well!")), + GenerateParams::new("mock-model", mock_client("I'm doing well!")).messages(vec![ + Message::user("Hello"), + Message::assistant("Hi"), + Message::user("How are you?"), + ]), ) .await .unwrap(); @@ -1255,8 +1216,8 @@ mod tests { model: "mock-model".into(), prompt: Some("Hello".into()), messages: Some(vec![Message::user("World")]), - client: Some(mock_client("test")), - ..GenerateParams::new("mock-model") + client: mock_client("test"), + ..GenerateParams::new("mock-model", mock_client("base")) }) .await; @@ -1341,7 +1302,7 @@ mod tests { let client = Arc::new(Client::new(providers, Some("mock".to_string()), vec![])); let result = generate( - GenerateParams::new("mock-model") + GenerateParams::new("mock-model", client) .prompt("What's the weather in SF?") .tools(vec![Tool::active( "get_weather", @@ -1352,8 +1313,7 @@ mod tests { Ok(serde_json::json!(format!("72F in {}", city))) }, )]) - .max_tool_rounds(5) - .client(client), + .max_tool_rounds(5), ) .await .unwrap(); @@ -1418,13 +1378,9 @@ mod tests { #[tokio::test] async fn stream_generate_returns_events() { let client = mock_client("Hello stream!"); - let mut stream = stream_generate( - GenerateParams::new("mock-model") - .prompt("Hi") - .client(client), - ) - .await - .unwrap(); + let mut stream = stream_generate(GenerateParams::new("mock-model", client).prompt("Hi")) + .await + .unwrap(); let first = stream.next().await.unwrap().unwrap(); match &first { @@ -1451,9 +1407,7 @@ mod tests { }); let result = generate_object( - GenerateParams::new("mock-model") - .prompt("Extract name and age") - .client(client), + GenerateParams::new("mock-model", client).prompt("Extract name and age"), schema, ) .await @@ -1470,9 +1424,7 @@ mod tests { let client = mock_client("not valid json"); let result = generate_object( - GenerateParams::new("mock-model") - .prompt("Extract data") - .client(client), + GenerateParams::new("mock-model", client).prompt("Extract data"), serde_json::json!({"type": "object"}), ) .await; @@ -1496,7 +1448,7 @@ mod tests { let client = Arc::new(Client::new(providers, Some("mock".to_string()), vec![])); let result = generate( - GenerateParams::new("mock-model") + GenerateParams::new("mock-model", client) .prompt("What's the weather in SF?") .tools(vec![Tool::active( "get_weather", @@ -1508,8 +1460,7 @@ mod tests { }, )]) .max_tool_rounds(5) - .stop_when(|_steps| true) // Stop immediately after first round - .client(client), + .stop_when(|_steps| true), // Stop immediately after first round ) .await .unwrap(); @@ -1521,7 +1472,7 @@ mod tests { #[test] fn generate_params_builder_methods() { - let params = GenerateParams::new("test-model") + let params = GenerateParams::new("test-model", mock_client("builder")) .prompt("hello") .system("you are helpful") .temperature(0.7) @@ -1558,10 +1509,11 @@ mod tests { #[test] fn generate_params_timeout_builder() { - let params = GenerateParams::new("test-model").timeout(TimeoutOptions { - total: Some(30.0), - per_step: Some(10.0), - }); + let params = + GenerateParams::new("test-model", mock_client("timeout")).timeout(TimeoutOptions { + total: Some(30.0), + per_step: Some(10.0), + }); assert!(params.timeout.is_some()); let t = params.timeout.unwrap(); assert_eq!(t.total, Some(30.0)); @@ -1662,9 +1614,7 @@ mod tests { }); let obj_stream = stream_object( - GenerateParams::new("mock-model") - .prompt("Extract info") - .client(client), + GenerateParams::new("mock-model", client).prompt("Extract info"), schema, ) .await @@ -1700,9 +1650,7 @@ mod tests { }); let obj_stream = stream_object( - GenerateParams::new("mock-model") - .prompt("Extract info") - .client(client), + GenerateParams::new("mock-model", client).prompt("Extract info"), schema, ) .await @@ -1748,9 +1696,7 @@ mod tests { let schema = serde_json::json!({"type": "object"}); let obj_stream = stream_object( - GenerateParams::new("mock-model") - .prompt("Extract info") - .client(client), + GenerateParams::new("mock-model", client).prompt("Extract info"), schema, ) .await @@ -1768,9 +1714,8 @@ mod tests { token.cancel(); let result = generate( - GenerateParams::new("mock-model") + GenerateParams::new("mock-model", mock_client("Hi")) .prompt("Hello") - .client(mock_client("Hi")) .abort_signal(token), ) .await; @@ -1839,7 +1784,7 @@ mod tests { let client = Arc::new(Client::new(providers, Some("mock".to_string()), vec![])); let result = generate( - GenerateParams::new("mock-model") + GenerateParams::new("mock-model", client) .prompt("What's the weather?") .tools(vec![Tool::active( "get_weather", @@ -1848,8 +1793,7 @@ mod tests { |_args, _ctx| async { Ok(serde_json::json!("72F")) }, )]) .max_tool_rounds(10) - .abort_signal(token) - .client(client), + .abort_signal(token), ) .await; @@ -1869,9 +1813,8 @@ mod tests { token_clone.cancel(); let mut stream_result = stream( - GenerateParams::new("mock-model") + GenerateParams::new("mock-model", client) .prompt("Hi") - .client(client) .abort_signal(token), ) .await @@ -1897,7 +1840,7 @@ mod tests { let tool_executed_clone = tool_executed.clone(); let result = generate( - GenerateParams::new("mock-model") + GenerateParams::new("mock-model", client) .prompt("What's the weather in SF?") .tools(vec![Tool::active( "get_weather", @@ -1911,8 +1854,7 @@ mod tests { } }, )]) - .max_tool_rounds(0) - .client(client), + .max_tool_rounds(0), ) .await .unwrap(); @@ -1928,20 +1870,16 @@ mod tests { #[test] fn generate_params_abort_signal_builder() { let token = CancellationToken::new(); - let params = GenerateParams::new("test-model").abort_signal(token); + let params = GenerateParams::new("test-model", mock_client("abort")).abort_signal(token); assert!(params.abort_signal.is_some()); } #[tokio::test] async fn stream_result_accumulates_response() { let client = mock_client("Hello!"); - let mut result = stream( - GenerateParams::new("mock-model") - .prompt("Hi") - .client(client), - ) - .await - .unwrap(); + let mut result = stream(GenerateParams::new("mock-model", client).prompt("Hi")) + .await + .unwrap(); assert!(result.response().is_none()); assert!(result.partial_response().is_none()); @@ -1956,13 +1894,9 @@ mod tests { #[tokio::test] async fn stream_result_text_stream() { let client = streaming_json_mock_client(vec!["Hello", " ", "world"]); - let result = stream( - GenerateParams::new("mock-model") - .prompt("Hi") - .client(client), - ) - .await - .unwrap(); + let result = stream(GenerateParams::new("mock-model", client).prompt("Hi")) + .await + .unwrap(); let texts: Vec = result .text_stream() @@ -2077,7 +2011,7 @@ mod tests { let client = Arc::new(Client::new(providers, Some("mock".to_string()), vec![])); let mut result = stream( - GenerateParams::new("mock-model") + GenerateParams::new("mock-model", client) .prompt("What's the weather in SF?") .tools(vec![Tool::active( "get_weather", @@ -2085,8 +2019,7 @@ mod tests { serde_json::json!({"type": "object", "properties": {"city": {"type": "string"}}}), |_args, _ctx| async { Ok(serde_json::json!("72F")) }, )]) - .max_tool_rounds(5) - .client(client), + .max_tool_rounds(5), ) .await .unwrap(); @@ -2130,7 +2063,7 @@ mod tests { let client = Arc::new(Client::new(providers, Some("mock".to_string()), vec![])); let mut result = stream( - GenerateParams::new("mock-model") + GenerateParams::new("mock-model", client) .prompt("What's the weather?") .tools(vec![Tool::active( "get_weather", @@ -2138,8 +2071,7 @@ mod tests { serde_json::json!({"type": "object", "properties": {"city": {"type": "string"}}}), |_args, _ctx| async { Ok(serde_json::json!("72F")) }, )]) - .max_tool_rounds(0) - .client(client), + .max_tool_rounds(0), ) .await .unwrap(); @@ -2205,7 +2137,7 @@ mod tests { let client = Arc::new(Client::new(providers, Some("mock".to_string()), vec![])); let mut result = stream( - GenerateParams::new("mock-model") + GenerateParams::new("mock-model", client) .prompt("What's the weather in SF?") .tools(vec![Tool::active( "get_weather", @@ -2213,8 +2145,7 @@ mod tests { serde_json::json!({"type": "object", "properties": {"city": {"type": "string"}}}), |_args, _ctx| async { Ok(serde_json::json!("72F")) }, )]) - .max_tool_rounds(5) - .client(client), + .max_tool_rounds(5), ) .await .unwrap(); @@ -2267,7 +2198,7 @@ mod tests { let client = Arc::new(Client::new(providers, Some("mock".to_string()), vec![])); let mut result = stream( - GenerateParams::new("mock-model") + GenerateParams::new("mock-model", client) .prompt("What's the weather in SF?") .tools(vec![Tool::active( "get_weather", @@ -2276,8 +2207,7 @@ mod tests { |_args, _ctx| async { Ok(serde_json::json!("72F")) }, )]) .max_tool_rounds(5) - .stop_when(|_steps| true) // Stop immediately after first round - .client(client), + .stop_when(|_steps| true), // Stop immediately after first round ) .await .unwrap(); @@ -2392,7 +2322,7 @@ mod tests { // Need active tools so the tool loop path (with retry) is used let mut result = stream( - GenerateParams::new("mock-model") + GenerateParams::new("mock-model", client) .prompt("Hi") .tools(vec![Tool::active( "get_weather", @@ -2401,8 +2331,7 @@ mod tests { |_args, _ctx| async { Ok(serde_json::json!("72F")) }, )]) .max_tool_rounds(1) - .max_retries(3) - .client(client), + .max_retries(3), ) .await .unwrap(); @@ -2489,7 +2418,7 @@ mod tests { // Need active tools so the tool loop path (with timeout) is used let mut result = stream( - GenerateParams::new("mock-model") + GenerateParams::new("mock-model", client) .prompt("Hi") .tools(vec![Tool::active( "get_weather", @@ -2502,8 +2431,7 @@ mod tests { total: None, per_step: Some(0.01), // 10ms timeout, provider takes 5s }) - .max_retries(0) - .client(client), + .max_retries(0), ) .await .unwrap(); @@ -2621,7 +2549,7 @@ mod tests { let client = Arc::new(Client::new(providers, Some("mock".to_string()), vec![])); let mut result = stream( - GenerateParams::new("mock-model") + GenerateParams::new("mock-model", client) .prompt("What's the weather?") .tools(vec![Tool::active( "get_weather", @@ -2634,8 +2562,7 @@ mod tests { total: Some(0.05), // 50ms total timeout per_step: None, }) - .max_retries(0) - .client(client), + .max_retries(0), ) .await .unwrap(); diff --git a/lib/crates/fabro-llm/src/lib.rs b/lib/crates/fabro-llm/src/lib.rs index 323d5abe6..1ac82a93a 100644 --- a/lib/crates/fabro-llm/src/lib.rs +++ b/lib/crates/fabro-llm/src/lib.rs @@ -9,7 +9,5 @@ pub mod retry; pub mod tools; pub mod types; -// Re-export module-level default client helpers (Section 2.5). pub use error::{Error, ProviderErrorDetail, ProviderErrorKind, Result}; pub use fabro_model::{ModelHandle, Provider}; -pub use generate::set_default_client; diff --git a/lib/crates/fabro-llm/src/model_test.rs b/lib/crates/fabro-llm/src/model_test.rs index 4e1d93175..fdcd11cf4 100644 --- a/lib/crates/fabro-llm/src/model_test.rs +++ b/lib/crates/fabro-llm/src/model_test.rs @@ -71,22 +71,10 @@ impl ModelTestOutcome { } } -pub async fn run_model_test(info: &Model, mode: ModelTestMode) -> ModelTestOutcome { - run_model_test_inner(info, mode, None).await -} - -pub async fn run_model_test_with_client( +pub async fn run_model_test( info: &Model, mode: ModelTestMode, client: Arc, -) -> ModelTestOutcome { - run_model_test_inner(info, mode, Some(client)).await -} - -async fn run_model_test_inner( - info: &Model, - mode: ModelTestMode, - client: Option>, ) -> ModelTestOutcome { match mode { ModelTestMode::Basic => run_basic_test(info, client).await, @@ -94,14 +82,11 @@ async fn run_model_test_inner( } } -async fn run_basic_test(info: &Model, client: Option>) -> ModelTestOutcome { - let mut params = GenerateParams::new(&info.id) +async fn run_basic_test(info: &Model, client: Arc) -> ModelTestOutcome { + let params = GenerateParams::new(&info.id, client) .provider(info.provider.as_str()) .prompt("Say OK") .max_tokens(16); - if let Some(client) = client { - params = params.client(client); - } let result = time::timeout( Duration::from_secs(ModelTestMode::Basic.timeout_secs()), @@ -116,7 +101,7 @@ async fn run_basic_test(info: &Model, client: Option>) -> ModelTestO } } -async fn run_deep_test(info: &Model, client: Option>) -> ModelTestOutcome { +async fn run_deep_test(info: &Model, client: Arc) -> ModelTestOutcome { let Some(params) = build_deep_test_params(info, client) else { return ModelTestOutcome::error("model does not support tools"); }; @@ -137,7 +122,7 @@ async fn run_deep_test(info: &Model, client: Option>) -> ModelTestOu } } -fn build_deep_test_params(info: &Model, client: Option>) -> Option { +fn build_deep_test_params(info: &Model, client: Arc) -> Option { if !info.features.tools { return None; } @@ -166,7 +151,7 @@ fn build_deep_test_params(info: &Model, client: Option>) -> Option>) -> Option Result<(), String> { #[cfg(test)] mod tests { + use std::collections::HashMap; + use fabro_model::{ModelCosts, ModelFeatures, ModelLimits, Provider}; use super::*; @@ -248,6 +231,10 @@ mod tests { } } + fn empty_test_client() -> Arc { + Arc::new(Client::new(HashMap::new(), None, vec![])) + } + #[tokio::test] async fn run_model_test_deep_errors_when_model_lacks_tools() { let info = test_model_with(ModelFeatures { @@ -257,7 +244,7 @@ mod tests { effort: true, }); - let outcome = run_model_test(&info, ModelTestMode::Deep).await; + let outcome = run_model_test(&info, ModelTestMode::Deep, empty_test_client()).await; assert_eq!(outcome.status, ModelTestStatus::Error); assert_eq!( diff --git a/lib/crates/fabro-server/src/diagnostics.rs b/lib/crates/fabro-server/src/diagnostics.rs index 7ea025eee..160480a7a 100644 --- a/lib/crates/fabro-server/src/diagnostics.rs +++ b/lib/crates/fabro-server/src/diagnostics.rs @@ -2,6 +2,7 @@ use std::time::Duration; use base64::Engine as _; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; +use fabro_auth::auth_issue_message; use fabro_llm::client::Client as LlmClient; use fabro_llm::types::{Message, Request}; use fabro_model::{Catalog, Provider}; @@ -15,7 +16,6 @@ use serde::Serialize; use tokio::time::timeout; use crate::server::AppState; -use crate::server_secrets::auth_issue_message; fn http_client_or_check( name: &str, @@ -75,7 +75,7 @@ pub async fn run_all(state: &AppState) -> DiagnosticsReport { } async fn check_llm_providers(state: &AppState) -> CheckResult { - let result = match state.build_llm_client().await { + let result = match state.resolve_llm_client().await { Ok(result) => result, Err(err) => { return CheckResult { diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index c5cc7b87d..8275725ea 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -4,6 +4,7 @@ use std::sync::Arc; use anyhow::{Result, anyhow, bail}; use fabro_api::types; +use fabro_auth::auth_issue_message; use fabro_config::{ CliLayer, CliOutputLayer, DaytonaDockerfileLayer, ReplaceMap, RunExecutionLayer, RunLayer, RunModelLayer, RunSandboxLayer, WorkflowSettingsBuilder, @@ -34,7 +35,6 @@ use fabro_workflow::run_materialization::materialize_run; use fabro_workflow::workflow_bundle::{BundledWorkflow, WorkflowBundle}; use crate::server::AppState; -use crate::server_secrets::auth_issue_message; #[derive(Clone)] pub(crate) struct PreparedManifest { @@ -389,7 +389,7 @@ async fn build_preflight_report( )); } - let configured_providers = state.provider_credentials.configured_providers().await; + let configured_providers = state.llm_source.configured_providers().await; let materialized = materialize_run( prepared.settings.clone(), graph, @@ -600,7 +600,7 @@ async fn run_llm_check( let (model, provider) = resolve_model_provider(settings, graph, configured_providers); let default_provider = provider.as_deref().unwrap_or("anthropic"); - match state.build_llm_client().await { + match state.resolve_llm_client().await { Ok(result) => { let configured = result .client diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index a23b0a993..1a85f1ec3 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -37,14 +37,17 @@ pub use fabro_api::types::{ StageStatus as ApiStageStatus, StartRunRequest, SubmitAnswerRequest, SystemFeatures, SystemInfoResponse, SystemRunCounts, WriteBlobResponse, }; -use fabro_auth::parse_credential_secret; +use fabro_auth::{ + CredentialSource, VaultCredentialSource, auth_issue_message, parse_credential_secret, +}; use fabro_config::daemon::ServerDaemon; use fabro_config::{RunLayer, RunSettingsBuilder, ServerSettingsBuilder, Storage, envfile}; use fabro_interview::{ Answer, ControlInterviewer, Interviewer, Question, QuestionType, WorkerControlEnvelope, }; +use fabro_llm::client::Client as LlmClient; use fabro_llm::generate::{GenerateParams, generate_object}; -use fabro_llm::model_test::{ModelTestMode, run_model_test_with_client}; +use fabro_llm::model_test::{ModelTestMode, run_model_test}; use fabro_llm::types::{ ContentPart, FinishReason, Message as LlmMessage, Request as LlmRequest, Role, ToolChoice, ToolDefinition, @@ -113,9 +116,7 @@ use crate::ip_allowlist::{IpAllowlistConfig, ip_allowlist_middleware}; use crate::jwt_auth::{self, AuthMode, AuthenticatedService, AuthenticatedSubject}; use crate::run_files::{FilesInFlight, list_run_files, new_files_in_flight}; use crate::run_selector::{ResolveRunError, resolve_run_by_selector}; -use crate::server_secrets::{ - LlmClientResult, ProviderCredentials, ServerSecrets, auth_issue_message, -}; +use crate::server_secrets::{LlmClientResult, ServerSecrets}; use crate::spawn_env::{apply_render_graph_env, apply_worker_env}; use crate::worker_token::{ AuthorizeRunBlob, AuthorizeRunScoped, AuthorizeStageArtifact, WorkerTokenKeys, @@ -548,18 +549,18 @@ 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) provider_credentials: ProviderCredentials, - manifest_run_defaults: RwLock>, - manifest_run_settings: RwLock>, - 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, + manifest_run_defaults: RwLock>, + manifest_run_settings: RwLock>, + 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 { @@ -664,8 +665,20 @@ impl AppState { ) } - pub(crate) async fn build_llm_client(&self) -> Result { - self.provider_credentials.build_llm_client().await + pub(crate) async fn resolve_llm_client(&self) -> Result { + let resolved = self + .llm_source + .resolve() + .await + .map_err(|err| err.to_string())?; + let client = LlmClient::from_credentials(resolved.credentials) + .await + .map_err(|err| err.to_string())?; + + Ok(LlmClientResult { + client, + auth_issues: resolved.auth_issues, + }) } pub(crate) fn vault_or_env(&self, name: &str) -> Option { @@ -2667,10 +2680,13 @@ 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 (global_event_tx, _) = broadcast::channel(4096); let current_server_settings = Arc::new(resolved_settings.server_settings); let current_manifest_run_defaults = Arc::new(resolved_settings.manifest_run_defaults); @@ -2713,7 +2729,7 @@ pub(crate) fn build_app_state(config: AppStateConfig) -> anyhow::Result result, Err(err) => { return ApiError::new( StatusCode::INTERNAL_SERVER_ERROR, - format!("Failed to build LLM client: {err}"), + format!("Failed to resolve LLM client: {err}"), ) .into_response(); } @@ -6828,7 +6844,7 @@ async fn test_model( } let client = Arc::new(llm_result.client); - let outcome = run_model_test_with_client(info, mode, client).await; + let outcome = run_model_test(info, mode, client).await; Json(serde_json::json!({ "model_id": info.id, "status": outcome.status.as_str(), @@ -6976,8 +6992,7 @@ async fn create_completion( // Force non-streaming for structured output let use_stream = req.stream && req.schema.is_none(); - // Get or create LLM client (cached in AppState) - let llm_result = match state.build_llm_client().await { + let llm_result = match state.resolve_llm_client().await { Ok(result) => result, Err(err) => { return ApiError::new( @@ -7041,9 +7056,9 @@ async fn create_completion( if let Some(schema) = req.schema { // Structured output uses generate_object for JSON parsing logic - let mut params = GenerateParams::new(&request.model) - .messages(request.messages) - .client(std::sync::Arc::new(client.clone())); + let mut params = + GenerateParams::new(&request.model, std::sync::Arc::new(client.clone())) + .messages(request.messages); if let Some(ref p) = request.provider { params = params.provider(p); } @@ -7316,13 +7331,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; @@ -7381,6 +7400,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!())) @@ -7954,6 +8006,120 @@ provider = "invalid-provider" 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( + default_test_server_settings(), + RunLayer::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( + default_test_server_settings(), + RunLayer::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( + default_test_server_settings(), + RunLayer::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(); @@ -8304,27 +8470,6 @@ allowed_usernames = ["octocat"] .unwrap_or(EnvOverride::Unchanged) } - #[test] - fn provider_credentials_resolve_process_env_before_vault() { - let dir = tempfile::tempdir().unwrap(); - let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); - vault - .set("OPENAI_API_KEY", "vault-key", SecretType::Environment, None) - .unwrap(); - - let provider_credentials = - ProviderCredentials::with_env_lookup(Arc::new(AsyncRwLock::new(vault)), |name| { - match name { - "OPENAI_API_KEY" => Some("env-key".to_string()), - _ => None, - } - }); - - let runtime = tokio::runtime::Runtime::new().unwrap(); - let resolved = runtime.block_on(provider_credentials.get("OPENAI_API_KEY")); - assert_eq!(resolved.as_deref(), Some("env-key")); - } - #[tokio::test] async fn subprocess_answer_transport_cancel_run_enqueues_cancel_message() { let (control_tx, mut control_rx) = tokio::sync::mpsc::channel(1); diff --git a/lib/crates/fabro-server/src/server_secrets.rs b/lib/crates/fabro-server/src/server_secrets.rs index b09ed710b..5db4919e7 100644 --- a/lib/crates/fabro-server/src/server_secrets.rs +++ b/lib/crates/fabro-server/src/server_secrets.rs @@ -1,15 +1,10 @@ use std::collections::HashMap; use std::path::Path; -use std::sync::Arc; -use fabro_auth::{CredentialResolver, CredentialUsage, ResolveError, ResolvedCredential}; +use fabro_auth::ResolveError; use fabro_config::envfile; -use fabro_llm::client::Client as LlmClient; +use fabro_llm::client::Client; use fabro_model::Provider; -use fabro_vault::Vault; -use tokio::sync::RwLock as AsyncRwLock; - -type EnvLookup = Arc Option + Send + Sync>; pub fn process_env_snapshot() -> HashMap { std::env::vars().collect() @@ -57,150 +52,18 @@ impl std::fmt::Debug for ServerSecrets { } } -#[derive(Clone)] -pub(crate) struct ProviderCredentials { - vault: Arc>, - env_lookup: EnvLookup, -} - -impl ProviderCredentials { - pub(crate) fn with_env_lookup(vault: Arc>, env_lookup: F) -> Self - where - F: Fn(&str) -> Option + Send + Sync + 'static, - { - Self { - vault, - env_lookup: Arc::new(env_lookup), - } - } - - #[cfg(test)] - pub(crate) async fn get(&self, name: &str) -> Option { - let env_value = (self.env_lookup)(name); - if env_value.is_some() { - return env_value; - } - - self.vault.read().await.get(name).map(str::to_string) - } - - pub(crate) async fn build_llm_client(&self) -> Result { - let resolver = - CredentialResolver::with_env_lookup(Arc::clone(&self.vault), self.env_lookup.clone()); - let mut api_credentials = Vec::new(); - let mut auth_issues = Vec::new(); - - for provider in Provider::ALL { - match resolver - .resolve(*provider, CredentialUsage::ApiRequest) - .await - { - Ok(ResolvedCredential::Api(credential)) => api_credentials.push(credential), - Ok(ResolvedCredential::Cli(_)) | Err(ResolveError::NotConfigured(_)) => {} - Err(err) => auth_issues.push((*provider, err)), - } - } - - let client = LlmClient::from_credentials(api_credentials) - .await - .map_err(|err| err.to_string())?; - - Ok(LlmClientResult { - client, - auth_issues, - }) - } - - pub(crate) async fn configured_providers(&self) -> Vec { - let resolver = - CredentialResolver::with_env_lookup(Arc::clone(&self.vault), self.env_lookup.clone()); - let vault = self.vault.read().await; - resolver.configured_providers(&vault) - } -} - pub(crate) struct LlmClientResult { - pub client: LlmClient, + pub client: Client, pub auth_issues: Vec<(Provider, ResolveError)>, } -pub(crate) fn auth_issue_message(provider: Provider, err: &ResolveError) -> String { - match err { - ResolveError::NotConfigured(_) => { - format!("{} is not configured", provider.display_name()) - } - ResolveError::RefreshFailed { source, .. } => format!( - "{} requires re-authentication: {}", - provider.display_name(), - source - ), - ResolveError::RefreshTokenMissing(_) => format!( - "{} requires re-authentication: refresh token missing", - provider.display_name() - ), - } -} - -impl std::fmt::Debug for ProviderCredentials { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_struct("ProviderCredentials") - .finish_non_exhaustive() - } -} - #[cfg(test)] mod tests { use std::collections::HashMap; - use std::sync::Arc; - use fabro_auth::{AuthCredential, AuthDetails}; use fabro_config::envfile; - use fabro_vault::{SecretType, Vault}; - use tokio::sync::RwLock as AsyncRwLock; - use super::{ProviderCredentials, ServerSecrets}; - use crate::server_secrets::Provider; - - #[tokio::test] - async fn configured_providers_respects_injected_env_lookup() { - let dir = tempfile::tempdir().unwrap(); - let vault = Arc::new(AsyncRwLock::new( - Vault::load(dir.path().join("secrets.json")).unwrap(), - )); - let credentials = ProviderCredentials::with_env_lookup(Arc::clone(&vault), |name| { - (name == "OPENAI_API_KEY").then(|| "openai-key".to_string()) - }); - - assert_eq!(credentials.configured_providers().await, vec![ - Provider::OpenAi - ]); - } - - #[tokio::test] - async fn configured_providers_includes_vault_credentials() { - let dir = tempfile::tempdir().unwrap(); - let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); - vault - .set( - "anthropic", - &serde_json::to_string(&AuthCredential { - provider: Provider::Anthropic, - details: AuthDetails::ApiKey { - key: "anthropic-key".to_string(), - }, - }) - .unwrap(), - SecretType::Credential, - None, - ) - .unwrap(); - let credentials = - ProviderCredentials::with_env_lookup(Arc::new(AsyncRwLock::new(vault)), |_| None); - - assert_eq!(credentials.configured_providers().await, vec![ - Provider::Anthropic - ]); - } + use super::ServerSecrets; #[test] fn server_secrets_snapshot_prefers_env_over_file() { 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/event.rs b/lib/crates/fabro-workflow/src/event.rs index a02e2b4fd..bd81bc20a 100644 --- a/lib/crates/fabro-workflow/src/event.rs +++ b/lib/crates/fabro-workflow/src/event.rs @@ -2888,6 +2888,19 @@ impl Emitter { self.emit_with_scope(event, Some(scope)); } + pub fn notice( + &self, + level: RunNoticeLevel, + code: impl Into, + message: impl Into, + ) { + self.emit(&Event::RunNotice { + level, + code: code.into(), + message: message.into(), + }); + } + fn emit_with_scope(&self, event: &Event, scope: Option<&StageScope>) { self.last_event_at.store(epoch_millis(), Ordering::Relaxed); event.trace(); diff --git a/lib/crates/fabro-workflow/src/handler/agent.rs b/lib/crates/fabro-workflow/src/handler/agent.rs index 598f7dc29..8ed952359 100644 --- a/lib/crates/fabro-workflow/src/handler/agent.rs +++ b/lib/crates/fabro-workflow/src/handler/agent.rs @@ -254,10 +254,10 @@ impl Handler for AgentHandler { let prompt_provider = node .provider() .map(String::from) - .or_else(|| Some(services.provider.as_str().to_string())); + .or_else(|| Some(services.run.provider.as_str().to_string())); let prompt_model = node.model().map(String::from); let stage_scope = StageScope::for_handler(context, &node.id); - services.emitter.emit_scoped( + services.run.emitter.emit_scoped( &Event::Prompt { stage: node.id.clone(), visit: stage_scope.visit, @@ -276,10 +276,10 @@ impl Handler for AgentHandler { .parse::() .map_err(|err| Error::handler(format!("invalid internal run_id: {err}")))?; let tool_hooks: Option> = - services.hook_runner.as_ref().map(|hr| { + services.run.hook_runner.as_ref().map(|hr| { Arc::new(fabro_hooks::WorkflowToolHookCallback { hook_runner: Arc::clone(hr), - sandbox: Arc::clone(&services.sandbox), + sandbox: Arc::clone(&services.run.sandbox), run_id, workflow_name: graph.name.clone(), work_dir: None, @@ -294,8 +294,8 @@ impl Handler for AgentHandler { &prompt, context, thread_id.as_deref(), - &services.emitter, - &services.sandbox, + &services.run.emitter, + &services.run.sandbox, tool_hooks, ) .await; @@ -331,9 +331,9 @@ impl Handler for AgentHandler { let response_provider = node .provider() .map(String::from) - .or_else(|| Some(services.provider.as_str().to_string())) + .or_else(|| Some(services.run.provider.as_str().to_string())) .unwrap_or_default(); - services.emitter.emit_scoped( + services.run.emitter.emit_scoped( &Event::PromptCompleted { node_id: node.id.clone(), response: response_text.clone(), @@ -366,6 +366,7 @@ impl Handler for AgentHandler { if !found_in_response { let mut found_in_status_json = false; if let Ok(result) = services + .run .sandbox .exec_command("cat status.json", 5_000, None, None, None) .await @@ -379,6 +380,7 @@ impl Handler for AgentHandler { let quoted = shlex::try_quote(path).unwrap_or_else(|_| path.into()); let cmd = format!("cat {quoted}"); if let Ok(result) = services + .run .sandbox .exec_command(&cmd, 5_000, None, None, None) .await @@ -435,13 +437,13 @@ mod tests { ) { let store = test_store(); let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); - let services = EngineServices { - emitter: Arc::new(crate::event::Emitter::new(fixtures::RUN_1)), - run_store: run_store.clone().into(), - ..EngineServices::test_default() - }; + let mut services = EngineServices::test_default(); + services.run = services + .run + .with_emitter(Arc::new(crate::event::Emitter::new(fixtures::RUN_1))) + .with_run_store(run_store.clone().into()); let logger = crate::event::StoreProgressLogger::new(run_store.clone()); - logger.register(services.emitter.as_ref()); + logger.register(services.run.emitter.as_ref()); (services, run_store, logger) } @@ -576,9 +578,12 @@ mod tests { let tmp = TempDir::new().unwrap(); let mut services = EngineServices::test_default(); - services.sandbox = std::sync::Arc::new(fabro_agent::LocalSandbox::new( - sandbox_dir.path().to_path_buf(), - )); + services.run = + services + .run + .with_sandbox(std::sync::Arc::new(fabro_agent::LocalSandbox::new( + sandbox_dir.path().to_path_buf(), + ))); let outcome = handler .execute(&node, &context, &graph, tmp.path(), &services) @@ -634,9 +639,12 @@ mod tests { let tmp = TempDir::new().unwrap(); let mut services = EngineServices::test_default(); - services.sandbox = std::sync::Arc::new(fabro_agent::LocalSandbox::new( - sandbox_dir.path().to_path_buf(), - )); + services.run = + services + .run + .with_sandbox(std::sync::Arc::new(fabro_agent::LocalSandbox::new( + sandbox_dir.path().to_path_buf(), + ))); let outcome = handler .execute(&node, &context, &graph, tmp.path(), &services) @@ -692,9 +700,12 @@ mod tests { let tmp = TempDir::new().unwrap(); let mut services = EngineServices::test_default(); - services.sandbox = std::sync::Arc::new(fabro_agent::LocalSandbox::new( - sandbox_dir.path().to_path_buf(), - )); + services.run = + services + .run + .with_sandbox(std::sync::Arc::new(fabro_agent::LocalSandbox::new( + sandbox_dir.path().to_path_buf(), + ))); let outcome = handler .execute(&node, &context, &graph, tmp.path(), &services) diff --git a/lib/crates/fabro-workflow/src/handler/command.rs b/lib/crates/fabro-workflow/src/handler/command.rs index be550ec27..6dd7af4aa 100644 --- a/lib/crates/fabro-workflow/src/handler/command.rs +++ b/lib/crates/fabro-workflow/src/handler/command.rs @@ -89,7 +89,7 @@ impl Handler for CommandHandler { script.to_string() }; let stage_scope = StageScope::for_handler(context, &node.id); - services.emitter.emit_scoped( + services.run.emitter.emit_scoped( &Event::CommandStarted { node_id: node.id.clone(), script: script.to_string(), @@ -106,9 +106,10 @@ impl Handler for CommandHandler { } else { Some(&services.env) }; - let cancel_token = services.sandbox_cancel_token(); + let cancel_token = services.run.sandbox_cancel_token(); let result = services + .run .sandbox .exec_command(&command, timeout_ms, None, env_vars, cancel_token.clone()) .await; @@ -117,7 +118,7 @@ impl Handler for CommandHandler { } let result = result.map_err(|e| Error::handler(format!("Failed to spawn script: {e}")))?; - services.emitter.emit_scoped( + services.run.emitter.emit_scoped( &Event::CommandCompleted { node_id: node.id.clone(), stdout: result.stdout.clone(), @@ -205,13 +206,13 @@ mod tests { ) { let store = test_store(); let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); - let services = EngineServices { - emitter: Arc::new(crate::event::Emitter::new(fixtures::RUN_1)), - run_store: run_store.clone().into(), - ..EngineServices::test_default() - }; + let mut services = EngineServices::test_default(); + services.run = services + .run + .with_emitter(Arc::new(crate::event::Emitter::new(fixtures::RUN_1))) + .with_run_store(run_store.clone().into()); let logger = crate::event::StoreProgressLogger::new(run_store.clone()); - logger.register(services.emitter.as_ref()); + logger.register(services.run.emitter.as_ref()); (services, run_store, logger) } @@ -806,7 +807,7 @@ mod tests { fn make_spy_services(sandbox: std::sync::Arc) -> EngineServices { let mut services = EngineServices::test_default(); - services.sandbox = sandbox; + services.run = services.run.with_sandbox(sandbox); services } @@ -952,7 +953,9 @@ mod tests { let run_dir = tempfile::tempdir().unwrap(); let mut services = make_spy_services(spy.clone()); - services.cancel_requested = Some(Arc::new(AtomicBool::new(false))); + services.run = services + .run + .with_cancel_requested(Some(Arc::new(AtomicBool::new(false)))); handler .execute(&node, &context, &graph, run_dir.path(), &services) diff --git a/lib/crates/fabro-workflow/src/handler/fan_in.rs b/lib/crates/fabro-workflow/src/handler/fan_in.rs index 880387dcc..8b0bb756f 100644 --- a/lib/crates/fabro-workflow/src/handler/fan_in.rs +++ b/lib/crates/fabro-workflow/src/handler/fan_in.rs @@ -84,8 +84,8 @@ impl Handler for FanInHandler { context, run_dir, &node.id, - &services.emitter, - &services.sandbox, + &services.run.emitter, + &services.run.sandbox, ) .await? } else { @@ -116,7 +116,7 @@ impl Handler for FanInHandler { }; if let (Some(ref sha), Some(_)) = (&best_head_sha, services.git_state()) { - git_merge_ff_only(&*services.sandbox, sha).await; + git_merge_ff_only(&*services.run.sandbox, sha).await; } let mut outcome = Outcome::success(); diff --git a/lib/crates/fabro-workflow/src/handler/human.rs b/lib/crates/fabro-workflow/src/handler/human.rs index 9b2a675da..a56252ec7 100644 --- a/lib/crates/fabro-workflow/src/handler/human.rs +++ b/lib/crates/fabro-workflow/src/handler/human.rs @@ -262,7 +262,7 @@ impl Handler for HumanHandler { let question_id = question.id.clone(); let stage_scope = StageScope::for_handler(context, &node.id); self.emit( - &services.emitter, + &services.run.emitter, &Event::InterviewStarted { question_id: question_id.clone(), question: question_text.clone(), @@ -282,14 +282,15 @@ impl Handler for HumanHandler { }, &stage_scope, ); - self.tracker.interview_started(services.emitter.as_ref()); + self.tracker + .interview_started(services.run.emitter.as_ref()); let interview_start = Instant::now(); let answer = self.interviewer.ask(question).await; // 4. Handle timeout if answer.value == AnswerValue::Timeout { self.emit( - &services.emitter, + &services.run.emitter, &Event::InterviewTimeout { question_id: question_id.clone(), question: question_text, @@ -298,7 +299,8 @@ impl Handler for HumanHandler { }, &stage_scope, ); - self.tracker.interview_resolved(services.emitter.as_ref()); + self.tracker + .interview_resolved(services.run.emitter.as_ref()); let default_choice = node .attrs .get("human.default_choice") @@ -320,6 +322,7 @@ impl Handler for HumanHandler { // 5. Handle unanswered / interrupted interview sessions. if answer.value == AnswerValue::Interrupted { if services + .run .cancel_requested .as_ref() .is_some_and(|flag| flag.load(Ordering::SeqCst)) @@ -327,7 +330,7 @@ impl Handler for HumanHandler { return Err(Error::Cancelled); } self.emit( - &services.emitter, + &services.run.emitter, &Event::InterviewInterrupted { question_id: question_id.clone(), question: question_text, @@ -337,14 +340,15 @@ impl Handler for HumanHandler { }, &stage_scope, ); - self.tracker.interview_resolved(services.emitter.as_ref()); + self.tracker + .interview_resolved(services.run.emitter.as_ref()); return Ok(unanswered_human_gate( "human interaction interrupted before an answer was provided", )); } if answer.value == AnswerValue::Skipped { self.emit( - &services.emitter, + &services.run.emitter, &Event::InterviewCompleted { question_id, question: question_text, @@ -353,13 +357,14 @@ impl Handler for HumanHandler { }, &stage_scope, ); - self.tracker.interview_resolved(services.emitter.as_ref()); + self.tracker + .interview_resolved(services.run.emitter.as_ref()); return Ok(unanswered_human_gate("human skipped interaction")); } // Emit interview completed for successful interactions self.emit( - &services.emitter, + &services.run.emitter, &Event::InterviewCompleted { question_id, question: question_text, @@ -368,7 +373,8 @@ impl Handler for HumanHandler { }, &stage_scope, ); - self.tracker.interview_resolved(services.emitter.as_ref()); + self.tracker + .interview_resolved(services.run.emitter.as_ref()); // 6. Try fixed-choice match if let Some(selected) = find_choice_match(&answer, &choices) { @@ -477,7 +483,7 @@ mod tests { .expect("event log lock poisoned") .push(event.clone()); }); - services.emitter = emitter; + services.run = services.run.with_emitter(emitter); services } diff --git a/lib/crates/fabro-workflow/src/handler/llm/api.rs b/lib/crates/fabro-workflow/src/handler/llm/api.rs index c4bb57ec7..36c93f5c3 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/api.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/api.rs @@ -7,7 +7,7 @@ use fabro_agent::{ AgentEvent, AgentProfile, AnthropicProfile, GeminiProfile, OpenAiProfile, Sandbox, Session, SessionOptions, Turn, }; -use fabro_auth::{CredentialResolver, CredentialUsage, ResolveError, ResolvedCredential}; +use fabro_auth::{CredentialSource, EnvCredentialSource}; use fabro_graphviz::graph::Node; use fabro_llm::client::Client; use fabro_llm::types::{Message, Request, TokenCounts}; @@ -35,65 +35,6 @@ fn build_profile(model: &str, provider: Provider) -> Box { } } -pub(crate) struct LlmClientBuildResult { - pub(crate) client: Client, - pub(crate) auth_issues: Vec<(Provider, ResolveError)>, -} - -pub(crate) async fn build_llm_client( - resolver: Option<&CredentialResolver>, -) -> Result { - let Some(resolver) = resolver else { - let client = Client::from_env() - .await - .map_err(|e| Error::handler(format!("Failed to create LLM client: {e}")))?; - return Ok(LlmClientBuildResult { - client, - auth_issues: Vec::new(), - }); - }; - - let mut api_credentials = Vec::new(); - let mut auth_issues = Vec::new(); - - for provider in Provider::ALL { - match resolver - .resolve(*provider, CredentialUsage::ApiRequest) - .await - { - Ok(ResolvedCredential::Api(credential)) => api_credentials.push(credential), - Ok(ResolvedCredential::Cli(_)) | Err(ResolveError::NotConfigured(_)) => {} - Err(err) => auth_issues.push((*provider, err)), - } - } - - let client = Client::from_credentials(api_credentials) - .await - .map_err(|e| Error::handler(format!("Failed to create LLM client: {e}")))?; - - Ok(LlmClientBuildResult { - client, - auth_issues, - }) -} - -pub(crate) fn auth_issue_message(provider: Provider, err: &ResolveError) -> String { - match err { - ResolveError::NotConfigured(_) => { - format!("{} is not configured", provider.display_name()) - } - ResolveError::RefreshFailed { source, .. } => format!( - "{} requires re-authentication: {}", - provider.display_name(), - source - ), - ResolveError::RefreshTokenMissing(_) => format!( - "{} requires re-authentication: refresh token missing", - provider.display_name() - ), - } -} - /// Shared state for tracking file modifications from agent tool calls. struct FileTracking { /// Maps tool_call_id → file_path for in-flight write/edit calls. @@ -180,7 +121,7 @@ pub struct AgentApiBackend { sessions: Mutex>, env: HashMap, mcp_servers: Vec, - resolver: Option, + source: Arc, } impl AgentApiBackend { @@ -189,7 +130,7 @@ impl AgentApiBackend { model: String, provider: Provider, fallback_chain: Vec, - resolver: CredentialResolver, + source: Arc, ) -> Self { Self { model, @@ -198,7 +139,7 @@ impl AgentApiBackend { sessions: Mutex::new(HashMap::new()), env: HashMap::new(), mcp_servers: Vec::new(), - resolver: Some(resolver), + source, } } @@ -208,15 +149,12 @@ impl AgentApiBackend { provider: Provider, fallback_chain: Vec, ) -> Self { - Self { + Self::new( model, provider, fallback_chain, - sessions: Mutex::new(HashMap::new()), - env: HashMap::new(), - mcp_servers: Vec::new(), - resolver: None, - } + Arc::new(EnvCredentialSource::new()), + ) } #[must_use] @@ -247,7 +185,7 @@ impl AgentApiBackend { provider, node, sandbox, - self.resolver.as_ref(), + self.source.as_ref(), &self.env, tool_hooks, self.mcp_servers.clone(), @@ -260,12 +198,14 @@ impl AgentApiBackend { provider: Provider, node: &Node, sandbox: &Arc, - resolver: Option<&CredentialResolver>, + source: &dyn CredentialSource, env: &HashMap, tool_hooks: Option>, mcp_servers: Vec, ) -> Result { - let client = build_llm_client(resolver).await?.client; + let client = Client::from_source(source) + .await + .map_err(|e| Error::handler(format!("Failed to create LLM client: {e}")))?; let mut profile = build_profile(model, provider); @@ -346,7 +286,9 @@ impl CodergenBackend for AgentApiBackend { prompt: &str, system_prompt: Option<&str>, ) -> Result { - let client = build_llm_client(self.resolver.as_ref()).await?.client; + let client = Client::from_source(self.source.as_ref()) + .await + .map_err(|e| Error::handler(format!("Failed to create LLM client: {e}")))?; let model = node.model().unwrap_or(&self.model); let provider = node @@ -587,7 +529,7 @@ impl CodergenBackend for AgentApiBackend { target_provider, node, sandbox, - self.resolver.as_ref(), + self.source.as_ref(), &self.env, tool_hooks.clone(), self.mcp_servers.clone(), @@ -697,7 +639,7 @@ impl CodergenBackend for AgentApiBackend { #[cfg(test)] mod tests { use fabro_agent::subagent::SessionFactory; - use fabro_auth::{AuthCredential, AuthDetails, CredentialResolver}; + use fabro_auth::{AuthCredential, AuthDetails, VaultCredentialSource}; use fabro_vault::{SecretType, Vault}; use tokio::sync::RwLock as AsyncRwLock; @@ -847,7 +789,7 @@ mod tests { } #[tokio::test] - async fn build_llm_client_uses_resolver_credentials() { + async fn api_backend_uses_source_credentials() { let dir = tempfile::tempdir().unwrap(); let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); vault @@ -864,14 +806,18 @@ mod tests { None, ) .unwrap(); - let resolver = CredentialResolver::with_env_lookup( - Arc::new(AsyncRwLock::new(vault)), - Arc::new(|_| None), + let backend = AgentApiBackend::new( + "claude-opus-4-6".to_string(), + Provider::Anthropic, + Vec::new(), + Arc::new(VaultCredentialSource::with_env_lookup( + Arc::new(AsyncRwLock::new(vault)), + |_| None, + )), ); - let result = build_llm_client(Some(&resolver)).await.unwrap(); + let client = Client::from_source(backend.source.as_ref()).await.unwrap(); - assert_eq!(result.client.provider_names(), vec!["anthropic"]); - assert!(result.auth_issues.is_empty()); + assert_eq!(client.provider_names(), vec!["anthropic"]); } } diff --git a/lib/crates/fabro-workflow/src/handler/manager_loop.rs b/lib/crates/fabro-workflow/src/handler/manager_loop.rs index d6d8c98f4..95463fb34 100644 --- a/lib/crates/fabro-workflow/src/handler/manager_loop.rs +++ b/lib/crates/fabro-workflow/src/handler/manager_loop.rs @@ -206,7 +206,7 @@ impl Handler for SubWorkflowHandler { run_dir: child_logs, cancel_token: Some(cancel_token), // Child workflows are part of the parent run's event stream. - run_id: services.emitter.run_id(), + run_id: services.run.emitter.run_id(), labels: HashMap::new(), workflow_slug: None, github_app: None, @@ -227,10 +227,8 @@ impl Handler for SubWorkflowHandler { } let before_snapshot = context.snapshot(); - let emitter = Arc::clone(&services.emitter); - let sandbox = Arc::clone(&services.sandbox); + let parent_run = Arc::clone(&services.run); let registry = Arc::clone(&services.registry); - let hook_runner = services.hook_runner.clone(); let env = services.env.clone(); let inputs = services.inputs.clone(); let dry_run = services.dry_run; @@ -250,28 +248,29 @@ impl Handler for SubWorkflowHandler { // Spawn child engine let mut child_handle = tokio::spawn(async move { + let child_run = parent_run + .with_run_store(run_store.into()) + .with_cancel_requested(None); let initialized = Initialized { - graph: child_graph, - source: String::new(), - inputs, - run_options: child_run_options, - workflow_path: child_workflow_path, - workflow_bundle, - run_store: run_store.into(), - checkpoint: None, - seed_context: Some(child_context), - emitter, - sandbox, - registry, - on_node: None, + graph: child_graph, + source: String::new(), + run_options: child_run_options, + checkpoint: None, + seed_context: Some(child_context), + on_node: None, artifact_sink: Some(ArtifactSink::Store(artifact_store)), - run_control: None, - hook_runner, - env, - dry_run, - llm_client: None, - model: String::new(), - provider: fabro_llm::Provider::Anthropic, + run_control: None, + engine: Arc::new(EngineServices { + run: child_run, + registry, + git_state: std::sync::RwLock::new(None), + env, + inputs, + dry_run, + workflow_path: child_workflow_path, + workflow_bundle, + }), + model: String::new(), }; let executed = pipeline::execute(initialized).await; Ok::<_, Error>((executed.outcome?, executed.final_context)) diff --git a/lib/crates/fabro-workflow/src/handler/mod.rs b/lib/crates/fabro-workflow/src/handler/mod.rs index f8f07174e..b5cdbd408 100644 --- a/lib/crates/fabro-workflow/src/handler/mod.rs +++ b/lib/crates/fabro-workflow/src/handler/mod.rs @@ -13,163 +13,18 @@ pub mod wait; use std::any::Any; use std::collections::HashMap; -use std::path::{Path, PathBuf}; +use std::path::Path; use std::sync::Arc; -use std::sync::atomic::{AtomicBool, Ordering}; -use std::time::Duration; use async_trait::async_trait; -use fabro_agent::Sandbox; use fabro_graphviz::graph::{Graph, Node, shape_to_handler_type}; -use fabro_hooks::{HookContext, HookDecision, HookRunner}; use fabro_interview::Interviewer; -use fabro_model::Provider; -#[cfg(test)] -use fabro_store::Database; -#[cfg(test)] -use object_store::memory::InMemory; -use tokio::time; -use tokio_util::sync::CancellationToken; use crate::context::Context; use crate::error::Error; -use crate::event::Emitter; use crate::outcome::{Outcome, OutcomeExt}; -use crate::runtime_store::RunStoreHandle; -use crate::sandbox_git::GitState; -use crate::workflow_bundle::WorkflowBundle; - -/// Shared services available to all handlers during execution. -pub struct EngineServices { - pub registry: Arc, - pub emitter: Arc, - pub sandbox: Arc, - pub run_store: RunStoreHandle, - /// Git state for the current run. Set via `set_git_state` at the start of - /// `run_via_core` and read by parallel/fan-in handlers. - pub(crate) git_state: std::sync::RwLock>>, - /// Hook runner for user-defined lifecycle hooks. - pub hook_runner: Option>, - /// Environment variables from `[sandbox.env]` config, injected into command - /// nodes. - pub env: HashMap, - /// Typed values from `[run.inputs]`, available to prompt templates. - pub inputs: HashMap, - /// When true, handlers should skip real execution and return simulated - /// results. - pub dry_run: bool, - /// Optional run-scoped cancellation flag from the core executor. - pub cancel_requested: Option>, - /// Resolved default provider for the current run. - pub provider: Provider, - /// Logical path of the current workflow when running from a bundle. - pub workflow_path: Option, - /// Bundled workflows available for child-workflow resolution. - pub workflow_bundle: Option>, -} - -impl EngineServices { - /// Read the current git state (if any). - pub fn git_state(&self) -> Option> { - self.git_state.read().unwrap().clone() - } - - /// Set the git state for the current run. - pub fn set_git_state(&self, state: Option>) { - *self.git_state.write().unwrap() = state; - } - - /// Bridge the core executor's atomic cancel flag to sandbox command - /// cancellation. - pub fn sandbox_cancel_token(&self) -> Option { - sandbox_cancel_token(self.cancel_requested.clone()) - } - - /// Run lifecycle hooks and return the merged decision. - /// Returns `Proceed` if no hook runner is configured. - pub async fn run_hooks(&self, hook_context: &HookContext) -> HookDecision { - let Some(ref runner) = self.hook_runner else { - return HookDecision::Proceed; - }; - runner.run(hook_context, self.sandbox.clone(), None).await - } - - /// Test-only default: empty registry, no hooks, local sandbox at cwd. - #[cfg(test)] - #[expect( - clippy::disallowed_methods, - reason = "This test helper must initialize a current-thread runtime safely from both sync tests and #[tokio::test]." - )] - pub fn test_default() -> Self { - let store = Arc::new(Database::new( - Arc::new(InMemory::new()), - "", - Duration::from_millis(1), - None, - )); - Self { - registry: Arc::new(HandlerRegistry::new(Box::new(start::StartHandler))), - emitter: Arc::new(Emitter::default()), - sandbox: Arc::new(fabro_agent::LocalSandbox::new( - std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")), - )), - // Build the test run store on a dedicated runtime so this helper - // remains safe to call from both sync tests and #[tokio::test]. - run_store: std::thread::spawn(move || { - tokio::runtime::Builder::new_current_thread() - .enable_all() - .build() - .expect("test runtime should initialize") - .block_on(async { - store - .create_run(&fabro_types::RunId::new()) - .await - .expect("slate-backed test run store should initialize") - }) - }) - .join() - .expect("test run store thread should join") - .into(), - git_state: std::sync::RwLock::new(None), - hook_runner: None, - env: HashMap::new(), - inputs: HashMap::new(), - dry_run: false, - cancel_requested: None, - provider: Provider::Anthropic, - workflow_path: None, - workflow_bundle: None, - } - } -} - -pub(crate) fn sandbox_cancel_token( - cancel_requested: Option>, -) -> Option { - let cancel_requested = cancel_requested?; - let token = CancellationToken::new(); - - if cancel_requested.load(Ordering::Relaxed) { - token.cancel(); - return Some(token); - } - - let token_clone = token.clone(); - tokio::spawn(async move { - loop { - if token_clone.is_cancelled() { - return; - } - if cancel_requested.load(Ordering::Relaxed) { - token_clone.cancel(); - return; - } - time::sleep(Duration::from_millis(10)).await; - } - }); - - Some(token) -} +pub(crate) use crate::services::sandbox_cancel_token; +pub use crate::services::{EngineServices, RunServices}; /// The handler interface for node execution. #[async_trait] diff --git a/lib/crates/fabro-workflow/src/handler/parallel.rs b/lib/crates/fabro-workflow/src/handler/parallel.rs index 6927da366..035db5e0c 100644 --- a/lib/crates/fabro-workflow/src/handler/parallel.rs +++ b/lib/crates/fabro-workflow/src/handler/parallel.rs @@ -152,7 +152,7 @@ impl Handler for ParallelHandler { let parallel_stage_scope = StageScope::for_handler(context, &node.id); let parallel_group_id = StageId::new(node.id.clone(), parallel_stage_scope.visit); - services.emitter.emit_scoped( + services.run.emitter.emit_scoped( &Event::ParallelStarted { node_id: node.id.clone(), visit: parallel_stage_scope.visit, @@ -169,7 +169,7 @@ impl Handler for ParallelHandler { let mut hook_ctx = HookContext::new(HookEvent::ParallelStart, run_id, graph.name.clone()); set_hook_node(&mut hook_ctx, node); - let _ = services.run_hooks(&hook_ctx).await; + let _ = services.run.run_hooks(&hook_ctx).await; } let max_parallel = node .attrs @@ -184,7 +184,7 @@ impl Handler for ParallelHandler { // --- Git isolation: checkpoint "parallel base" before fan-out --- let base_sha: Option = if let Some(ref gs) = git_state { let result = git_checkpoint( - &*services.sandbox, + &*services.run.sandbox, &gs.run_id.to_string(), &node.id, "parallel_base", @@ -239,7 +239,7 @@ impl Handler for ParallelHandler { ); // Compute worktree path (each sandbox type knows its own path scheme) - let wt_path_str = services.sandbox.parallel_worktree_path( + let wt_path_str = services.run.sandbox.parallel_worktree_path( run_dir, &gs.run_id.to_string(), &node.id, @@ -254,8 +254,10 @@ impl Handler for ParallelHandler { worktree_path: wt_path_str.clone(), skip_branch_creation: false, }; - let mut wt_sandbox = WorktreeSandbox::new(Arc::clone(&services.sandbox), wt_config); - wt_sandbox.set_event_callback(Arc::clone(&services.emitter).worktree_callback()); + let mut wt_sandbox = + WorktreeSandbox::new(Arc::clone(&services.run.sandbox), wt_config); + wt_sandbox + .set_event_callback(Arc::clone(&services.run.emitter).worktree_callback()); wt_sandbox .initialize() .await @@ -267,7 +269,7 @@ impl Handler for ParallelHandler { let env: Arc = Arc::new(wt_sandbox); (env, Some(wt_path)) } else { - (Arc::clone(&services.sandbox), None) + (Arc::clone(&services.run.sandbox), None) }; branch_setups.push(BranchSetup { @@ -283,15 +285,11 @@ impl Handler for ParallelHandler { // --- Fan out: concurrent execution --- let mut handles = Vec::new(); for setup in branch_setups { + let parent_run = Arc::clone(&services.run); let registry = Arc::clone(&services.registry); - let emitter = Arc::clone(&services.emitter); - let hook_runner = services.hook_runner.clone(); - let run_store = services.run_store.clone(); let env = services.env.clone(); let inputs = services.inputs.clone(); let dry_run = services.dry_run; - let cancel_requested = services.cancel_requested.clone(); - let provider = services.provider; let workflow_path = services.workflow_path.clone(); let workflow_bundle = services.workflow_bundle.clone(); let graph = graph.clone(); @@ -317,7 +315,7 @@ impl Handler for ParallelHandler { .await .map_err(|e| Error::handler(format!("semaphore error: {e}")))?; - emitter.emit_scoped( + parent_run.emitter.emit_scoped( &Event::ParallelBranchStarted { parallel_group_id: group_id.clone(), parallel_branch_id: setup.parallel_branch_id.clone(), @@ -333,7 +331,7 @@ impl Handler for ParallelHandler { "branch target node not found: {}", setup.target_id )); - emitter.emit_scoped( + parent_run.emitter.emit_scoped( &Event::ParallelBranchCompleted { parallel_group_id: group_id.clone(), parallel_branch_id: setup.parallel_branch_id.clone(), @@ -354,17 +352,12 @@ impl Handler for ParallelHandler { }; let branch_services = EngineServices { + run: parent_run.with_sandbox(Arc::clone(&setup.sandbox)), registry: Arc::clone(®istry), - emitter: Arc::clone(&emitter), - sandbox: Arc::clone(&setup.sandbox), - run_store: run_store.clone(), git_state: std::sync::RwLock::new(None), - hook_runner: hook_runner.clone(), env: env.clone(), inputs: inputs.clone(), dry_run, - cancel_requested, - provider, workflow_path, workflow_bundle, }; @@ -412,7 +405,7 @@ impl Handler for ParallelHandler { match sha_result { Ok(r) if r.exit_code == 0 => { let sha = r.stdout.trim().to_string(); - emitter.emit_scoped( + parent_run.emitter.emit_scoped( &Event::GitCommit { node_id: Some(setup.target_id.clone()), sha: sha.clone(), @@ -427,7 +420,7 @@ impl Handler for ParallelHandler { None }; - emitter.emit_scoped( + parent_run.emitter.emit_scoped( &Event::ParallelBranchCompleted { parallel_group_id: group_id.clone(), parallel_branch_id: setup.parallel_branch_id.clone(), @@ -484,8 +477,9 @@ impl Handler for ParallelHandler { for result in &results { if let Some(ref wt_path) = result.worktree_path { let wt_str = wt_path.to_string_lossy().into_owned(); - git_remove_worktree(&*services.sandbox, &wt_str).await; + git_remove_worktree(&*services.run.sandbox, &wt_str).await; services + .run .emitter .emit(&Event::GitWorktreeRemove { path: wt_str }); } @@ -502,7 +496,7 @@ impl Handler for ParallelHandler { successful.sort_by(|a, b| a.id.cmp(&b.id)); if let Some(winner) = successful.first() { if let Some(sha) = winner.head_sha.as_ref() { - git_merge_ff_only(&*services.sandbox, sha).await; + git_merge_ff_only(&*services.run.sandbox, sha).await; } } } @@ -535,7 +529,7 @@ impl Handler for ParallelHandler { context.set(keys::PARALLEL_RESULTS, serde_json::json!(results_json)); context.set(keys::PARALLEL_BRANCH_COUNT, serde_json::json!(total)); - services.emitter.emit_scoped( + services.run.emitter.emit_scoped( &Event::ParallelCompleted { node_id: node.id.clone(), visit: parallel_stage_scope.visit, @@ -554,7 +548,7 @@ impl Handler for ParallelHandler { let mut hook_ctx = HookContext::new(HookEvent::ParallelComplete, run_id, graph.name.clone()); set_hook_node(&mut hook_ctx, node); - let _ = services.run_hooks(&hook_ctx).await; + let _ = services.run.run_hooks(&hook_ctx).await; } // Evaluate join policy @@ -689,13 +683,13 @@ mod tests { async fn parallel_handler_with_branches() { let store = test_store(); let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); - let services = EngineServices { - emitter: Arc::new(crate::event::Emitter::new(fixtures::RUN_1)), - run_store: run_store.clone().into(), - ..EngineServices::test_default() - }; + let mut services = EngineServices::test_default(); + services.run = services + .run + .with_emitter(Arc::new(crate::event::Emitter::new(fixtures::RUN_1))) + .with_run_store(run_store.clone().into()); let logger = crate::event::StoreProgressLogger::new(run_store.clone()); - logger.register(services.emitter.as_ref()); + logger.register(services.run.emitter.as_ref()); let mut node = Node::new("par"); node.attrs.insert( "shape".to_string(), @@ -741,13 +735,13 @@ mod tests { async fn parallel_handler_stores_results_in_run_store() { let store = test_store(); let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); - let services = EngineServices { - emitter: Arc::new(crate::event::Emitter::new(fixtures::RUN_1)), - run_store: run_store.clone().into(), - ..EngineServices::test_default() - }; + let mut services = EngineServices::test_default(); + services.run = services + .run + .with_emitter(Arc::new(crate::event::Emitter::new(fixtures::RUN_1))) + .with_run_store(run_store.clone().into()); let logger = crate::event::StoreProgressLogger::new(run_store.clone()); - logger.register(services.emitter.as_ref()); + logger.register(services.run.emitter.as_ref()); let mut node = Node::new("par"); node.attrs.insert( "shape".to_string(), diff --git a/lib/crates/fabro-workflow/src/handler/prompt.rs b/lib/crates/fabro-workflow/src/handler/prompt.rs index df5e1f1ca..786721435 100644 --- a/lib/crates/fabro-workflow/src/handler/prompt.rs +++ b/lib/crates/fabro-workflow/src/handler/prompt.rs @@ -61,13 +61,13 @@ impl Handler for PromptHandler { // 1b. Discover project docs for system prompt when project_memory is enabled let system_prompt = if node.project_memory() { - let working_dir = services.sandbox.working_directory(); + let working_dir = services.run.sandbox.working_directory(); let provider = node .provider() .and_then(|s| s.parse::().ok()) - .unwrap_or(services.provider); + .unwrap_or(services.run.provider); let docs = fabro_agent::discover_memory( - &*services.sandbox, + &*services.run.sandbox, working_dir, working_dir, provider, @@ -86,10 +86,10 @@ impl Handler for PromptHandler { let prompt_provider = node .provider() .map(String::from) - .or_else(|| Some(services.provider.as_str().to_string())); + .or_else(|| Some(services.run.provider.as_str().to_string())); let prompt_model = node.model().map(String::from); let stage_scope = StageScope::for_handler(context, &node.id); - services.emitter.emit_scoped( + services.run.emitter.emit_scoped( &Event::Prompt { stage: node.id.clone(), visit: stage_scope.visit, @@ -138,10 +138,10 @@ impl Handler for PromptHandler { let response_provider = node .provider() .map(String::from) - .or_else(|| Some(services.provider.as_str().to_string())) + .or_else(|| Some(services.run.provider.as_str().to_string())) .unwrap_or_default(); - services.emitter.emit_scoped( + services.run.emitter.emit_scoped( &Event::PromptCompleted { node_id: node.id.clone(), response: response_text.clone(), @@ -208,13 +208,13 @@ mod tests { ) { let store = test_store(); let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); - let services = EngineServices { - emitter: Arc::new(crate::event::Emitter::new(fixtures::RUN_1)), - run_store: run_store.clone().into(), - ..EngineServices::test_default() - }; + let mut services = EngineServices::test_default(); + services.run = services + .run + .with_emitter(Arc::new(crate::event::Emitter::new(fixtures::RUN_1))) + .with_run_store(run_store.clone().into()); let logger = crate::event::StoreProgressLogger::new(run_store.clone()); - logger.register(services.emitter.as_ref()); + logger.register(services.run.emitter.as_ref()); (services, run_store, logger) } diff --git a/lib/crates/fabro-workflow/src/lib.rs b/lib/crates/fabro-workflow/src/lib.rs index 1d05277b3..77483e746 100644 --- a/lib/crates/fabro-workflow/src/lib.rs +++ b/lib/crates/fabro-workflow/src/lib.rs @@ -150,6 +150,7 @@ pub mod run_options; pub mod run_status; pub mod runtime_store; pub mod sandbox_git; +pub mod services; #[doc(hidden)] pub mod test_support; #[doc(hidden)] diff --git a/lib/crates/fabro-workflow/src/lifecycle/artifact.rs b/lib/crates/fabro-workflow/src/lifecycle/artifact.rs index cd6eb06a8..c8bdfbc6d 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/artifact.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/artifact.rs @@ -121,11 +121,11 @@ impl RunLifecycle for ArtifactLifecycle { ) .await { - self.emitter.emit(&Event::RunNotice { - level: RunNoticeLevel::Warn, - code: "artifact_upload_failed".to_string(), - message: format!("[node: {node_id}] artifact upload failed: {err}"), - }); + self.emitter.notice( + RunNoticeLevel::Warn, + "artifact_upload_failed", + format!("[node: {node_id}] artifact upload failed: {err}"), + ); return Ok(()); } let scope = stage_scope_for(state, node_id); @@ -147,11 +147,11 @@ impl RunLifecycle for ArtifactLifecycle { } Ok(_) => {} // no files collected Err(e) => { - self.emitter.emit(&Event::RunNotice { - level: RunNoticeLevel::Warn, - code: "artifact_collection_failed".to_string(), - message: format!("[node: {node_id}] artifact collection failed: {e}"), - }); + self.emitter.notice( + RunNoticeLevel::Warn, + "artifact_collection_failed", + format!("[node: {node_id}] artifact collection failed: {e}"), + ); } } @@ -170,11 +170,11 @@ impl RunLifecycle for ArtifactLifecycle { if let Err(e) = offload_large_values(&mut result.outcome.context_updates, &self.run_store).await { - self.emitter.emit(&Event::RunNotice { - level: RunNoticeLevel::Warn, - code: "artifact_offload_failed".to_string(), - message: format!("[node: {node_id}] artifact offload failed: {e}"), - }); + self.emitter.notice( + RunNoticeLevel::Warn, + "artifact_offload_failed", + format!("[node: {node_id}] artifact offload failed: {e}"), + ); } normalize_durable_updates(&mut result.outcome.context_updates); @@ -183,11 +183,11 @@ impl RunLifecycle for ArtifactLifecycle { if let Err(e) = sync_artifacts_to_env(&mut result.outcome.context_updates, &*self.sandbox).await { - self.emitter.emit(&Event::RunNotice { - level: RunNoticeLevel::Warn, - code: "artifact_sync_failed".to_string(), - message: format!("[node: {node_id}] artifact sync failed: {e}"), - }); + self.emitter.notice( + RunNoticeLevel::Warn, + "artifact_sync_failed", + format!("[node: {node_id}] artifact sync failed: {e}"), + ); } Ok(()) diff --git a/lib/crates/fabro-workflow/src/node_handler.rs b/lib/crates/fabro-workflow/src/node_handler.rs index 98018ffda..0ed609acd 100644 --- a/lib/crates/fabro-workflow/src/node_handler.rs +++ b/lib/crates/fabro-workflow/src/node_handler.rs @@ -43,8 +43,8 @@ impl NodeHandler for WorkflowNodeHandler { let wf_context = artifact::resolve_context_for_execution( context, - &self.services.run_store, - &*self.services.sandbox, + &self.services.run.run_store, + &*self.services.run.sandbox, &self.run_dir, ) .await diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index ff21b2e8f..7cff959ba 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -723,7 +723,7 @@ impl RunSession { let mut initialized = Box::pin(pipeline::initialize(persisted, init_options)).await?; initialized.on_node = on_node; - let sandbox_for_cleanup = Arc::clone(&initialized.sandbox); + let sandbox_for_cleanup = Arc::clone(&initialized.engine.run.sandbox); let cleanup_guard = scopeguard::guard((), move |()| { if preserve_sandbox { return; @@ -745,17 +745,13 @@ impl RunSession { let retro_opts = RetroOptions { run_id: executed.run_options.run_id, - run_store: executed.run_store.clone(), + services: Arc::clone(&executed.engine.run), workflow_name: executed.graph.name.clone(), goal: executed.graph.goal().to_string(), run_dir: executed.run_options.run_dir.clone(), - sandbox: Arc::clone(&executed.sandbox), - emitter: Some(Arc::clone(&executed.emitter)), failed, run_duration_ms: executed.duration_ms, enabled: self.retro_enabled, - llm_client: executed.llm_client.clone(), - provider: executed.provider, model: executed.model.clone(), }; @@ -766,15 +762,11 @@ impl RunSession { let finalize_opts = FinalizeOptions { run_dir: retroed.run_options.run_dir.clone(), run_id: retroed.run_options.run_id, - run_store: retroed.run_store.clone(), workflow_name: retroed.graph.name.clone(), - hook_runner: retroed.hook_runner.clone(), preserve_sandbox: self.preserve_sandbox, last_git_sha: last_git_sha.lock().unwrap().clone(), }; let pr_opts = PullRequestOptions { - run_dir: retroed.run_options.run_dir.clone(), - run_store: retroed.run_store.clone(), pr_config: self.pr_config, github_app: self.pr_github_app, origin_url: self.pr_origin_url, diff --git a/lib/crates/fabro-workflow/src/pipeline/execute.rs b/lib/crates/fabro-workflow/src/pipeline/execute.rs index cf42d40d1..b6b045a80 100644 --- a/lib/crates/fabro-workflow/src/pipeline/execute.rs +++ b/lib/crates/fabro-workflow/src/pipeline/execute.rs @@ -13,7 +13,6 @@ use crate::context::{self, Context}; use crate::error::Error; use crate::event::Event; use crate::graph::WorkflowGraph; -use crate::handler::EngineServices; use crate::lifecycle::WorkflowLifecycle; use crate::node_handler::WorkflowNodeHandler; use crate::outcome::{Outcome, StageStatus}; @@ -37,29 +36,16 @@ pub async fn execute(init: Initialized) -> Executed { let Initialized { graph, source: _, - inputs, run_options, - workflow_path, - workflow_bundle, - run_store, checkpoint, seed_context, - emitter, - sandbox, - registry, on_node, artifact_sink, run_control, - hook_runner, - env, - dry_run, - llm_client, + engine, model, - provider, } = init; - let service_inputs = inputs; - let mut checkpoint = checkpoint; if let Some(cp) = checkpoint.as_mut() { artifact::normalize_checkpoint_for_resume(cp); @@ -80,37 +66,22 @@ pub async fn execute(init: Initialized) -> Executed { git_author: run_options.git_author(), })) }); - - let shared_services = Arc::new(EngineServices { - registry, - emitter: Arc::clone(&emitter), - sandbox: Arc::clone(&sandbox), - run_store: run_store.clone(), - git_state: std::sync::RwLock::new(git_state), - hook_runner: hook_runner.clone(), - env, - inputs: service_inputs, - dry_run, - cancel_requested: run_options.cancel_token.clone(), - provider, - workflow_path, - workflow_bundle, - }); + engine.set_git_state(git_state); let handler = Arc::new(WorkflowNodeHandler { - services: shared_services, + services: Arc::clone(&engine), run_dir: run_options.run_dir.clone(), graph: Arc::clone(&graph_arc), }); let settings_arc = Arc::new(run_options.clone()); let lifecycle = WorkflowLifecycle::new( - &emitter, - hook_runner.clone(), - &sandbox, + &engine.run.emitter, + engine.run.hook_runner.clone(), + &engine.run.sandbox, graph_arc, &run_options.run_dir, - &run_store, + &engine.run.run_store, artifact_sink, &settings_arc, checkpoint.is_some(), @@ -168,15 +139,10 @@ pub async fn execute(init: Initialized) -> Executed { graph, outcome: Err(err), run_options, - run_store, - hook_runner, - emitter, - sandbox, duration_ms: crate::millis_u64(start.elapsed()), final_context: seed_context_from_checkpoint(checkpoint.as_ref()), - llm_client, + engine, model, - provider, }; } } @@ -193,15 +159,10 @@ pub async fn execute(init: Initialized) -> Executed { graph, outcome: Err(err), run_options, - run_store, - hook_runner, - emitter, - sandbox, duration_ms: crate::millis_u64(start.elapsed()), final_context: seed, - llm_client, + engine, model, - provider, }; } } @@ -213,15 +174,10 @@ pub async fn execute(init: Initialized) -> Executed { graph, outcome: Err(err), run_options, - run_store, - hook_runner, - emitter, - sandbox, duration_ms: crate::millis_u64(start.elapsed()), final_context: Context::new(), - llm_client, + engine, model, - provider, }; } } @@ -243,7 +199,7 @@ pub async fn execute(init: Initialized) -> Executed { let stall_shutdown = if let (Some(stall_timeout), Some(ref token)) = (stall_timeout_opt, &stall_token) { let shutdown = CancellationToken::new(); - let emitter = Arc::clone(&emitter); + let emitter = Arc::clone(&engine.run.emitter); let token_clone = token.clone(); let shutdown_clone = shutdown.clone(); emitter.touch(); @@ -316,7 +272,7 @@ pub async fn execute(init: Initialized) -> Executed { Err(fabro_core::Error::StallTimeout { node_id }) => { let stall_timeout = graph.stall_timeout().unwrap_or_default(); let idle_secs = stall_timeout.as_secs(); - emitter.emit(&Event::StallWatchdogTimeout { + engine.run.emitter.emit(&Event::StallWatchdogTimeout { node: node_id.clone(), idle_seconds: idle_secs, }); @@ -340,15 +296,10 @@ pub async fn execute(init: Initialized) -> Executed { graph, outcome, run_options, - run_store, - hook_runner, - emitter, - sandbox, duration_ms, final_context, - llm_client, + engine, model, - provider, } } diff --git a/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs b/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs index 7df826cf3..e8a97bd94 100644 --- a/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs +++ b/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs @@ -535,6 +535,8 @@ async fn execute_saves_checkpoint() { let executed = execute_test_run(dir.path(), simple_graph(), "test-run").await; assert!( executed + .engine + .run .run_store .state() .await @@ -586,6 +588,8 @@ async fn execute_mirrors_graph_goal_to_context() { let dir = tempfile::tempdir().unwrap(); let executed = execute_test_run(dir.path(), simple_graph(), "test-run").await; let cp = executed + .engine + .run .run_store .state() .await @@ -632,6 +636,8 @@ async fn execute_conditional_routing_uses_unconditional_success_path() { let executed = execute_test_run(dir.path(), g, "test-run").await; let cp = executed + .engine + .run .run_store .state() .await @@ -653,7 +659,7 @@ async fn execute_persists_start_record_and_node_status() { }); let executed = execute_test_run_with_options(run_options, simple_graph(), None).await; - let state = executed.run_store.state().await.unwrap(); + let state = executed.engine.run.run_store.state().await.unwrap(); let start = state.start.as_ref().unwrap(); assert_eq!(start.run_id, test_run_id("test-run")); assert_eq!( @@ -712,7 +718,7 @@ async fn timeout_causes_fail_status_record() { Some(Arc::new(registry)), ) .await; - let state = executed.run_store.state().await.unwrap(); + let state = executed.engine.run.run_store.state().await.unwrap(); let status = state .node(&fabro_store::StageId::new("work", 1)) .unwrap() diff --git a/lib/crates/fabro-workflow/src/pipeline/finalize.rs b/lib/crates/fabro-workflow/src/pipeline/finalize.rs index df87ec021..470e53381 100644 --- a/lib/crates/fabro-workflow/src/pipeline/finalize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/finalize.rs @@ -1,11 +1,9 @@ -use std::sync::Arc; - -use fabro_hooks::{HookContext, HookEvent, HookRunner}; +use fabro_hooks::{HookContext, HookEvent}; use fabro_types::{BilledTokenCounts, EventBody}; use super::types::{Concluded, FinalizeOptions, Retroed}; use crate::error::Error; -use crate::event::{Emitter, Event, RunNoticeLevel}; +use crate::event::{Event, RunNoticeLevel}; use crate::git::MetadataStore; use crate::outcome::{Outcome, OutcomeExt, StageStatus}; use crate::records::{Checkpoint, Conclusion, StageSummary}; @@ -14,19 +12,7 @@ use crate::run_options::RunOptions; use crate::run_status::{FailureReason, RunStatus, SuccessReason}; use crate::runtime_store::RunStoreHandle; use crate::sandbox_git::{git_diff_with_timeout, git_push_host}; - -fn emit_run_notice( - emitter: &Emitter, - level: RunNoticeLevel, - code: impl Into, - message: impl Into, -) { - emitter.emit(&Event::RunNotice { - level, - code: code.into(), - message: message.into(), - }); -} +use crate::services::RunServices; pub fn classify_engine_result( engine_result: &Result, @@ -196,21 +182,19 @@ pub async fn write_finalize_commit( /// workspace can't stall downstream consumers waiting on the terminal event. async fn compute_final_patch( run_options: &RunOptions, - sandbox: &dyn fabro_agent::Sandbox, + services: &RunServices, status: StageStatus, - emitter: &Emitter, ) -> Option { let base_sha = run_options.git.as_ref().and_then(|g| g.base_sha.clone())?; let timeout_ms = match status { StageStatus::Success | StageStatus::PartialSuccess => 30_000, _ => 10_000, }; - match git_diff_with_timeout(sandbox, &base_sha, timeout_ms).await { + match git_diff_with_timeout(&*services.sandbox, &base_sha, timeout_ms).await { Ok(patch) if !patch.is_empty() => Some(patch), Ok(_) => None, Err(err) => { - emit_run_notice( - emitter, + services.emitter.notice( RunNoticeLevel::Warn, "git_diff_failed", format!("final diff failed: {err}"), @@ -287,20 +271,8 @@ pub(crate) fn build_terminal_event( } } -async fn run_hooks( - hook_runner: Option<&HookRunner>, - hook_context: &HookContext, - sandbox: Arc, -) { - let Some(runner) = hook_runner else { - return; - }; - let _ = runner.run(hook_context, sandbox, None).await; -} - async fn cleanup_sandbox( - hook_runner: Option>, - sandbox: Arc, + services: &RunServices, run_id: &fabro_types::RunId, workflow_name: &str, preserve: bool, @@ -310,9 +282,9 @@ async fn cleanup_sandbox( *run_id, workflow_name.to_string(), ); - run_hooks(hook_runner.as_deref(), &hook_ctx, Arc::clone(&sandbox)).await; + let _ = services.run_hooks(&hook_ctx).await; if !preserve { - sandbox.cleanup().await?; + services.sandbox.cleanup().await?; } Ok(()) } @@ -331,23 +303,20 @@ pub async fn finalize(retroed: Retroed, options: &FinalizeOptions) -> Result Result Result Result Result, - message: impl Into, -) { - emitter.emit(&Event::RunNotice { - level, - code: code.into(), - message: message.into(), - }); -} - async fn resolve_worktree_plan(options: &mut InitOptions) -> Result, Error> { let Some(worktree_mode) = options.worktree_mode else { options.run_options.display_base_sha = None; @@ -111,8 +101,7 @@ async fn resolve_worktree_plan(options: &mut InitOptions) -> Result None, }; if let Some(env_name) = env_name { - emit_run_notice( - &options.emitter, + options.emitter.notice( RunNoticeLevel::Warn, "dirty_worktree", format!("Uncommitted changes will not be included in the {env_name}."), @@ -151,14 +140,12 @@ async fn resolve_worktree_plan(options: &mut InitOptions) -> Result emit_run_notice( - &options.emitter, + Ok(()) => options.emitter.notice( RunNoticeLevel::Info, "git_push_succeeded", format!("{branch} (synced local commits to remote)"), ), - Err(e) => emit_run_notice( - &options.emitter, + Err(e) => options.emitter.notice( RunNoticeLevel::Warn, "git_push_failed", format!("Failed to push {branch} to origin: {e}"), @@ -188,8 +175,7 @@ async fn resolve_worktree_plan(options: &mut InitOptions) -> Result { - emit_run_notice( - &options.emitter, + options.emitter.notice( RunNoticeLevel::Warn, "worktree_setup_failed", format!("Git worktree setup failed ({e}), running without worktree."), @@ -264,8 +250,7 @@ async fn build_sandbox_env( Ok(token) => { env.insert("GITHUB_TOKEN".to_string(), token); } - Err(e) => emit_run_notice( - emitter, + Err(e) => emitter.notice( RunNoticeLevel::Warn, "github_token_failed", format!("Failed to mint GitHub token: {e}"), @@ -283,12 +268,13 @@ async fn build_registry( interviewer: Arc, sandbox_env: &HashMap, graph: &graph::Graph, - vault: Option>>, -) -> Result<(Arc, Option, bool), Error> { + llm_source: Arc, + cli_resolver: Option, +) -> Result<(Arc, bool), Error> { let build_no_backend = || Arc::new(default_registry(Arc::clone(&interviewer), || None)); if spec.dry_run { - return Ok((build_no_backend(), None, true)); + return Ok((build_no_backend(), true)); } let graph_needs_llm = graph @@ -296,10 +282,8 @@ async fn build_registry( .values() .any(|n| graph::is_llm_handler_type(n.handler_type())); - let resolver = vault.map(CredentialResolver::new); - - match build_llm_client(resolver.as_ref()).await { - Ok(result) if result.client.provider_names().is_empty() => { + match llm_source.resolve().await { + Ok(result) if result.credentials.is_empty() => { if graph_needs_llm { let detail = (!result.auth_issues.is_empty()).then(|| { result @@ -317,38 +301,25 @@ async fn build_registry( "{prefix}. Set ANTHROPIC_API_KEY or OPENAI_API_KEY, or pass --dry-run to simulate." ))); } - Ok((build_no_backend(), None, false)) + Ok((build_no_backend(), false)) } - Ok(result) => { + Ok(_result) => { let env = sandbox_env.clone(); let model = spec.model.clone(); let provider = spec.provider; let fallback_chain = spec.fallback_chain.clone(); let mcp_servers = spec.mcp_servers.clone(); - let client = result.client; + let llm_source_for_api = Arc::clone(&llm_source); let registry = Arc::new(default_registry(interviewer, move || { - let api = resolver - .clone() - .map_or_else( - || { - AgentApiBackend::new_from_env( - model.clone(), - provider, - fallback_chain.clone(), - ) - }, - |resolver| { - AgentApiBackend::new( - model.clone(), - provider, - fallback_chain.clone(), - resolver, - ) - }, - ) - .with_env(env.clone()) - .with_mcp_servers(mcp_servers.clone()); - let cli = resolver + let api = AgentApiBackend::new( + model.clone(), + provider, + fallback_chain.clone(), + Arc::clone(&llm_source_for_api), + ) + .with_env(env.clone()) + .with_mcp_servers(mcp_servers.clone()); + let cli = cli_resolver .clone() .map_or_else( || AgentCliBackend::new_from_env(model.clone(), provider), @@ -357,7 +328,7 @@ async fn build_registry( .with_env(env.clone()); Some(Box::new(BackendRouter::new(Box::new(api), cli))) })); - Ok((registry, Some(client), false)) + Ok((registry, false)) } Err(e) => { if graph_needs_llm { @@ -365,11 +336,18 @@ async fn build_registry( "Failed to initialize LLM client: {e}. Set ANTHROPIC_API_KEY or OPENAI_API_KEY, or pass --dry-run to simulate.", ))); } - Ok((build_no_backend(), None, false)) + Ok((build_no_backend(), false)) } } } +fn build_llm_source(vault: Option>>) -> Arc { + match vault { + Some(vault) => Arc::new(VaultCredentialSource::new(vault)), + None => Arc::new(EnvCredentialSource::new()), + } +} + async fn resolve_devcontainer(options: &mut InitOptions) -> Result<(), Error> { let Some(devcontainer) = options.devcontainer.clone() else { return Ok(()); @@ -397,27 +375,15 @@ async fn resolve_devcontainer(options: &mut InitOptions) -> Result<(), Error> { .apply_devcontainer_snapshot(devcontainer_to_snapshot_config(&config)); let timeout = std::time::Duration::from_mins(5); - for command in &config.initialize_commands { - let shell_commands = match command { - fabro_devcontainer::Command::Shell(shell) => vec![shell.clone()], - fabro_devcontainer::Command::Args(args) => { - vec![ - args.iter() - .map(|arg| try_quote(arg).unwrap_or_else(|_| arg.into()).to_string()) - .collect::>() - .join(" "), - ] - } - fabro_devcontainer::Command::Parallel(commands) => commands.values().cloned().collect(), - }; - - for shell_command in shell_commands { + let run_shell = |shell_command: String| { + let cwd = devcontainer.resolve_dir.clone(); + async move { let output = tokio_timeout( timeout, TokioCommand::new("sh") .arg("-c") .arg(&shell_command) - .current_dir(&devcontainer.resolve_dir) + .current_dir(&cwd) .output(), ) .await @@ -442,6 +408,25 @@ async fn resolve_devcontainer(options: &mut InitOptions) -> Result<(), Error> { "Devcontainer initializeCommand failed (exit code {code}): {shell_command}\n{stderr}" ))); } + Ok::<(), Error>(()) + } + }; + + for command in &config.initialize_commands { + match command { + fabro_devcontainer::Command::Shell(shell) => run_shell(shell.clone()).await?, + fabro_devcontainer::Command::Args(args) => { + let shell_command = args + .iter() + .map(|arg| try_quote(arg).unwrap_or_else(|_| arg.into()).to_string()) + .collect::>() + .join(" "); + run_shell(shell_command).await?; + } + fabro_devcontainer::Command::Parallel(commands) => { + let futures = commands.values().cloned().map(&run_shell); + try_join_all(futures).await?; + } } } @@ -469,10 +454,16 @@ pub async fn initialize( options.run_options.run_dir = run_dir.clone(); options.run_options.git = options.git.clone(); + let llm_source = build_llm_source(options.vault.clone()); + let cli_resolver = options.vault.clone().map(CredentialResolver::new); + let hook_runner = if options.hooks.hooks.is_empty() { None } else { - Some(Arc::new(HookRunner::new(options.hooks.clone()))) + Some(Arc::new(HookRunner::new( + options.hooks.clone(), + Arc::clone(&llm_source), + ))) }; resolve_devcontainer(&mut options).await?; @@ -512,8 +503,7 @@ pub async fn initialize( Arc::new(ReadBeforeWriteSandbox::new(Arc::new(worktree))) } Err(e) => { - emit_run_notice( - &options.emitter, + options.emitter.notice( RunNoticeLevel::Warn, "worktree_setup_failed", format!("Git worktree setup failed ({e}), running without worktree."), @@ -584,20 +574,20 @@ pub async fn initialize( &options.emitter, ) .await?; - let (registry, llm_client, effective_dry_run) = - if let Some(registry) = options.registry_override.clone() { - // A caller-supplied registry owns execution behavior for its handlers. - (registry, None, options.dry_run) - } else { - build_registry( - &options.llm, - Arc::clone(&options.interviewer), - &env, - &graph, - options.vault.clone(), - ) - .await? - }; + let (registry, effective_dry_run) = if let Some(registry) = options.registry_override.clone() { + // A caller-supplied registry owns execution behavior for its handlers. + (registry, options.dry_run) + } else { + build_registry( + &options.llm, + Arc::clone(&options.interviewer), + &env, + &graph, + Arc::clone(&llm_source), + cli_resolver, + ) + .await? + }; if effective_dry_run { use fabro_types::settings::run::RunMode; @@ -710,30 +700,39 @@ pub async fn initialize( .await?; } + let run_services = RunServices::new( + options.run_store.clone(), + Arc::clone(&options.emitter), + Arc::clone(&sandbox), + hook_runner.clone(), + options.run_options.cancel_token.clone(), + options.llm.provider, + Arc::clone(&llm_source), + ); + let engine = Arc::new(EngineServices { + run: Arc::clone(&run_services), + registry, + git_state: std::sync::RwLock::new(None), + env, + inputs: options.run_options.settings.run.inputs.clone(), + dry_run: options.dry_run, + workflow_path: options.workflow_path.clone(), + workflow_bundle: options.workflow_bundle.clone(), + }); + scopeguard::ScopeGuard::into_inner(cleanup_guard); Ok(Initialized { graph, source, - inputs: options.run_options.settings.run.inputs.clone(), run_options: options.run_options, - workflow_path: options.workflow_path, - workflow_bundle: options.workflow_bundle, - run_store: options.run_store, checkpoint: options.checkpoint, seed_context: options.seed_context, - emitter: options.emitter, - sandbox, - registry, on_node: None, artifact_sink: options.artifact_sink, run_control: options.run_control, - hook_runner, - env, - dry_run: options.dry_run, - llm_client, + engine, model: options.llm.model, - provider: options.llm.provider, }) } @@ -927,15 +926,28 @@ mod tests { assert_eq!(initialized.run_options.run_dir, run_dir); assert_eq!(initialized.source, source); - assert!(initialized.hook_runner.is_none()); + assert!(initialized.engine.run.hook_runner.is_none()); assert_eq!( - initialized.env.get("TEST_KEY").map(String::as_str), + initialized.engine.env.get("TEST_KEY").map(String::as_str), Some("value") ); - assert!(initialized.dry_run); + assert!(initialized.engine.dry_run); assert_eq!(initialized.model, "test-model"); - assert_eq!(initialized.provider, fabro_llm::Provider::Anthropic); - assert!(initialized.llm_client.is_none()); + assert_eq!( + initialized.engine.run.provider, + fabro_llm::Provider::Anthropic + ); + assert!( + initialized + .engine + .run + .llm_source + .resolve() + .await + .unwrap() + .credentials + .is_empty() + ); } #[tokio::test] @@ -957,8 +969,9 @@ mod tests { ) .unwrap(); let (graph, _) = llm_graph(); + let vault = Arc::new(AsyncRwLock::new(vault)); - let (_, llm_client, effective_dry_run) = build_registry( + let (_registry, effective_dry_run) = build_registry( &LlmSpec { model: "claude-opus-4-6".to_string(), provider: fabro_llm::Provider::Anthropic, @@ -969,13 +982,13 @@ mod tests { Arc::new(AutoApproveInterviewer), &HashMap::new(), &graph, - Some(Arc::new(AsyncRwLock::new(vault))), + Arc::new(VaultCredentialSource::new(Arc::clone(&vault))), + Some(CredentialResolver::new(vault)), ) .await .unwrap(); assert!(!effective_dry_run); - assert!(llm_client.unwrap().provider_names().contains(&"anthropic")); } #[tokio::test] diff --git a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs index 734ca78af..29923abfc 100644 --- a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs +++ b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs @@ -1,5 +1,9 @@ +use std::sync::Arc; + +use fabro_auth::CredentialSource; use fabro_github::{self as github_app, GitHubCredentials, ssh_url_to_https}; use fabro_graphviz::parser; +use fabro_llm::client::Client; use fabro_llm::generate::{GenerateParams, generate}; use fabro_retro::retro::Retro; use fabro_store::RunProjection; @@ -9,7 +13,7 @@ use fabro_util::text::strip_goal_decoration; use tracing::{debug, info}; use super::types::{Concluded, Finalized, PullRequestOptions}; -use crate::event::{Emitter, Event, RunNoticeLevel}; +use crate::event::{Event, RunNoticeLevel}; use crate::outcome::{StageStatus, format_cost as outcome_format_cost}; use crate::records::{Conclusion, RunSpec}; use crate::runtime_store::RunStoreHandle; @@ -270,19 +274,6 @@ fn assemble_pr_body( parts.join("\n") } -fn emit_run_notice( - emitter: &Emitter, - level: RunNoticeLevel, - code: impl Into, - message: impl Into, -) { - emitter.emit(&Event::RunNotice { - level, - code: code.into(), - message: message.into(), - }); -} - async fn load_pull_request_diff(run_store: &RunStoreHandle) -> String { run_store .state() @@ -302,23 +293,26 @@ pub async fn build_pr_body( goal: &str, model: &str, run_store: &RunStoreHandle, + llm_source: &dyn CredentialSource, conclusion: Option<&Conclusion>, +) -> Result { + let client = Client::from_source(llm_source) + .await + .map_err(|e| format!("Failed to create LLM client: {e}"))?; + + build_pr_body_with_client(diff, goal, model, run_store, conclusion, Arc::new(client)).await +} + +async fn build_pr_body_with_client( + diff: &str, + goal: &str, + model: &str, + run_store: &RunStoreHandle, + conclusion: Option<&Conclusion>, + client: Arc, ) -> Result { debug!("Building PR body"); - let loaded_conclusion = if conclusion.is_none() { - run_store - .state() - .await - .inspect_err(|err| { - tracing::warn!(error = %err, "Failed to load conclusion from store for PR body"); - }) - .ok() - .and_then(|state| state.conclusion) - } else { - None - }; - let conclusion = conclusion.or(loaded_conclusion.as_ref()); let run_state = run_store .state() .await @@ -326,6 +320,15 @@ pub async fn build_pr_body( tracing::warn!(error = %err, "Failed to load run state from store for PR body"); }) .ok(); + let loaded_conclusion = conclusion + .is_none() + .then(|| { + run_state + .as_ref() + .and_then(|state| state.conclusion.clone()) + }) + .flatten(); + let conclusion = conclusion.or(loaded_conclusion.as_ref()); let plan_text = run_state.as_ref().and_then(read_plan_text); let retro = run_state.as_ref().and_then(|state| state.retro.clone()); let run_spec = run_state.as_ref().and_then(|state| state.spec.clone()); @@ -363,7 +366,9 @@ pub async fn build_pr_body( format!("Goal: {goal}\n\nDiff:\n```\n{truncated_diff}\n```") }; - let params = GenerateParams::new(model).system(system).prompt(prompt); + let params = GenerateParams::new(model, client) + .system(system) + .prompt(prompt); let result = generate(params) .await @@ -413,6 +418,7 @@ pub async fn maybe_open_pull_request( draft: bool, auto_merge: Option, run_store: &RunStoreHandle, + llm_source: &dyn CredentialSource, conclusion: Option<&Conclusion>, ) -> Result, String> { if diff.is_empty() { @@ -423,7 +429,7 @@ pub async fn maybe_open_pull_request( let https_url = ssh_url_to_https(origin_url); let (owner, repo) = github_app::parse_github_owner_repo(&https_url)?; - let body = build_pr_body(diff, goal, model, run_store, conclusion).await?; + let body = build_pr_body(diff, goal, model, run_store, llm_source, conclusion).await?; let body = truncate_pr_body(&body); let title = pr_title_from_goal(goal); @@ -491,13 +497,11 @@ pub async fn maybe_open_pull_request( /// completes. pub async fn pull_request(concluded: Concluded, options: &PullRequestOptions) -> Finalized { let Concluded { - run_id, outcome, conclusion, - pushed_branch, graph, run_options, - emitter, + services, } = concluded; let mut pr_url = None; @@ -511,10 +515,10 @@ pub async fn pull_request(concluded: Concluded, options: &PullRequestOptions) -> result.status, StageStatus::Success | StageStatus::PartialSuccess ) { - let diff = load_pull_request_diff(&options.run_store).await; + 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, - pushed_branch.as_deref(), + run_options.run_branch(), &options.github_app, &options.origin_url, ) { @@ -536,13 +540,14 @@ pub async fn pull_request(concluded: Concluded, options: &PullRequestOptions) -> &options.model, pr_cfg.draft, auto_merge, - &options.run_store, + &services.run_store, + services.llm_source.as_ref(), Some(&conclusion), ) .await { Ok(Some(record)) => { - emitter.emit(&Event::PullRequestCreated { + services.emitter.emit(&Event::PullRequestCreated { pr_url: record.html_url.clone(), pr_number: record.number, owner: record.owner.clone(), @@ -556,9 +561,10 @@ pub async fn pull_request(concluded: Concluded, options: &PullRequestOptions) -> } Ok(None) => {} Err(e) => { - emitter.emit(&Event::PullRequestFailed { error: e.clone() }); - emit_run_notice( - &emitter, + services + .emitter + .emit(&Event::PullRequestFailed { error: e.clone() }); + services.emitter.notice( RunNoticeLevel::Warn, "pull_request_failed", format!("PR creation failed: {e}"), @@ -571,10 +577,10 @@ pub async fn pull_request(concluded: Concluded, options: &PullRequestOptions) -> } Finalized { - run_id, + run_id: run_options.run_id, outcome, conclusion, - pushed_branch, + pushed_branch: run_options.run_branch().map(str::to_string), pr_url, } } @@ -583,34 +589,43 @@ pub async fn pull_request(concluded: Concluded, options: &PullRequestOptions) -> mod tests { use std::collections::HashMap; use std::path::PathBuf; - use std::sync::{Arc, Once}; + use std::sync::Arc; use std::time::Duration; use chrono::Utc; + 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, set_default_client}; use fabro_retro::retro::{ AggregateStats, FrictionKind, FrictionPoint, OpenItem, OpenItemKind, StageRetro, }; use fabro_store::Database; 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}; use crate::records::StageSummary; struct MockProvider { + name: String, response_text: String, } impl MockProvider { - fn new(text: &str) -> Self { + fn new(name: &str, text: &str) -> Self { Self { + name: name.to_string(), response_text: text.to_string(), } } @@ -619,7 +634,7 @@ mod tests { #[async_trait::async_trait] impl ProviderAdapter for MockProvider { fn name(&self) -> &str { - "mock" + &self.name } async fn complete(&self, _request: &Request) -> Result { @@ -681,17 +696,54 @@ mod tests { )) } - fn install_mock_llm() { - static INIT: Once = Once::new(); + fn explicit_client(provider_name: &str, text: &str) -> Arc { + let mut providers: HashMap> = HashMap::new(); + providers.insert( + provider_name.to_string(), + Arc::new(MockProvider::new(provider_name, text)), + ); + Arc::new(Client::new( + providers, + Some(provider_name.to_string()), + vec![], + )) + } - INIT.call_once(|| { - let mut providers: HashMap> = HashMap::new(); - providers.insert( - "mock".to_string(), - Arc::new(MockProvider::new("Narrative from mock.")), - ); - set_default_client(Client::new(providers, Some("mock".to_string()), vec![])); - }); + fn test_llm_source() -> Arc { + 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 { @@ -1057,17 +1109,15 @@ mod tests { #[tokio::test] async fn build_pr_body_uses_in_memory_conclusion() { - install_mock_llm(); - let store = test_store(); let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); - let conclusion = make_test_conclusion(); - let body = build_pr_body( + let body = build_pr_body_with_client( "diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n", "Implement feature", "mock-model", &run_store.clone().into(), - Some(&conclusion), + Some(&make_test_conclusion()), + explicit_client("mock", "Narrative from mock."), ) .await .unwrap(); @@ -1080,8 +1130,6 @@ mod tests { #[tokio::test] async fn build_pr_body_uses_store_records_without_legacy_files() { - install_mock_llm(); - let store = test_store(); let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); @@ -1126,13 +1174,13 @@ mod tests { .await .unwrap(); - let conclusion = make_test_conclusion(); - let body = build_pr_body( + let body = build_pr_body_with_client( "diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n", "Implement feature", "mock-model", &run_store.clone().into(), - Some(&conclusion), + Some(&make_test_conclusion()), + explicit_client("mock", "Narrative from mock."), ) .await .unwrap(); @@ -1145,8 +1193,6 @@ mod tests { #[tokio::test] async fn build_pr_body_uses_plan_text_from_store_without_response_md() { - install_mock_llm(); - let store = test_store(); let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); @@ -1208,12 +1254,13 @@ mod tests { .await .unwrap(); - let body = build_pr_body( + let body = build_pr_body_with_client( "diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n", "Implement feature", "mock-model", &run_store.clone().into(), Some(&make_test_conclusion()), + explicit_client("mock", "Narrative from mock."), ) .await .unwrap(); @@ -1222,6 +1269,78 @@ mod tests { assert!(body.contains("Plan from store")); } + #[tokio::test] + async fn build_pr_body_uses_explicit_llm_client() { + let store = test_store(); + let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); + let body = build_pr_body_with_client( + "diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n", + "Implement feature", + "gpt-5.4", + &run_store.clone().into(), + Some(&make_test_conclusion()), + explicit_client("openai", "Narrative from explicit client."), + ) + .await + .unwrap(); + + assert!(body.contains("Narrative from explicit client.")); + 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 run_store_handle: RunStoreHandle = run_store.into(); + + 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", + &run_store_handle, + llm_source.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] @@ -1341,6 +1460,8 @@ mod tests { async fn empty_diff_returns_none() { let store = test_store(); let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); + let run_store_handle: RunStoreHandle = run_store.into(); + let llm_source = test_llm_source(); let creds = GitHubCredentials::App(fabro_github::GitHubAppCredentials { app_id: "123".to_string(), private_key_pem: "unused".to_string(), @@ -1355,7 +1476,8 @@ mod tests { "claude-sonnet-4-20250514", false, None, - &run_store.clone().into(), + &run_store_handle, + llm_source.as_ref(), None, ) .await; diff --git a/lib/crates/fabro-workflow/src/pipeline/retro.rs b/lib/crates/fabro-workflow/src/pipeline/retro.rs index 78f689486..e9f9420bb 100644 --- a/lib/crates/fabro-workflow/src/pipeline/retro.rs +++ b/lib/crates/fabro-workflow/src/pipeline/retro.rs @@ -1,6 +1,7 @@ use std::sync::Arc; use fabro_agent::SessionEvent; +use fabro_llm::client::Client; use fabro_retro::retro::{Retro, derive_retro}; use fabro_retro::retro_agent::{ RETRO_DATA_DIR, build_retro_prompt, dry_run_narrative, run_retro_agent, @@ -10,38 +11,40 @@ use super::types::{Executed, RetroOptions, Retroed}; use crate::event::Event; pub async fn run_retro(options: &RetroOptions, dry_run: bool) -> Option { - let state = match options.run_store.state().await { + let services = &options.services; + let state = match services.run_store.state().await { Ok(state) => state, Err(e) => { tracing::warn!(error = %e, "Could not load run state, skipping retro"); - if let Some(ref emitter) = options.emitter { - emitter.emit(&Event::RetroFailed { - error: e.to_string(), - duration_ms: 0, - }); - } + services.emitter.emit(&Event::RetroFailed { + error: e.to_string(), + duration_ms: 0, + }); return None; } }; let Some(ref cp) = state.checkpoint else { tracing::warn!("Could not load checkpoint, skipping retro"); - if let Some(ref emitter) = options.emitter { - emitter.emit(&Event::RetroFailed { - error: "checkpoint not found".to_string(), - duration_ms: 0, - }); - } + services.emitter.emit(&Event::RetroFailed { + error: "checkpoint not found".to_string(), + duration_ms: 0, + }); return None; }; let completed_stages = crate::build_completed_stages(cp, options.failed); - let stage_durations = match options.run_store.list_events().await { - Ok(events) => crate::extract_stage_durations_from_events(&events), + let events = match services.run_store.list_events().await { + Ok(events) => events, Err(err) => { - tracing::warn!(error = %err, "Could not load events from store, skipping stage durations"); - std::collections::HashMap::default() + tracing::warn!(error = %err, "Could not load events from store, skipping retro"); + services.emitter.emit(&Event::RetroFailed { + error: err.to_string(), + duration_ms: 0, + }); + return None; } }; + let stage_durations = crate::extract_stage_durations_from_events(&events); let mut retro = derive_retro( options.run_id, &options.workflow_name, @@ -53,81 +56,63 @@ pub async fn run_retro(options: &RetroOptions, dry_run: bool) -> Option { let retro_start = std::time::Instant::now(); let retro_prompt = build_retro_prompt(RETRO_DATA_DIR); - if let Some(ref emitter) = options.emitter { - emitter.emit(&Event::RetroStarted { - prompt: Some(retro_prompt), - provider: Some(options.provider.as_str().to_string()), - model: Some(options.model.clone()), - }); - } + services.emitter.emit(&Event::RetroStarted { + prompt: Some(retro_prompt), + provider: Some(services.provider.as_str().to_string()), + model: Some(options.model.clone()), + }); let retro_result = if dry_run { Ok((dry_run_narrative(), String::new())) - } else if let Some(client) = options.llm_client.as_ref() { - let emitter_clone = options.emitter.clone(); - let event_callback: Option> = - emitter_clone.map(|emitter| -> Arc { - Arc::new(move |event: SessionEvent| { - emitter.touch(); - if !event.event.is_streaming_noise() { - emitter.emit(&Event::Agent { - stage: "retro".to_string(), - visit: 1, - event: event.event.clone(), - session_id: Some(event.session_id.clone()), - parent_session_id: event.parent_session_id.clone(), - }); - } - }) - }); - let events = match options.run_store.list_events().await { - Ok(events) => events, - Err(err) => { - tracing::warn!(error = %err, "Could not load events from store, skipping retro"); - if let Some(ref emitter) = options.emitter { - emitter.emit(&Event::RetroFailed { - error: err.to_string(), - duration_ms: 0, - }); - } - return None; - } - }; - run_retro_agent( - &options.sandbox, - &state, - &events, - &options.run_dir, - client, - options.provider, - &options.model, - event_callback, - ) - .await - .map(|result| (result.narrative, result.response)) } else { - Err(anyhow::anyhow!("No LLM client available")) + match Client::from_source(services.llm_source.as_ref()).await { + Ok(client) => { + let emitter = Arc::clone(&services.emitter); + let event_callback: Arc = + Arc::new(move |event: SessionEvent| { + emitter.touch(); + if !event.event.is_streaming_noise() { + emitter.emit(&Event::Agent { + stage: "retro".to_string(), + visit: 1, + event: event.event.clone(), + session_id: Some(event.session_id.clone()), + parent_session_id: event.parent_session_id.clone(), + }); + } + }); + run_retro_agent( + &services.sandbox, + &state, + &events, + &options.run_dir, + &client, + services.provider, + &options.model, + Some(event_callback), + ) + .await + .map(|result| (result.narrative, result.response)) + } + Err(err) => Err(anyhow::anyhow!(err.to_string())), + } }; let duration_ms = crate::millis_u64(retro_start.elapsed()); match retro_result { Ok((narrative, response)) => { retro.apply_narrative(narrative); - if let Some(ref emitter) = options.emitter { - emitter.emit(&Event::RetroCompleted { - duration_ms, - response: Some(response), - retro: serde_json::to_value(&retro).ok(), - }); - } + services.emitter.emit(&Event::RetroCompleted { + duration_ms, + response: Some(response), + retro: serde_json::to_value(&retro).ok(), + }); } Err(e) => { - if let Some(ref emitter) = options.emitter { - emitter.emit(&Event::RetroFailed { - error: e.to_string(), - duration_ms, - }); - } + services.emitter.emit(&Event::RetroFailed { + error: e.to_string(), + duration_ms, + }); tracing::debug!(error = %e, "Retro agent skipped"); } } @@ -144,15 +129,10 @@ pub async fn retro(executed: Executed, options: &RetroOptions) -> Retroed { graph, outcome, run_options, - run_store, - hook_runner, - emitter, - sandbox, duration_ms, final_context: _, - llm_client: _, + engine, model: _, - provider: _, } = executed; let dry_run = run_options.dry_run_enabled(); @@ -167,11 +147,8 @@ pub async fn retro(executed: Executed, options: &RetroOptions) -> Retroed { graph, outcome, run_options, - run_store, - hook_runner, - emitter, - sandbox, duration_ms, + services: Arc::clone(&engine.run), retro, } } @@ -182,6 +159,7 @@ mod tests { use std::sync::{Arc, Mutex}; use std::time::Duration; + use fabro_auth::{CredentialSource, EnvCredentialSource}; use fabro_graphviz::graph::Graph; use fabro_store::Database; use fabro_types::{RunId, WorkflowSettings, fixtures}; @@ -193,6 +171,7 @@ mod tests { use crate::pipeline::types::Executed; use crate::records::{Checkpoint, CheckpointExt, RunSpec}; use crate::run_options::RunOptions; + use crate::services::{EngineServices, RunServices}; fn test_run_id() -> RunId { fixtures::RUN_1 @@ -310,6 +289,10 @@ mod tests { } } + fn test_llm_source() -> Arc { + Arc::new(EnvCredentialSource::new()) + } + #[tokio::test] async fn retro_phase_persists_retro_in_projection() { let temp = tempfile::tempdir().unwrap(); @@ -324,34 +307,36 @@ mod tests { let sandbox: Arc = Arc::new(fabro_agent::LocalSandbox::new( std::env::current_dir().unwrap(), )); + let services = RunServices::new( + run_store.clone().into(), + Arc::clone(&emitter), + Arc::clone(&sandbox), + None, + None, + fabro_llm::Provider::Anthropic, + test_llm_source(), + ); + let mut engine = EngineServices::test_default(); + engine.run = Arc::clone(&services); let executed = Executed { graph: Graph::new("test"), outcome: Ok(crate::outcome::Outcome::success()), run_options: test_run_options(&run_dir), - run_store: run_store.clone().into(), - hook_runner: None, - emitter: Arc::clone(&emitter), - sandbox: Arc::clone(&sandbox), duration_ms: 1, final_context: Context::new(), - llm_client: None, + engine: Arc::new(engine), model: "test-model".to_string(), - provider: fabro_llm::Provider::Anthropic, }; let retroed = retro(executed, &RetroOptions { run_id: test_run_id(), - run_store: run_store.into(), + services, workflow_name: "test".to_string(), goal: "Ship it".to_string(), run_dir: run_dir.clone(), - sandbox, - emitter: Some(emitter), failed: false, run_duration_ms: 1, enabled: true, - llm_client: None, - provider: fabro_llm::Provider::Anthropic, model: "test-model".to_string(), }) .await; @@ -373,24 +358,29 @@ mod tests { let seen = Arc::clone(&seen); move |event| seen.lock().unwrap().push(event.clone()) }); + let services = RunServices::new( + test_run_store(&run_dir, &checkpoint).await.into(), + Arc::clone(&emitter), + Arc::new(fabro_agent::LocalSandbox::new( + std::env::current_dir().unwrap(), + )), + None, + None, + fabro_llm::Provider::Anthropic, + test_llm_source(), + ); let retro = run_retro( &RetroOptions { - run_id: test_run_id(), - run_store: test_run_store(&run_dir, &checkpoint).await.into(), - workflow_name: "test".to_string(), - goal: "Ship it".to_string(), - run_dir: run_dir.clone(), - sandbox: Arc::new(fabro_agent::LocalSandbox::new( - std::env::current_dir().unwrap(), - )), - emitter: Some(Arc::clone(&emitter)), - failed: false, + run_id: test_run_id(), + services, + workflow_name: "test".to_string(), + goal: "Ship it".to_string(), + run_dir: run_dir.clone(), + failed: false, run_duration_ms: 1, - enabled: true, - llm_client: None, - provider: fabro_llm::Provider::Anthropic, - model: "test-model".to_string(), + enabled: true, + model: "test-model".to_string(), }, true, ) diff --git a/lib/crates/fabro-workflow/src/pipeline/types.rs b/lib/crates/fabro-workflow/src/pipeline/types.rs index 3f78fb826..defa987f9 100644 --- a/lib/crates/fabro-workflow/src/pipeline/types.rs +++ b/lib/crates/fabro-workflow/src/pipeline/types.rs @@ -2,12 +2,9 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; use std::sync::Arc; -use fabro_agent::Sandbox; use fabro_graphviz::graph::Graph; -use fabro_hooks::HookRunner; use fabro_interview::Interviewer; use fabro_llm::Provider; -use fabro_llm::client::Client; use fabro_mcp::config::McpServerSettings; use fabro_model::FallbackTarget; use fabro_retro::retro::Retro; @@ -30,6 +27,7 @@ use crate::records::{Checkpoint, Conclusion, RunSpec}; use crate::run_control::RunControlState; use crate::run_options::{GitCheckpointOptions, LifecycleOptions, RunOptions}; use crate::runtime_store::RunStoreHandle; +use crate::services::{EngineServices, RunServices}; use crate::transforms::Transform; use crate::workflow_bundle::WorkflowBundle; @@ -263,25 +261,14 @@ pub struct InitOptions { pub struct Initialized { pub graph: Graph, pub source: String, - pub inputs: HashMap, pub run_options: RunOptions, - pub workflow_path: Option, - pub workflow_bundle: Option>, - pub run_store: RunStoreHandle, pub(crate) checkpoint: Option, pub(crate) seed_context: Option, - pub emitter: Arc, - pub sandbox: Arc, - pub registry: Arc, pub on_node: crate::OnNodeCallback, pub artifact_sink: Option, pub run_control: Option>, - pub hook_runner: Option>, - pub env: HashMap, - pub dry_run: bool, - pub llm_client: Option, + pub engine: Arc, pub model: String, - pub provider: Provider, } /// Output of the EXECUTE phase. @@ -290,15 +277,10 @@ pub struct Executed { pub graph: Graph, pub outcome: Result, pub run_options: RunOptions, - pub run_store: RunStoreHandle, - pub hook_runner: Option>, - pub emitter: Arc, - pub sandbox: Arc, pub duration_ms: u64, pub final_context: Context, - pub llm_client: Option, + pub engine: Arc, pub model: String, - pub provider: Provider, } /// Output of the RETRO phase. @@ -307,24 +289,19 @@ pub struct Retroed { pub graph: Graph, pub outcome: Result, pub run_options: RunOptions, - pub run_store: RunStoreHandle, - pub hook_runner: Option>, - pub emitter: Arc, - pub sandbox: Arc, pub duration_ms: u64, + pub services: Arc, pub retro: Option, } /// Output of the FINALIZE phase. #[non_exhaustive] pub struct Concluded { - pub run_id: RunId, - pub outcome: Result, - pub conclusion: Conclusion, - pub pushed_branch: Option, - pub graph: Graph, - pub run_options: RunOptions, - pub emitter: Arc, + pub outcome: Result, + pub conclusion: Conclusion, + pub graph: Graph, + pub run_options: RunOptions, + pub services: Arc, } /// Output of the PULL_REQUEST phase. @@ -348,17 +325,13 @@ pub struct TransformOptions { /// Options for the RETRO phase. pub struct RetroOptions { pub run_id: RunId, - pub run_store: RunStoreHandle, + pub services: Arc, pub workflow_name: String, pub goal: String, pub run_dir: PathBuf, - pub sandbox: Arc, - pub emitter: Option>, pub failed: bool, pub run_duration_ms: u64, pub enabled: bool, - pub llm_client: Option, - pub provider: Provider, pub model: String, } @@ -366,17 +339,13 @@ pub struct RetroOptions { pub struct FinalizeOptions { pub run_dir: PathBuf, pub run_id: RunId, - pub run_store: RunStoreHandle, pub workflow_name: String, - pub hook_runner: Option>, pub preserve_sandbox: bool, pub last_git_sha: Option, } /// Options for the PULL_REQUEST phase. pub struct PullRequestOptions { - pub run_dir: PathBuf, - pub run_store: RunStoreHandle, pub pr_config: Option, pub github_app: Option, pub origin_url: Option, diff --git a/lib/crates/fabro-workflow/src/run_options.rs b/lib/crates/fabro-workflow/src/run_options.rs index 5ee57fb05..bdab44985 100644 --- a/lib/crates/fabro-workflow/src/run_options.rs +++ b/lib/crates/fabro-workflow/src/run_options.rs @@ -57,6 +57,11 @@ impl RunOptions { pub fn artifact_globs(&self) -> Vec { self.settings.run.artifacts.include.clone() } + + /// Run branch name from git checkpoint options, if set. + pub fn run_branch(&self) -> Option<&str> { + self.git.as_ref().and_then(|g| g.run_branch.as_deref()) + } } /// Options for sandbox lifecycle management within the engine. diff --git a/lib/crates/fabro-workflow/src/services.rs b/lib/crates/fabro-workflow/src/services.rs new file mode 100644 index 000000000..90c968935 --- /dev/null +++ b/lib/crates/fabro-workflow/src/services.rs @@ -0,0 +1,259 @@ +use std::collections::HashMap; +use std::path::PathBuf; +use std::sync::Arc; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::time::Duration; + +use fabro_agent::Sandbox; +use fabro_auth::CredentialSource; +#[cfg(test)] +use fabro_auth::ResolvedCredentials; +use fabro_hooks::{HookContext, HookDecision, HookRunner}; +use fabro_model::Provider; +use tokio::time; +use tokio_util::sync::CancellationToken; + +use crate::event::Emitter; +use crate::handler::HandlerRegistry; +use crate::runtime_store::RunStoreHandle; +use crate::sandbox_git::GitState; +use crate::workflow_bundle::WorkflowBundle; + +/// 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, +} + +impl RunServices { + #[must_use] + pub fn new( + run_store: RunStoreHandle, + emitter: Arc, + sandbox: Arc, + hook_runner: Option>, + cancel_requested: Option>, + provider: Provider, + llm_source: Arc, + ) -> Arc { + Arc::new(Self { + run_store, + emitter, + sandbox, + hook_runner, + cancel_requested, + provider, + llm_source, + }) + } + + /// Bridge the core executor's atomic cancel flag to sandbox command + /// cancellation. + pub fn sandbox_cancel_token(&self) -> Option { + sandbox_cancel_token(self.cancel_requested.clone()) + } + + /// Run lifecycle hooks and return the merged decision. + /// Returns `Proceed` if no hook runner is configured. + pub async fn run_hooks(&self, hook_context: &HookContext) -> HookDecision { + let Some(ref runner) = self.hook_runner else { + return HookDecision::Proceed; + }; + runner + .run(hook_context, Arc::clone(&self.sandbox), None) + .await + } + + #[must_use] + pub fn with_run_store(self: &Arc, run_store: RunStoreHandle) -> Arc { + Arc::new(Self { + run_store, + ..self.as_ref().clone() + }) + } + + #[must_use] + pub fn with_emitter(self: &Arc, emitter: Arc) -> Arc { + Arc::new(Self { + emitter, + ..self.as_ref().clone() + }) + } + + #[must_use] + pub fn with_sandbox(self: &Arc, sandbox: Arc) -> Arc { + Arc::new(Self { + sandbox, + ..self.as_ref().clone() + }) + } + + #[must_use] + pub fn with_cancel_requested( + self: &Arc, + cancel_requested: Option>, + ) -> Arc { + Arc::new(Self { + cancel_requested, + ..self.as_ref().clone() + }) + } +} + +/// Services available only while executing workflow nodes. +pub struct EngineServices { + 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, + /// Typed values from `[run.inputs]`, available to prompt templates. + pub inputs: HashMap, + /// When true, handlers should skip real execution and return simulated + /// results. + pub dry_run: bool, + /// Logical path of the current workflow when running from a bundle. + pub workflow_path: Option, + /// Bundled workflows available for child-workflow resolution. + pub workflow_bundle: Option>, +} + +impl EngineServices { + /// Read the current git state (if any). + pub fn git_state(&self) -> Option> { + self.git_state.read().unwrap().clone() + } + + /// Set the git state for the current run. + pub fn set_git_state(&self, state: Option>) { + *self.git_state.write().unwrap() = state; + } + + /// Test-only default: empty registry and cross-phase services. + #[cfg(test)] + #[expect( + clippy::disallowed_methods, + reason = "Test scaffolding must build a slate-backed run store from sync code." + )] + pub fn test_default() -> Self { + use fabro_store::Database; + use object_store::memory::InMemory; + + use crate::handler::start; + + #[derive(Debug, Default)] + struct StubCredentialSource; + + #[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() + } + } + + let store = Arc::new(Database::new( + Arc::new(InMemory::new()), + "", + Duration::from_millis(1), + None, + )); + let run_store = std::thread::spawn(move || { + tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("test runtime should initialize") + .block_on(async { + store + .create_run(&fabro_types::RunId::new()) + .await + .expect("slate-backed test run store should initialize") + }) + }) + .join() + .expect("test run store thread should join"); + + Self { + run: RunServices::new( + run_store.into(), + Arc::new(Emitter::default()), + Arc::new(fabro_agent::LocalSandbox::new( + std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), + )), + None, + None, + Provider::Anthropic, + Arc::new(StubCredentialSource), + ), + registry: Arc::new(HandlerRegistry::new(Box::new(start::StartHandler))), + git_state: std::sync::RwLock::new(None), + env: HashMap::new(), + inputs: HashMap::new(), + dry_run: false, + workflow_path: None, + workflow_bundle: None, + } + } +} + +pub(crate) fn sandbox_cancel_token( + cancel_requested: Option>, +) -> Option { + let cancel_requested = cancel_requested?; + let token = CancellationToken::new(); + + if cancel_requested.load(Ordering::Relaxed) { + token.cancel(); + return Some(token); + } + + let token_clone = token.clone(); + tokio::spawn(async move { + loop { + if token_clone.is_cancelled() { + return; + } + if cancel_requested.load(Ordering::Relaxed) { + token_clone.cancel(); + return; + } + time::sleep(Duration::from_millis(10)).await; + } + }); + + Some(token) +} + +#[cfg(test)] +mod tests { + use super::EngineServices; + + #[tokio::test] + async fn test_default_uses_stub_credential_source() { + let services = EngineServices::test_default(); + + assert!( + services + .run + .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 62b28808a..57271354d 100644 --- a/lib/crates/fabro-workflow/src/test_support.rs +++ b/lib/crates/fabro-workflow/src/test_support.rs @@ -5,6 +5,7 @@ use std::sync::Arc; use std::time::Duration; use fabro_agent::Sandbox; +use fabro_auth::{CredentialSource, EnvCredentialSource}; use fabro_graphviz::graph::Graph as GvGraph; use fabro_store::{ArtifactStore, Database, RunProjection}; use object_store::local::LocalFileSystem; @@ -19,6 +20,7 @@ use crate::pipeline::types::{Executed, Initialized}; use crate::pipeline::{billing_from_checkpoint, build_terminal_event}; use crate::records::Checkpoint; use crate::run_options::RunOptions; +use crate::services::{EngineServices, RunServices}; /// These helpers stop at EXECUTE, so they emit the terminal event here to /// keep test consumers seeing the same end-of-run signal as production @@ -31,7 +33,7 @@ use crate::run_options::RunOptions; async fn execute_and_emit_terminal(initialized: InitializedState) -> Executed { let executed = Box::pin(pipeline::execute(initialized.initialized)).await; initialized.store_logger.flush().await; - let state = executed.run_store.state().await.ok(); + let state = executed.engine.run.run_store.state().await.ok(); let billing = state .as_ref() .and_then(|s| s.checkpoint.as_ref()) @@ -44,7 +46,7 @@ async fn execute_and_emit_terminal(initialized: InitializedState) -> Executed { None, billing, ); - executed.emitter.emit(&event); + executed.engine.run.emitter.emit(&event); initialized.store_logger.flush().await; executed } @@ -62,6 +64,7 @@ struct InitializedOptions { hook_runner: Option>, env: HashMap, checkpoint: Option, + llm_source: Option>, } struct InitializedState { @@ -143,27 +146,35 @@ async fn initialized( ); InitializedState { initialized: Initialized { - graph: graph.clone(), - source: String::new(), - inputs: run_options.settings.run.inputs.clone(), - run_options: run_options.clone(), - workflow_path: None, - workflow_bundle: None, - run_store: run_store.into(), - checkpoint: options.checkpoint, - seed_context: None, - emitter, - sandbox, - registry: Arc::new(registry), - 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, - hook_runner: options.hook_runner, - env: options.env, - dry_run: run_options.dry_run_enabled(), - llm_client: None, - model: String::new(), - provider: fabro_llm::Provider::Anthropic, + 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, + 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.settings.run.inputs.clone(), + dry_run: run_options.dry_run_enabled(), + workflow_path: None, + workflow_bundle: None, + }), + model: String::new(), }, store_logger, } @@ -186,6 +197,7 @@ pub async fn run_graph( hook_runner: None, env: HashMap::new(), checkpoint: None, + llm_source: None, }, ) .await; @@ -210,12 +222,15 @@ pub async fn run_graph_with_state( hook_runner: None, env: HashMap::new(), checkpoint: None, + llm_source: None, }, ) .await; let executed = execute_and_emit_terminal(initialized).await; let outcome = executed.outcome?; let state = executed + .engine + .run .run_store .state() .await @@ -242,6 +257,7 @@ pub async fn run_graph_with_hooks( hook_runner: Some(hook_runner), env: env.unwrap_or_default(), checkpoint: None, + llm_source: None, }, ) .await; @@ -268,12 +284,15 @@ 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; let executed = execute_and_emit_terminal(initialized).await; let outcome = executed.outcome?; let state = executed + .engine + .run .run_store .state() .await @@ -299,6 +318,7 @@ pub async fn run_graph_from_checkpoint( hook_runner: None, env: HashMap::new(), checkpoint: Some(checkpoint.clone()), + llm_source: None, }, ) .await; @@ -324,12 +344,50 @@ 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 = execute_and_emit_terminal(initialized).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; + 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 @@ -395,6 +453,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/daytona_integration.rs b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs index 66a4638e0..04a4dd38c 100644 --- a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs +++ b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs @@ -571,6 +571,7 @@ impl Handler for FileWriterHandler { let content = format!("output from {}", node.id); let cmd = format!("echo '{content}' > {}.txt", node.id); let _ = services + .run .sandbox .exec_command(&cmd, 10_000, None, None, None) .await; @@ -1288,6 +1289,7 @@ impl Handler for AssetCreatorHandler { "echo 'test output' > test-results/output.txt" ); services + .run .sandbox .exec_command(script, 30_000, None, None, None) .await diff --git a/lib/crates/fabro-workflow/tests/it/integration.rs b/lib/crates/fabro-workflow/tests/it/integration.rs index d21ae0a95..39acd777f 100644 --- a/lib/crates/fabro-workflow/tests/it/integration.rs +++ b/lib/crates/fabro-workflow/tests/it/integration.rs @@ -6141,11 +6141,10 @@ mod real_llm { } fabro_test::require_env("ANTHROPIC_API_KEY")?; - Some(Arc::new( - Client::from_env() - .await - .expect("unified-llm client should initialize from env"), - )) + let source = fabro_auth::EnvCredentialSource::new(); + Some(Arc::new(Client::from_source(&source).await.expect( + "unified-llm client should initialize from env source", + ))) } fn make_llm_backend(client: Arc) -> Box { @@ -6602,10 +6601,165 @@ 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: WorkflowSettings::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 run_store_handle: fabro_workflow::runtime_store::RunStoreHandle = run_store.into(); + + 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", + &run_store_handle, + llm_source.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] @@ -7376,9 +7530,10 @@ fn subgraph_without_label_no_class_derived() { // --------------------------------------------------------------------------- fn hook_runner_from_defs(hooks: Vec) -> Arc { - Arc::new(fabro_hooks::HookRunner::new(fabro_hooks::HookSettings { - hooks, - })) + Arc::new(fabro_hooks::HookRunner::new( + fabro_hooks::HookSettings { hooks }, + Arc::new(fabro_auth::EnvCredentialSource::new()), + )) } struct HookTestRunner { @@ -10175,9 +10330,10 @@ impl Handler for FileWriterHandler { _run_dir: &Path, services: &fabro_workflow::handler::EngineServices, ) -> Result { - let work_dir = services.sandbox.working_directory().to_string(); + let work_dir = services.run.sandbox.working_directory().to_string(); let file_path = format!("{}/{}.txt", work_dir, node.id); services + .run .sandbox .write_file(&file_path, &format!("written by {}", node.id)) .await @@ -12254,7 +12410,7 @@ impl Handler for KeepaliveHandler { let start = std::time::Instant::now(); while start.elapsed() < std::time::Duration::from_millis(self.total_ms) { tokio::time::sleep(std::time::Duration::from_millis(self.interval_ms)).await; - services.emitter.emit(&Event::Prompt { + services.run.emitter.emit(&Event::Prompt { stage: node.id.clone(), visit: 1, text: "keepalive".to_string(), @@ -12530,6 +12686,7 @@ impl Handler for AssetCreatorHandler { "echo 'test output' > test-results/output.txt" ); services + .run .sandbox .exec_command(script, 30_000, None, None, None) .await