diff --git a/CHANGELOG.md b/CHANGELOG.md index 9efd2cb..ec9774e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,7 +60,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/2.0.0. single parameter for `ApiClient` methods. Replaces the positional `(Vec, Option, Option>)` parameter lists on `stream_messages`, `create_message`, and their `_with_options` variants. - Builders: `new`, `system`, `system_opt`, `tools`, `tools_opt`. + Builders: `new`, `with_system`, `with_tools` (both take `Option`). - `GeminiClientBuilder::include_thoughts(bool)` builder: opt into Gemini's `thinkingConfig.includeThoughts` for reasoning-capable models (2.5 Pro/Flash, Gemini 3). Defaults to `false` — the Gemini API rejects `thinkingConfig` @@ -169,9 +169,54 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/2.0.0. Path-aware invalidation via the caller-supplied `PathExtractor` trait; TTL-based expiry per `ttl_turns`. Default-off; only successful results are cached. +- Fluent `with_*()` builder methods on `LoopConfig` (one per public field, + e.g. `with_model`, `with_max_turns`, `with_session_id`, + `with_parallel_tool_dispatch`). Each is `#[must_use]`, consuming, and + returns `Self` for chaining. Purely additive — `Default`-based and direct + struct-literal construction are unchanged and remain first-class. +- Consuming `with_*()` fluent builders on `BareLoop`, mirroring the existing + `&mut self` setters so a loop can be assembled as a chain off `BareLoop::new`: + `with_reflector`, `with_recovery_strategy`, `with_context_manager`, + `with_stream_handler`, `with_hook_executor` (`hooks` feature), + `with_health_registry` (`tool_health` feature), `with_pipeline` (returns + `Result` — chain with `?`), `with_observer`, + `with_text_streamer`, `with_contributor`, `with_request_options`. The + original `set_*`/`register_*` setters are unchanged and remain available. ### Changed +- **Breaking (renames):** consuming builder methods that return `Self` are now + uniformly prefixed `with_`, matching the crate-wide convention. The old + no-prefix names are removed. Affected types and methods (old → new): + `SessionResultBuilder` (`session_id`/`total_turns`/`input_tokens`/ + `output_tokens`/`total_duration`/`tool_calls`/`success`/`final_output`/ + `error` → `with_*`); `ModelSwitch` (`context_window`, `max_tokens` → + `with_*`); `StreamRequest` (`system`, `system_opt`, `tools`, `tools_opt` → + `with_*`); `RequestOptions` (`response_format`, `tool_constraint` → `with_*`); + `UnixShieldBuilder` (`warn_threshold`, `block_threshold`, `pattern`, + `combination_rule` → `with_*`); `AutoCommitConfigBuilder` (`enabled`, + `message_template`, `auto_push`, `files` → `with_*`); `ToolPipelineBuilder` + (`core` → `with_core`; the middleware accumulators `with` and `with_arc` are + renamed to `with_middleware` and `with_middleware_arc` so they no longer read + ambiguously next to `with_core`). Migration: add the `with_` prefix at each + call site. +- `ToolPipelineBuilder::build` now distinguishes its two failure cases: + `PipelineError::Empty` when neither middleware nor a core was added (the + builder is untouched), and `PipelineError::MissingCore` when at least one + middleware was added but no core registry was set. Previously both cases + returned `MissingCore`, and `Empty` was unreachable. The `Empty` and + `MissingCore` variants now carry multiline rustdoc explaining each condition. +- **Breaking (optional-field builders unified):** builder methods that set an + `Option` field now uniformly take `Option` and drop the `_opt` suffix — + the parameter type already conveys "optional," so the name should not. + `StreamRequest` loses `with_system_opt`/`with_tools_opt`; `with_system` and + `with_tools` now take `Option`/`Option>` (pass + `Some(…)` to set, `None` to clear). `LoopConfig::with_system_prompt` changes + from `impl Into` to `Option`, so it can now clear the + override with `None` (previously it could only set). `AttemptRecord::with_reason` + and `Operation::with_result_hash` already followed this shape and are + unchanged. Migration: wrap existing literal/value arguments in `Some(…)` (or + rename `with_*_opt` → `with_*`). - **Breaking:** `stream::DeltaPart` is now `#[non_exhaustive]`. Every exhaustive `match` on `DeltaPart` in downstream code must add a `_ =>` arm (or a `Thinking =>` arm for the new variant). Same-crate matches are diff --git a/src/api.rs b/src/api.rs index 34dd566..8b57578 100644 --- a/src/api.rs +++ b/src/api.rs @@ -21,8 +21,9 @@ use std::sync::Arc; /// so method signatures stay short (`&self, request, ...`) instead of /// repeating `messages, system, tools` at every call site. /// -/// Construct via `StreamRequest::new(messages)` and chain `.system(...)` -/// / `.tools(...)` as needed, or build with struct-literal syntax. +/// Construct via `StreamRequest::new(messages)` and chain +/// `.with_system(...)` / `.with_tools(...)` as needed, or build with +/// struct-literal syntax. /// /// # Example /// @@ -30,19 +31,38 @@ use std::sync::Arc; /// use loopctl::api::StreamRequest; /// /// let req = StreamRequest::new(vec![Message::user("hi")]) -/// .system("be brief") -/// .tools(vec![search_tool]); +/// .with_system(Some("be brief".to_string())) +/// .with_tools(Some(vec![search_tool])); /// let stream = client.stream_messages(req); /// ``` #[derive(Debug, Clone)] pub struct StreamRequest { - /// The conversation history. Owned because the request body must be built - /// from owned data — callers clone the full history each turn - /// (O(n) in messages). + /// The conversation history sent to the model. + /// + /// Owned because the request body is built from owned data — callers clone + /// the full history each turn (O(n) in messages). This is the only required + /// field; an empty `Vec` is permitted (a cold-start prompt with no prior + /// turns). pub messages: Vec, - /// An optional system prompt to prepend. + + /// An optional system prompt prepended above `messages`. + /// + /// When `Some`, providers emit it in their native system-prompt slot + /// (top-level `system` on Anthropic/Gemini, a leading `system` role message + /// on OpenAI). When `None`, the provider receives no system prompt for the + /// turn. The value is forwarded verbatim from + /// [`LoopConfig::system_prompt`](crate::config::LoopConfig::system_prompt); + /// set it there to drive this field. pub system: Option, - /// Optional tool definitions the model may invoke. + + /// Optional tool definitions the model may invoke this turn. + /// + /// When `Some`, providers advertise these schemas to the model (OpenAI + /// `tools`, Anthropic `tools`, Gemini `functionDeclarations`). When `None`, + /// no tools are advertised and the model cannot issue tool calls. Note the + /// framework silently suppresses `tools` when a + /// [`response_format`](crate::structured::RequestOptions) constraint is set + /// — see the structured-output docs. pub tools: Option>, } @@ -58,35 +78,24 @@ impl StreamRequest { } /// Set the system prompt. - #[must_use] - pub fn system(mut self, system: impl Into) -> Self { - self.system = Some(system.into()); - self - } - - /// Set the system prompt from an existing [`Option`]. /// - /// Convenience for call sites that already hold the prompt as an `Option` - /// (e.g. forwarded from another field). `None` leaves it unset. + /// The field is optional: pass `Some(prompt)` to set it or `None` to leave + /// it unset (the default). This mirrors the field type directly, so a + /// caller that already holds an `Option` (e.g. forwarded from + /// another config field) needs no conversion. #[must_use] - pub fn system_opt(mut self, system: Option) -> Self { + pub fn with_system(mut self, system: Option) -> Self { self.system = system; self } /// Set the tool definitions. - #[must_use] - pub fn tools(mut self, tools: Vec) -> Self { - self.tools = Some(tools); - self - } - - /// Set the tool definitions from an existing [`Option>`]. /// - /// Convenience for call sites that already hold tools as an `Option`. - /// `None` leaves them unset. + /// The field is optional: pass `Some(tools)` to attach tool schemas or + /// `None` to send no tools (the default). Mirrors the field type so an + /// existing `Option>` maps without conversion. #[must_use] - pub fn tools_opt(mut self, tools: Option>) -> Self { + pub fn with_tools(mut self, tools: Option>) -> Self { self.tools = tools; self } @@ -462,15 +471,9 @@ mod tests { #[test] fn stream_request_system_builder() { - let req = StreamRequest::new(vec![]).system("be brief"); + let req = StreamRequest::new(vec![]).with_system(Some("be brief".to_string())); assert_eq!(req.system.as_deref(), Some("be brief")); - } - - #[test] - fn stream_request_system_opt_builder() { - let req = StreamRequest::new(vec![]).system_opt(Some("x".into())); - assert_eq!(req.system.as_deref(), Some("x")); - let req = StreamRequest::new(vec![]).system_opt(None); + let req = StreamRequest::new(vec![]).with_system(None); assert!(req.system.is_none()); } @@ -481,20 +484,9 @@ mod tests { description: "Search".into(), input_schema: serde_json::json!({"type": "object"}), }]; - let req = StreamRequest::new(vec![]).tools(tools); - assert_eq!(req.tools.as_ref().unwrap().len(), 1); - } - - #[test] - fn stream_request_tools_opt_builder() { - let tools = vec![crate::tool::ToolSchema { - tool: "search".into(), - description: "Search".into(), - input_schema: serde_json::json!({"type": "object"}), - }]; - let req = StreamRequest::new(vec![]).tools_opt(Some(tools)); + let req = StreamRequest::new(vec![]).with_tools(Some(tools)); assert_eq!(req.tools.as_ref().unwrap().len(), 1); - let req = StreamRequest::new(vec![]).tools_opt(None); + let req = StreamRequest::new(vec![]).with_tools(None); assert!(req.tools.is_none()); } diff --git a/src/config.rs b/src/config.rs index 6908193..00ab6e7 100644 --- a/src/config.rs +++ b/src/config.rs @@ -16,9 +16,9 @@ use uuid::Uuid; /// /// # Construction /// -/// Use [`LoopConfig::default`] for sensible defaults or override individual -/// fields through the builder via -/// `LoopBuilder::with_config`. +/// Use [`LoopConfig::default`] for sensible defaults, then override individual +/// fields either with the fluent `with_*()` builders or direct struct-literal +/// assignment — both are supported. /// /// ``` /// use loopctl::config::LoopConfig; @@ -49,10 +49,10 @@ pub struct LoopConfig { /// Optional system prompt override. /// - /// When `None`, the agent core assembles its own system prompt from the - /// registered tools' `system_prompt()` contributions. When `Some(text)`, - /// that text replaces the default entirely. Set this to inject a custom - /// persona or instruction set. + /// When `Some`, this text is sent to the provider as the turn's system + /// prompt. When `None` (the default), the provider receives no system + /// prompt for the turn. Set this to inject a custom persona or instruction + /// set. pub system_prompt: Option, /// Maximum number of turns before forcing completion. @@ -87,6 +87,7 @@ pub struct LoopConfig { /// turn to avoid an overflow. Defaults to `0.80`. Must be finite and in /// `[0.0, 1.0]` ([`validate`](LoopConfig::validate)). pub compact_threshold: f64, + /// Whether auto-compaction is enabled. /// /// When `true` (the default), the compactor runs automatically when @@ -149,35 +150,56 @@ impl Default for LoopConfig { } impl LoopConfig { - /// Set the session ID. + /// Set the session identifier, overriding the random UUID v4 that + /// [`Default`](LoopConfig::default) generates. + /// + /// Use this to resume a prior session under its existing ID, or to pin a + /// deterministic ID in tests. #[must_use] pub fn with_session_id(mut self, session_id: Uuid) -> Self { self.session_id = session_id; self } - /// Set the model identifier. + /// Set the model identifier passed to the API client on each request. + /// + /// Accepts anything that converts into a `String` (for example `"gpt-4o"`), + /// so a `&str` literal works without an explicit `.to_string()`. The value + /// must be non-empty; see [`validate`](LoopConfig::validate). #[must_use] pub fn with_model(mut self, model: impl Into) -> Self { self.model = model.into(); self } - /// Set the system prompt. + /// Set the optional system-prompt override. + /// + /// When `Some`, this text is sent to the provider as the turn's system + /// prompt (verbatim). When `None` (the default), the provider receives no + /// system prompt for the turn. Pass `Some(prompt)` to inject a custom + /// persona or instructions, or `None` to revert an earlier override. #[must_use] - pub fn with_system_prompt(mut self, system_prompt: impl Into) -> Self { - self.system_prompt = Some(system_prompt.into()); + pub fn with_system_prompt(mut self, system_prompt: Option) -> Self { + self.system_prompt = system_prompt; self } - /// Set the maximum number of turns. + /// Set the maximum number of turns before the loop forces completion. + /// + /// A safety cap: the loop halts with + /// [`LoopError::MaxTurnsExceeded`](crate::error::LoopError::MaxTurnsExceeded) + /// once it is reached. Must be at least `1`; see + /// [`validate`](LoopConfig::validate). #[must_use] pub fn with_max_turns(mut self, max_turns: usize) -> Self { self.max_turns = max_turns; self } - /// Set the maximum tokens per API response. + /// Set the maximum tokens for each API response, sent to the provider as + /// `max_tokens` on every request. + /// + /// Must be at least `1`; see [`validate`](LoopConfig::validate). #[must_use] pub fn with_max_tokens(mut self, max_tokens: u32) -> Self { self.max_tokens = max_tokens; @@ -185,27 +207,47 @@ impl LoopConfig { } /// Set the context window size in tokens. + /// + /// Should match the actual window of the configured + /// [`model`](LoopConfig::model); the compaction subsystem uses it together + /// with [`compact_threshold`](LoopConfig::compact_threshold) to decide when + /// to run. Must be at least `1`; see [`validate`](LoopConfig::validate). #[must_use] pub fn with_context_window(mut self, context_window: u64) -> Self { self.context_window = context_window; self } - /// Set the auto-compaction threshold (0.0–1.0). + /// Set the auto-compaction trigger threshold as a fraction of the context + /// window (0.0–1.0). + /// + /// When estimated token usage exceeds `compact_threshold * context_window`, + /// compaction runs before the next turn. This builder stores the value + /// verbatim — it does **not** clamp; the range is enforced by + /// [`validate`](LoopConfig::validate), which is the single source of truth + /// for the `[0.0, 1.0]` bound. #[must_use] pub fn with_compact_threshold(mut self, compact_threshold: f64) -> Self { self.compact_threshold = compact_threshold; self } - /// Enable or disable auto-compaction. + /// Enable or disable automatic compaction. + /// + /// When `true` (the default), compaction runs automatically once + /// [`compact_threshold`](LoopConfig::compact_threshold) is reached. When + /// `false`, the host application must manage context size itself. #[must_use] pub fn with_auto_compact(mut self, auto_compact: bool) -> Self { self.auto_compact = auto_compact; self } - /// Set the parallel tool dispatch policy. + /// Set how independent tool calls within a single turn are dispatched. + /// + /// Defaults to sequential. See [`ParallelDispatchConfig`] and + /// [`ParallelMode`] for the observer and loop-detection ordering + /// implications of enabling parallel dispatch. #[must_use] pub fn with_parallel_tool_dispatch(mut self, config: ParallelDispatchConfig) -> Self { self.parallel_tool_dispatch = config; @@ -423,4 +465,95 @@ mod tests { let msg = err.to_string(); assert!(msg.contains("model"), "error should mention model: {msg}"); } + + #[test] + fn with_model_sets_field_and_accepts_str() { + let config = LoopConfig::default().with_model("gpt-4o"); + assert_eq!(config.model, "gpt-4o"); + } + + #[test] + fn with_system_prompt_sets_some() { + let config = LoopConfig::default().with_system_prompt(Some("p".to_string())); + assert_eq!(config.system_prompt, Some("p".to_string())); + } + + #[test] + fn with_system_prompt_none_clears_override() { + let config = LoopConfig::default().with_system_prompt(None); + assert!(config.system_prompt.is_none()); + } + + #[test] + fn with_max_turns_sets_field() { + let config = LoopConfig::default().with_max_turns(7); + assert_eq!(config.max_turns, 7); + } + + #[test] + fn with_max_tokens_sets_field() { + let config = LoopConfig::default().with_max_tokens(123); + assert_eq!(config.max_tokens, 123); + } + + #[test] + fn with_context_window_sets_field() { + let config = LoopConfig::default().with_context_window(8192); + assert_eq!(config.context_window, 8192); + } + + #[test] + fn with_compact_threshold_stores_value_without_clamping() { + let config = LoopConfig::default().with_compact_threshold(1.5); + assert!((config.compact_threshold - 1.5).abs() < f64::EPSILON); + } + + #[test] + fn with_auto_compact_flips_default() { + let config = LoopConfig::default().with_auto_compact(false); + assert!(!config.auto_compact); + } + + #[test] + fn with_session_id_round_trips() { + let id = Uuid::new_v4(); + let config = LoopConfig::default().with_session_id(id); + assert_eq!(config.session_id, id); + } + + #[test] + fn with_parallel_tool_dispatch_round_trips() { + let dispatch = ParallelDispatchConfig { + mode: ParallelMode::Parallel, + max_concurrency: 4, + }; + let config = LoopConfig::default().with_parallel_tool_dispatch(dispatch); + assert_eq!(config.parallel_tool_dispatch.mode, ParallelMode::Parallel); + assert_eq!(config.parallel_tool_dispatch.max_concurrency, 4); + } + + #[test] + fn builder_chain_composes_without_clobbering() { + let config = LoopConfig::default().with_model("x").with_max_turns(5); + assert_eq!(config.model, "x"); + assert_eq!(config.max_turns, 5); + let defaults = LoopConfig::default(); + assert_eq!(config.max_tokens, defaults.max_tokens); + assert_eq!(config.context_window, defaults.context_window); + assert!((config.compact_threshold - defaults.compact_threshold).abs() < f64::EPSILON); + assert_eq!(config.auto_compact, defaults.auto_compact); + } + + #[test] + fn builder_does_not_mutate_source() { + let original = LoopConfig::default(); + let _modified = original.clone().with_max_turns(1); + assert_eq!(original.max_turns, 200); + } + + #[test] + fn validate_still_rejects_builder_built_invalid_config() { + let config = LoopConfig::default().with_compact_threshold(1.5); + assert!(config.validate().is_err()); + } } diff --git a/src/engine/bare.rs b/src/engine/bare.rs index 6261178..af73bc8 100644 --- a/src/engine/bare.rs +++ b/src/engine/bare.rs @@ -649,7 +649,7 @@ impl BareLoop { /// In debug builds, panics if called after the session has started. /// /// Build the pipeline using [`ToolPipeline::builder()`], adding middleware - /// layers **without** calling `.core()` — the registry is injected + /// layers **without** calling `.with_core()` — the registry is injected /// automatically from `self.tools` so that schema generation and dispatch /// always share the same underlying registry: /// @@ -659,7 +659,7 @@ impl BareLoop { /// use loopctl::engine::middleware::{ToolPipeline, TimeoutMiddleware}; /// /// let builder = ToolPipeline::builder() - /// .with(TimeoutMiddleware::from_secs(30)); + /// .with_middleware(TimeoutMiddleware::from_secs(30)); /// /// let mut agent = BareLoop::new(client, registry, config); /// agent.set_pipeline(builder)?; @@ -672,7 +672,7 @@ impl BareLoop { pub fn set_pipeline(&mut self, builder: ToolPipelineBuilder) -> Result<(), LoopError> { self.debug_assert_idle(); let pipeline = builder - .core(Arc::clone(&self.tools)) + .with_core(Arc::clone(&self.tools)) .build() .map_err(|e| LoopError::Config(e.to_string()))?; self.managers.set_pipeline(pipeline); @@ -789,7 +789,7 @@ impl BareLoop { /// /// let mut agent = BareLoop::new(client, registry, config); /// agent.set_request_options( - /// RequestOptions::new().tool_constraint(ToolConstraint::Strict), + /// RequestOptions::new().with_tool_constraint(ToolConstraint::Strict), /// ); /// ``` pub fn set_request_options(&mut self, options: RequestOptions) { @@ -797,6 +797,107 @@ impl BareLoop { self.request_options = options; } + /// Set the reflector, consuming `self`. Fluent mirror of + /// [`set_reflector`](BareLoop::set_reflector). + #[must_use] + pub fn with_reflector(mut self, reflector: Arc) -> Self { + self.set_reflector(reflector); + self + } + + /// Set the recovery strategy, consuming `self`. Fluent mirror of + /// [`set_recovery_strategy`](BareLoop::set_recovery_strategy). + #[must_use] + pub fn with_recovery_strategy(mut self, strategy: Arc) -> Self { + self.set_recovery_strategy(strategy); + self + } + + /// Set the context manager, consuming `self`. Fluent mirror of + /// [`set_context_manager`](BareLoop::set_context_manager). + #[must_use] + pub fn with_context_manager(mut self, manager: Arc) -> Self { + self.set_context_manager(manager); + self + } + + /// Set the stream handler, consuming `self`. Fluent mirror of + /// [`set_stream_handler`](BareLoop::set_stream_handler). + #[must_use] + pub fn with_stream_handler(mut self, handler: StreamHandler) -> Self { + self.set_stream_handler(handler); + self + } + + /// Set the hook executor, consuming `self`. Fluent mirror of + /// [`set_hook_executor`](BareLoop::set_hook_executor). + /// + /// *Requires `hooks` feature.* + #[cfg(feature = "hooks")] + #[must_use] + pub fn with_hook_executor(mut self, executor: Arc) -> Self { + self.set_hook_executor(executor); + self + } + + /// Set the tool health registry, consuming `self`. Fluent mirror of + /// [`set_health_registry`](BareLoop::set_health_registry). + /// + /// *Requires `tool_health` feature.* + #[cfg(feature = "tool_health")] + #[must_use] + pub fn with_health_registry(mut self, registry: Arc) -> Self { + self.set_health_registry(registry); + self + } + + /// Set the middleware pipeline, consuming `self`. Fluent mirror of + /// [`set_pipeline`](BareLoop::set_pipeline). + /// + /// Because building the pipeline can fail, this returns `Result` — chain it with `?`. + /// + /// # Errors + /// + /// Returns [`LoopError::Config`] if the builder fails to produce a valid + /// pipeline. See [`set_pipeline`](BareLoop::set_pipeline). + pub fn with_pipeline(mut self, builder: ToolPipelineBuilder) -> Result { + self.set_pipeline(builder)?; + Ok(self) + } + + /// Register an observer, consuming `self`. Fluent mirror of + /// [`register_observer`](BareLoop::register_observer). + #[must_use] + pub fn with_observer(mut self, observer: Arc) -> Self { + self.register_observer(observer); + self + } + + /// Set the real-time text streaming callback, consuming `self`. Fluent + /// mirror of [`set_text_streamer`](BareLoop::set_text_streamer). + #[must_use] + pub fn with_text_streamer(mut self, f: Arc) -> Self { + self.set_text_streamer(f); + self + } + + /// Register a context contributor, consuming `self`. Fluent mirror of + /// [`add_contributor`](BareLoop::add_contributor). + #[must_use] + pub fn with_contributor(mut self, contributor: Box) -> Self { + self.add_contributor(contributor); + self + } + + /// Set the per-turn request options, consuming `self`. Fluent mirror of + /// [`set_request_options`](BareLoop::set_request_options). + #[must_use] + pub fn with_request_options(mut self, options: RequestOptions) -> Self { + self.set_request_options(options); + self + } + /// Begin a model switch operation. /// /// Returns a [`ModelSwitch`] builder that lets you optionally update @@ -813,7 +914,7 @@ impl BareLoop { /// # let client = std::sync::Arc::new(MockApiClient::new("model-a")); /// # let tools = ToolRegistry::new(); /// # let mut loop_ = BareLoop::new(client, tools, LoopConfig::default()); - /// loop_.switch_model("model-b").context_window(8192).apply().unwrap(); + /// loop_.switch_model("model-b").with_context_window(8192).apply().unwrap(); /// assert_eq!(loop_.config().model, "model-b"); /// assert_eq!(loop_.config().context_window, 8192); /// ``` @@ -1240,7 +1341,7 @@ impl ModelSwitch<'_, C> { /// significantly different context window — otherwise the /// auto-compactor will use the wrong threshold. #[must_use] - pub fn context_window(mut self, tokens: u64) -> Self { + pub fn with_context_window(mut self, tokens: u64) -> Self { self.context_window = Some(tokens); self } @@ -1249,7 +1350,7 @@ impl ModelSwitch<'_, C> { /// /// If omitted, the existing `LoopConfig::max_tokens` is kept. #[must_use] - pub fn max_tokens(mut self, tokens: u32) -> Self { + pub fn with_max_tokens(mut self, tokens: u32) -> Self { self.max_tokens = Some(tokens); self } @@ -3263,7 +3364,7 @@ mod tests { registry.register(EchoTool); let config = make_config(); let mut agent = BareLoop::new(Arc::new(client), registry, config); - // Build a builder WITHOUT calling .core() — set_pipeline must inject it. + // Build a builder WITHOUT calling .with_core() — set_pipeline must inject it. let builder = ToolPipeline::builder(); agent.set_pipeline(builder).unwrap(); @@ -3316,7 +3417,8 @@ mod tests { let capture = Arc::new(Mutex::new(Vec::::new())); let mut agent = BareLoop::new(Arc::new(client), registry, config); - let builder = ToolPipeline::builder().with(TurnNumberCapture::new(Arc::clone(&capture))); + let builder = + ToolPipeline::builder().with_middleware(TurnNumberCapture::new(Arc::clone(&capture))); agent.set_pipeline(builder).unwrap(); let result = agent.run("test").await; @@ -3512,7 +3614,7 @@ mod tests { loop_ .switch_model("small-model") - .context_window(8192) + .with_context_window(8192) .apply() .unwrap(); @@ -3526,7 +3628,11 @@ mod tests { let tools = ToolRegistry::new(); let mut loop_ = BareLoop::new(client, tools, LoopConfig::default()); - loop_.switch_model("m2").max_tokens(4096).apply().unwrap(); + loop_ + .switch_model("m2") + .with_max_tokens(4096) + .apply() + .unwrap(); assert_eq!(loop_.config().model, "m2"); assert_eq!(loop_.config().max_tokens, 4096); @@ -4431,7 +4537,7 @@ mod tests { let mut agent = BareLoop::new(Arc::new(client.clone()), ToolRegistry::new(), config); agent.set_request_options( crate::structured::RequestOptions::new() - .tool_constraint(crate::structured::ToolConstraint::Strict), + .with_tool_constraint(crate::structured::ToolConstraint::Strict), ); agent.run("Hi").await.unwrap(); @@ -4661,4 +4767,45 @@ mod tests { "thinking callback fires once (for the Thinking delta)" ); } + + #[tokio::test] + async fn fluent_with_chain_builds_a_working_loop() { + let client = MockClient::new("test-model"); + client.add_text_response("done"); + + let observer = Arc::new(CountingObserver::new()); + let registered: Arc = observer.clone(); + + let mut agent = BareLoop::new(Arc::new(client), ToolRegistry::new(), make_config()) + .with_observer(registered) + .with_reflector(Arc::new(NoopReflector)) + .with_request_options(RequestOptions::default()); + + let result = agent.run("Hi").await.unwrap(); + + assert!(result.success, "the fluent-built loop runs a turn"); + assert_eq!( + observer.turn_starts.load(Ordering::SeqCst), + 1, + "with_observer registered the observer (it received the turn event)" + ); + } + + #[test] + fn fluent_with_observer_equivalent_to_register_observer() { + let client = MockClient::new("test-model"); + let observer: Arc = Arc::new(CountingObserver::new()); + + let fluent = BareLoop::new(Arc::new(client.clone()), ToolRegistry::new(), make_config()) + .with_observer(Arc::clone(&observer)); + + let mut imperative = BareLoop::new(Arc::new(client), ToolRegistry::new(), make_config()); + imperative.register_observer(Arc::clone(&observer)); + + assert_eq!( + fluent.managers.observers().len(), + imperative.managers.observers().len(), + "both paths register the same number of observers" + ); + } } diff --git a/src/engine/bare/stream.rs b/src/engine/bare/stream.rs index 0c3a497..0a4c1b2 100644 --- a/src/engine/bare/stream.rs +++ b/src/engine/bare/stream.rs @@ -54,8 +54,8 @@ impl BareLoop { let mut stream = handler.stream_turn( &*self.client, crate::api::StreamRequest::new(self.conversation.clone()) - .system_opt(self.config.system_prompt.clone()) - .tools_opt(self.build_tool_schemas()), + .with_system(self.config.system_prompt.clone()) + .with_tools(self.build_tool_schemas()), self.request_options.clone(), &self.cancelled, ); diff --git a/src/engine/loop_core.rs b/src/engine/loop_core.rs index 11a371e..9fbfec7 100644 --- a/src/engine/loop_core.rs +++ b/src/engine/loop_core.rs @@ -568,10 +568,10 @@ impl SessionResult { /// # use std::time::Duration; /// # use uuid::Uuid; /// let result = SessionResult::builder() - /// .session_id(Uuid::nil()) - /// .total_turns(5) - /// .tool_calls(3) - /// .success(true) + /// .with_session_id(Uuid::nil()) + /// .with_total_turns(5) + /// .with_tool_calls(3) + /// .with_success(true) /// .build(); /// assert_eq!(result.total_turns, 5); /// assert_eq!(result.tool_calls, 3); @@ -626,63 +626,63 @@ pub struct SessionResultBuilder { impl SessionResultBuilder { /// Set the session ID. #[must_use] - pub fn session_id(mut self, id: Uuid) -> Self { + pub fn with_session_id(mut self, id: Uuid) -> Self { self.session_id = id; self } /// Set the total turns executed. #[must_use] - pub fn total_turns(mut self, turns: usize) -> Self { + pub fn with_total_turns(mut self, turns: usize) -> Self { self.total_turns = turns; self } /// Set the total input tokens. #[must_use] - pub fn input_tokens(mut self, tokens: u64) -> Self { + pub fn with_input_tokens(mut self, tokens: u64) -> Self { self.input_tokens = tokens; self } /// Set the total output tokens. #[must_use] - pub fn output_tokens(mut self, tokens: u64) -> Self { + pub fn with_output_tokens(mut self, tokens: u64) -> Self { self.output_tokens = tokens; self } /// Set the total session duration. #[must_use] - pub fn total_duration(mut self, duration: Duration) -> Self { + pub fn with_total_duration(mut self, duration: Duration) -> Self { self.total_duration = duration; self } /// Set the total tool calls made. #[must_use] - pub fn tool_calls(mut self, calls: usize) -> Self { + pub fn with_tool_calls(mut self, calls: usize) -> Self { self.tool_calls = calls; self } /// Set whether the session succeeded. #[must_use] - pub fn success(mut self, success: bool) -> Self { + pub fn with_success(mut self, success: bool) -> Self { self.success = success; self } /// Set the final output text. #[must_use] - pub fn final_output(mut self, output: impl Into) -> Self { + pub fn with_final_output(mut self, output: impl Into) -> Self { self.final_output = Some(output.into()); self } /// Set the error message. #[must_use] - pub fn error(mut self, error: impl Into) -> Self { + pub fn with_error(mut self, error: impl Into) -> Self { self.error = Some(error.into()); self } diff --git a/src/hooks/builtin/auto_commit.rs b/src/hooks/builtin/auto_commit.rs index 618c2d2..1840814 100644 --- a/src/hooks/builtin/auto_commit.rs +++ b/src/hooks/builtin/auto_commit.rs @@ -346,28 +346,28 @@ impl AutoCommitConfigBuilder { /// Enable or disable auto-commit. #[must_use] - pub fn enabled(mut self, enabled: bool) -> Self { + pub fn with_enabled(mut self, enabled: bool) -> Self { self.config.enabled = enabled; self } /// Set the commit message template. #[must_use] - pub fn message_template(mut self, template: impl Into) -> Self { + pub fn with_message_template(mut self, template: impl Into) -> Self { self.config.message_template = template.into(); self } /// Set whether to auto-push. #[must_use] - pub fn auto_push(mut self, value: bool) -> Self { + pub fn with_auto_push(mut self, value: bool) -> Self { self.config.auto_push = value; self } /// Set files to add before commit. #[must_use] - pub fn files(mut self, files: Vec) -> Self { + pub fn with_files(mut self, files: Vec) -> Self { self.config.files = files; self } @@ -491,9 +491,9 @@ mod tests { #[test] fn config_builder() { let config = AutoCommitConfigBuilder::new() - .enabled(true) - .message_template("feat: {{tool}}") - .auto_push(true) + .with_enabled(true) + .with_message_template("feat: {{tool}}") + .with_auto_push(true) .build(); assert!(config.enabled); diff --git a/src/middleware.rs b/src/middleware.rs index 72fefa2..411a8fa 100644 --- a/src/middleware.rs +++ b/src/middleware.rs @@ -38,9 +38,9 @@ //! let registry = Arc::new(ToolRegistry::new()); //! //! let pipeline = ToolPipeline::builder() -//! .with(PermissionMiddleware::deny_all()) -//! .with(TimeoutMiddleware::from_secs(120)) -//! .core(registry) +//! .with_middleware(PermissionMiddleware::deny_all()) +//! .with_middleware(TimeoutMiddleware::from_secs(120)) +//! .with_core(registry) //! .build() //! .expect("pipeline configuration is valid"); //! @@ -73,10 +73,6 @@ pub use tool_call::ToolCallMiddleware; pub use unknown_tool::UnknownToolMiddleware; pub use verify::{NoopVerifier, Verifier, VerifyMiddleware, VerifyResult}; -// ================================================== -// Dispatch context -// ================================================== - /// Context for a single tool invocation, passed through the middleware chain. /// /// Built once per tool call by the framework, then threaded through each @@ -85,26 +81,48 @@ pub use verify::{NoopVerifier, Verifier, VerifyMiddleware, VerifyResult}; /// Session identity is available via [`tool_context.session_id`](ToolContext::session_id) /// rather than duplicated here. pub struct ToolDispatchContext { - /// From the model's tool call. Middlewares can redirect by modifying this. + /// The tool name from the model's tool call. + /// + /// Middlewares may rewrite this to redirect the call to a different tool + /// (the rewritten name is what [`ToolCallMiddleware`] looks up). pub tool_name: String, - /// Raw JSON from the model. Middlewares can inspect/transform before invocation. + + /// The raw JSON input from the model. + /// + /// Middlewares can inspect or transform it before the tool runs — for + /// example to scrub secrets, enforce limits, or fill defaults. pub input: Value, - /// Generated by the model provider, used to correlate results back to the request. + + /// The call identifier generated by the model provider. + /// + /// Used to correlate each tool result back to the request that issued the + /// call; forwarded onto [`ToolDispatchResult`] unchanged. pub call_id: String, - /// Monotonically increasing counter. Useful for per-turn limits. + + /// The monotonically increasing turn counter for this session. + /// + /// Useful for per-turn limits (e.g. capping tool calls within a turn). pub turn_number: usize, - /// Checked via `tokio::select!` in long-running middlewares (e.g. [`TimeoutMiddleware`]). + + /// The cancellation signal for this dispatch. + /// + /// Long-running middlewares (e.g. [`TimeoutMiddleware`]) should poll this + /// via `tokio::select!` so a cancel aborts in-flight work promptly. pub cancel: Arc, - /// Middlewares (e.g. [`PermissionMiddleware`]) can read and modify this. + + /// The permission policy in effect for this call. + /// + /// Middlewares such as [`PermissionMiddleware`] read this to decide allow + /// /deny and may modify it to escalate or restrict a subsequent decision. pub permission: PermissionCheck, - /// Augmented by middlewares before [`ToolCallMiddleware`] invokes `Tool::call()`. + + /// The [`ToolContext`] passed to the eventual `Tool::call()`. + /// + /// Middlewares may augment it (add extensions, set fields) before + /// [`ToolCallMiddleware`] invokes the tool. pub tool_context: ToolContext, } -// ================================================== -// Middleware trait -// ================================================== - /// The trait for a middleware in the tool dispatch pipeline. /// /// Each middleware wraps the "next" dispatch layer. The outermost middleware @@ -147,15 +165,23 @@ pub struct ToolDispatchContext { /// } /// ``` pub trait ToolMiddleware: Send + Sync { - /// Human-readable name for this middleware. + /// Return a human-readable name for this middleware. /// - /// Used in logging, diagnostics, and error messages. + /// Surfaced by [`ToolPipeline::middleware_names`] and used in logging, + /// diagnostics, and error messages. Pick something stable and distinctive + /// (e.g. `"permission"`, `"timeout"`) rather than a generated identifier. fn name(&self) -> &str; - /// Process a tool dispatch through this middleware layer. + /// Process a single tool dispatch through this middleware layer. + /// + /// This is the heart of the middleware: pre-process `ctx`, then either call + /// `next.dispatch(ctx)` to continue down the chain (and optionally + /// post-process the returned [`ToolDispatchResult`]), or return a result + /// directly to short-circuit the pipeline (e.g. deny a call before it + /// reaches the tool). /// - /// Call `next.dispatch(ctx)` to invoke the next layer in the pipeline. - /// Return a [`ToolDispatchResult`] directly to short-circuit. + /// The returned future is pinned and boxed for object safety — implementors + /// typically write `Box::pin(async move { … })`. fn dispatch<'a>( &'a self, ctx: &'a mut ToolDispatchContext, @@ -164,58 +190,101 @@ pub trait ToolMiddleware: Send + Sync { } /// Errors that can occur during pipeline construction. +/// +/// Returned by [`ToolPipelineBuilder::build`] when the builder's state is not +/// sufficient to produce a runnable pipeline. Both variants are recoverable by +/// the caller: add the missing piece to the builder and rebuild. #[derive(Debug, thiserror::Error)] pub enum PipelineError { - /// No core dispatch (tool registry) was provided. - #[error("pipeline requires a core dispatch (call .core() with a ToolRegistry)")] + /// The pipeline has middleware layers but no core dispatch. + /// + /// Returned by [`build`](ToolPipelineBuilder::build) when at least one + /// middleware was added via + /// [`with_middleware`](ToolPipelineBuilder::with_middleware) but no tool + /// registry was set via [`with_core`](ToolPipelineBuilder::with_core). + /// Without a core, a middleware chain has nothing to dispatch into. + /// + /// Fix by calling `.with_core(registry)` before `.build()`. + #[error("pipeline requires a core dispatch (call .with_core() with a ToolRegistry)")] MissingCore, - /// The pipeline is empty — no middlewares and no core. + + /// The pipeline has nothing in it at all — no middleware and no core. + /// + /// Returned by [`build`](ToolPipelineBuilder::build) when neither + /// [`with_middleware`](ToolPipelineBuilder::with_middleware) nor + /// [`with_core`](ToolPipelineBuilder::with_core) was ever called. A + /// completely empty pipeline cannot dispatch anything. + /// + /// Fix by adding at least a core: `.with_core(registry)`. (For the common + /// case of "no middleware, just the registry," prefer the shortcut + /// [`ToolPipeline::new`].) #[error("pipeline has no middlewares and no core dispatch")] Empty, } -// =================================================== -// Pipeline -// =================================================== - -/// An assembled middleware pipeline for tool dispatch. +/// An assembled, immutable middleware pipeline for tool dispatch. /// -/// Holds an ordered list of [`ToolMiddleware`] layers around an innermost -/// [`ToolCallMiddleware`] that performs the actual tool invocation via a -/// [`ToolRegistry`]. An internal `index` cursor tracks the current position -/// during dispatch — the entry point starts at index 0, and each middleware -/// receives a pipeline advanced by one position as its `next`. +/// Holds an ordered list of [`ToolMiddleware`] layers wrapped around an +/// innermost [`ToolCallMiddleware`], which performs the actual tool lookup and +/// `Tool::call()` invocation against a [`ToolRegistry`]. Once built, a pipeline +/// does not change: the middleware list and core are both held behind [`Arc`], +/// so a pipeline is cheap to share across tasks and threads. +/// +/// # The cursor model +/// +/// Rather than threading a separate "next" pointer through each layer, the +/// pipeline carries an internal `index` cursor. Entry starts at index 0; each +/// middleware receives a pipeline advanced by one position as its `next` +/// argument, and when the cursor runs off the end of the middleware list it +/// falls through to the core. Constructing those one-step-advanced views costs +/// only `Arc` clones (no middleware is copied), so the design stays allocation +/// -light per dispatch. /// /// # Construction /// -/// Use [`ToolPipeline::builder()`] to create a [`ToolPipelineBuilder`], -/// add middlewares, set the core registry, and call [`build()`](ToolPipelineBuilder::build). +/// Use [`builder`](Self::builder) to get a [`ToolPipelineBuilder`], add +/// middleware layers with +/// [`with_middleware`](ToolPipelineBuilder::with_middleware), set the core +/// registry with [`with_core`](ToolPipelineBuilder::with_core), and finalize +/// with [`build`](ToolPipelineBuilder::build). For the zero-middleware case, +/// [`new`](Self::new) is a shortcut. /// /// # Dispatch /// -/// - [`dispatch()`](ToolPipeline::dispatch) — execute a single tool call -/// through the full chain. -/// - [`dispatch_all()`](ToolPipeline::dispatch_all) — execute multiple tool -/// calls sequentially (parallel dispatch is planned). +/// - [`invoke`](Self::invoke) — the public entry point: run one tool call +/// through the full chain from index 0. +/// - [`dispatch_all`](Self::dispatch_all) — run several calls in sequence, +/// checking cancellation between them. +/// - [`dispatch`](Self::dispatch) — the internal, cursor-aware step used by +/// middlewares to continue the chain. Callers normally use `invoke`. /// /// # Example /// /// ```rust,ignore /// let pipeline = ToolPipeline::builder() -/// .with(PermissionMiddleware::deny_all()) -/// .with(TimeoutMiddleware::from_secs(120)) -/// .core(registry) +/// .with_middleware(PermissionMiddleware::deny_all()) +/// .with_middleware(TimeoutMiddleware::from_secs(120)) +/// .with_core(registry) /// .build() /// .expect("pipeline configuration is valid"); /// /// let result = pipeline.invoke(ctx).await; /// ``` pub struct ToolPipeline { - /// Outermost first. Index 0 runs first. + /// The middleware layers in execution order — index 0 is the outermost + /// (runs its pre-processing first, post-processing last). Held in an + /// `Arc<[…]>` so advancing the cursor only clones the shared slice. middlewares: Arc<[Arc]>, - /// Created from the [`ToolRegistry`] when the pipeline is built. + + /// The innermost dispatch layer, built from the [`ToolRegistry`] at + /// [`build`](ToolPipelineBuilder::build) time and wrapped in a + /// [`ToolCallMiddleware`]. Reached once the cursor passes the last + /// middleware: it performs the actual `Tool::call()`. core: Arc, - /// `0` at entry. Each middleware receives a pipeline at `index + 1`. + + /// The cursor into `middlewares`: `0` at the chain entry, incremented by + /// one for each `next` view handed to a middleware. Past-the-end means + /// "delegate to `core`". index: usize, } @@ -229,18 +298,27 @@ impl std::fmt::Debug for ToolPipeline { } impl ToolPipeline { - /// Create a new pipeline builder. + /// Create a new pipeline builder for fluent configuration. + /// + /// Returns a [`ToolPipelineBuilder`] starting with no middleware layers. + /// Add layers with + /// [`with_middleware`](ToolPipelineBuilder::with_middleware), set the core + /// with [`with_core`](ToolPipelineBuilder::with_core), then + /// [`build`](ToolPipelineBuilder::build). /// - /// Returns a [`ToolPipelineBuilder`] for fluent configuration. + /// Prefer this over [`new`](Self::new) whenever you want any middleware at + /// all; `new` is the shortcut for the zero-middleware case. #[must_use] pub fn builder() -> ToolPipelineBuilder { ToolPipelineBuilder::new() } - /// Create a minimal pipeline with only the core dispatch. + /// Create a minimal pipeline with only the core dispatch and no middleware. /// - /// Equivalent to `ToolPipeline::builder().core(registry).build().unwrap()`. - /// Useful for cases where no middleware is needed (e.g. testing). + /// Equivalent to `ToolPipeline::builder().with_core(registry).build()` with + /// the core guaranteed present. Useful for cases where no middleware is + /// needed (e.g. tests, or a host app that wants raw `Tool::call()` with no + /// wrapping). To add layers, use [`builder`](Self::builder) instead. #[must_use] pub fn new(registry: Arc) -> Self { Self { @@ -252,10 +330,18 @@ impl ToolPipeline { /// Dispatch to the middleware layer at the current cursor index. /// - /// If `index` points past the middleware list, falls through to the - /// core [`ToolCallMiddleware`]. Middlewares receive a pipeline advanced - /// by one position as their `next` and call this method to continue - /// the chain. + /// This is the internal step that walks the chain. If [`index`](Self) points + /// at a middleware, that middleware's [`dispatch`](ToolMiddleware::dispatch) + /// runs, receiving a one-step-advanced copy of the pipeline as its `next` + /// argument so it can continue the chain by calling `next.dispatch(ctx)`. + /// Once the cursor advances past the last middleware, the call falls + /// through to the core [`ToolCallMiddleware`], which performs the actual + /// `Tool::call()`. + /// + /// Callers outside the middleware trait usually want [`invoke`](Self::invoke) + /// (which starts a fresh dispatch at index 0) rather than this method. + /// This method is `pub` because middlewares receive a `&ToolPipeline` and + /// must be able to advance it. pub fn dispatch<'a>( &'a self, ctx: &'a mut ToolDispatchContext, @@ -272,11 +358,17 @@ impl ToolPipeline { Box::pin(async move { middleware.dispatch(ctx, &next).await }) } - /// Invoke the full pipeline from the beginning. + /// Run one tool call through the full pipeline from the start. /// - /// Always starts at index 0 regardless of the stored cursor — this - /// ensures correct behavior even if called on a non-root pipeline. - /// The cost is two `Arc::clone`s, which is negligible. + /// This is the public entry point for dispatch. It builds a fresh root view + /// of the pipeline at `index` 0 and drives it to completion, so it is safe + /// to call on any pipeline — including a `next` view a middleware received + /// mid-chain — without that view's advanced cursor affecting the result. + /// Building the root view costs two `Arc` clones (the middleware slice and + /// the core), which is negligible relative to a tool call. + /// + /// To run several calls with cancellation checked between them, use + /// [`dispatch_all`](Self::dispatch_all). pub async fn invoke(&self, mut ctx: ToolDispatchContext) -> ToolDispatchResult { let root = ToolPipeline { middlewares: Arc::clone(&self.middlewares), @@ -286,18 +378,21 @@ impl ToolPipeline { root.dispatch(&mut ctx).await } - /// Dispatch multiple tool calls sequentially. + /// Dispatch multiple tool calls sequentially, in input order. /// - /// Each call goes through the full middleware chain. Cancellation is - /// checked between calls — if the signal is set, remaining calls - /// are skipped and an [`LoopError::Cancelled`] is returned. + /// Each call independently traverses the full middleware chain via + /// [`invoke`](Self::invoke). The cancellation signal is checked before + /// every call and again after each one returns; if it is ever set, the + /// remaining calls are skipped and the error is returned immediately. Note + /// that any results produced before the cancellation are discarded — this + /// returns either all results or an error, never a partial batch. /// - /// Returns results in the same order as the input calls. + /// Results come back in the same order as the input calls. /// /// # Errors /// - /// Returns [`LoopError::Cancelled`] if the cancellation signal is - /// set before all calls have been dispatched. + /// Returns [`LoopError::Cancelled`] if the cancellation signal is set at + /// any check point before all calls have completed. pub async fn dispatch_all( &self, calls: Vec, @@ -317,9 +412,10 @@ impl ToolPipeline { Ok(results) } - /// Get the names of all middleware layers in order. + /// Return the names of all middleware layers in execution order. /// - /// Useful for diagnostics and logging. + /// Includes the core [`ToolCallMiddleware`] as the final entry. Useful for + /// diagnostics, logging, and asserting pipeline composition in tests. #[must_use] pub fn middleware_names(&self) -> Vec<&str> { let mut names: Vec<&str> = self.middlewares.iter().map(|m| m.name()).collect(); @@ -328,19 +424,15 @@ impl ToolPipeline { } } -// =================================================== -// Pipeline builder -// =================================================== - /// Builds an ordered middleware pipeline for tool dispatch. /// /// # Example /// /// ```rust,ignore /// let pipeline = ToolPipeline::builder() -/// .with(PermissionMiddleware::deny_all()) -/// .with(TimeoutMiddleware::from_secs(120)) -/// .core(Arc::new(tool_registry)) +/// .with_middleware(PermissionMiddleware::deny_all()) +/// .with_middleware(TimeoutMiddleware::from_secs(120)) +/// .with_core(Arc::new(tool_registry)) /// .build() /// .expect("pipeline configuration is valid"); /// ``` @@ -351,6 +443,10 @@ pub struct ToolPipelineBuilder { impl ToolPipelineBuilder { /// Create a new, empty builder. + /// + /// Start here, then chain [`with_middleware`](Self::with_middleware) calls + /// to add layers, finish with [`with_core`](Self::with_core) to set the + /// registry, and call [`build`](Self::build) to finalize. #[must_use] pub fn new() -> Self { Self { @@ -359,44 +455,70 @@ impl ToolPipelineBuilder { } } - /// Add a middleware layer to the pipeline. + /// Append a middleware layer to the pipeline. /// - /// Middlewares are executed in the order they are added (outermost first). - /// The core dispatch is always the innermost layer. + /// Layers execute in the order they are added: the first call becomes the + /// outermost wrapper (its pre-processing runs before everything else, its + /// post-processing runs last). The core dispatch registered via + /// [`with_core`](Self::with_core) is always the innermost layer. + /// + /// The middleware is boxed into an [`Arc`] internally, so each call moves + /// a distinct owned instance into the pipeline. To share one instance + /// across multiple pipelines, use + /// [`with_middleware_arc`](Self::with_middleware_arc). #[must_use] - pub fn with(mut self, middleware: M) -> Self { + pub fn with_middleware(mut self, middleware: M) -> Self { self.middlewares.push(Arc::new(middleware)); self } - /// Add an already-`Arc`-wrapped middleware. + /// Append an already-`Arc`-wrapped middleware. /// - /// Useful when the same middleware instance needs to be shared - /// across multiple pipelines. + /// Like [`with_middleware`](Self::with_middleware) but takes a pre-wrapped + /// [`Arc`]. Use this when the same middleware instance + /// must be shared across multiple pipelines: by passing the `Arc` in, + /// every pipeline that receives it observes the same underlying state + /// (rather than each getting its own owned copy as `with_middleware` would). #[must_use] - pub fn with_arc(mut self, middleware: Arc) -> Self { + pub fn with_middleware_arc(mut self, middleware: Arc) -> Self { self.middlewares.push(middleware); self } - /// Set the core tool registry for the pipeline. + /// Set the core tool registry the pipeline dispatches into. /// - /// The registry is wrapped in a [`ToolCallMiddleware`] that performs - /// the actual tool lookup and invocation — the innermost - /// layer of the pipeline. + /// The registry is wrapped in a [`ToolCallMiddleware`] that performs the + /// actual tool lookup and `Tool::call()` invocation — this is always the + /// innermost layer of the pipeline, reached after every registered + /// middleware has run its pre-processing. A pipeline without a core cannot + /// be built (see [`build`](Self::build)). #[must_use] - pub fn core(mut self, registry: Arc) -> Self { + pub fn with_core(mut self, registry: Arc) -> Self { self.core = Some(registry); self } - /// Build the pipeline. + /// Finalize the builder into an assembled [`ToolPipeline`]. + /// + /// Freezes the middleware order and wraps the core registry in a + /// [`ToolCallMiddleware`]. The returned pipeline is ready to + /// [`invoke`](ToolPipeline::invoke). /// /// # Errors /// - /// Returns [`PipelineError::MissingCore`] if no core registry was provided. + /// - [`PipelineError::Empty`] if neither middleware nor a core registry + /// was added — the builder is untouched. + /// - [`PipelineError::MissingCore`] if at least one middleware was added + /// but no core registry was set via [`with_core`](Self::with_core). A + /// middleware chain needs somewhere to dispatch. pub fn build(self) -> Result { - let registry = self.core.ok_or(PipelineError::MissingCore)?; + let Some(registry) = self.core else { + return Err(if self.middlewares.is_empty() { + PipelineError::Empty + } else { + PipelineError::MissingCore + }); + }; Ok(ToolPipeline { middlewares: self.middlewares.into(), core: Arc::new(ToolCallMiddleware::new(registry)), @@ -535,9 +657,21 @@ mod tests { } #[test] - fn test_builder_requires_core() { + fn test_builder_empty_is_empty_error() { + // No middleware and no core -> Empty (not MissingCore). let result = ToolPipeline::builder().build(); - assert!(result.is_err(), "should fail without core"); + match result { + Err(PipelineError::Empty) => {} + other => panic!("expected Empty for an untouched builder, got {other:?}"), + } + } + + #[test] + fn test_builder_middleware_without_core_is_missing_core() { + // Middleware added but no core -> MissingCore (distinct from Empty). + let result = ToolPipeline::builder() + .with_middleware(TimeoutMiddleware::none()) + .build(); match result { Err(PipelineError::MissingCore) => {} other => panic!("expected MissingCore, got {other:?}"), @@ -546,14 +680,14 @@ mod tests { #[test] fn test_builder_succeeds_with_core() { - let result = ToolPipeline::builder().core(test_registry()).build(); + let result = ToolPipeline::builder().with_core(test_registry()).build(); assert!(result.is_ok()); } #[test] fn test_pipeline_names_no_middleware() { let pipeline = ToolPipeline::builder() - .core(test_registry()) + .with_core(test_registry()) .build() .expect("valid"); assert_eq!(pipeline.middleware_names(), vec!["tool_call"]); @@ -562,9 +696,9 @@ mod tests { #[test] fn test_pipeline_names_with_middleware() { let pipeline = ToolPipeline::builder() - .with(PermissionMiddleware::allow_all()) - .with(TimeoutMiddleware::none()) - .core(test_registry()) + .with_middleware(PermissionMiddleware::allow_all()) + .with_middleware(TimeoutMiddleware::none()) + .with_core(test_registry()) .build() .expect("valid"); assert_eq!( @@ -608,8 +742,8 @@ mod tests { #[tokio::test] async fn test_permission_deny_all() { let pipeline = ToolPipeline::builder() - .with(PermissionMiddleware::deny_all()) - .core(test_registry()) + .with_middleware(PermissionMiddleware::deny_all()) + .with_core(test_registry()) .build() .expect("valid"); @@ -627,8 +761,8 @@ mod tests { #[tokio::test] async fn test_permission_allow_all() { let pipeline = ToolPipeline::builder() - .with(PermissionMiddleware::allow_all()) - .core(test_registry()) + .with_middleware(PermissionMiddleware::allow_all()) + .with_core(test_registry()) .build() .expect("valid"); @@ -639,7 +773,7 @@ mod tests { #[tokio::test] async fn test_permission_custom_check() { let pipeline = ToolPipeline::builder() - .with(PermissionMiddleware::from_context().with_check(|ctx| { + .with_middleware(PermissionMiddleware::from_context().with_check(|ctx| { if ctx.tool_name == "echo" { PermissionCheck::Allow } else { @@ -648,7 +782,7 @@ mod tests { } } })) - .core(test_registry()) + .with_core(test_registry()) .build() .expect("valid"); @@ -674,8 +808,8 @@ mod tests { }; let pipeline = ToolPipeline::builder() - .with(PermissionMiddleware::from_context()) - .core(test_registry()) + .with_middleware(PermissionMiddleware::from_context()) + .with_core(test_registry()) .build() .expect("valid"); @@ -692,8 +826,8 @@ mod tests { // from_context() has no ask_resolver configured. let pipeline = ToolPipeline::builder() - .with(PermissionMiddleware::from_context()) - .core(test_registry()) + .with_middleware(PermissionMiddleware::from_context()) + .with_core(test_registry()) .build() .expect("valid"); @@ -718,8 +852,8 @@ mod tests { }; let pipeline = ToolPipeline::builder() - .with(TimeoutMiddleware::from_secs(5)) - .core(registry) + .with_middleware(TimeoutMiddleware::from_secs(5)) + .with_core(registry) .build() .expect("valid"); @@ -736,12 +870,12 @@ mod tests { }; let pipeline = ToolPipeline::builder() - .with(TimeoutMiddleware::new(TimeoutConfig { + .with_middleware(TimeoutMiddleware::new(TimeoutConfig { timeout: Duration::from_millis(50), retry_on_timeout: false, max_retries: 0, })) - .core(registry) + .with_core(registry) .build() .expect("valid"); @@ -814,13 +948,13 @@ mod tests { }; let pipeline = ToolPipeline::builder() - .with(PermissionMiddleware::deny_all()) - .with(TimeoutMiddleware::new(TimeoutConfig { + .with_middleware(PermissionMiddleware::deny_all()) + .with_middleware(TimeoutMiddleware::new(TimeoutConfig { timeout: Duration::from_millis(50), retry_on_timeout: false, max_retries: 0, })) - .core(registry) + .with_core(registry) .build() .expect("valid"); @@ -852,10 +986,10 @@ mod tests { }; let pipeline = ToolPipeline::builder() - .with(PermissionMiddleware::allow_all()) - .with(TimeoutMiddleware::from_secs(30)) - .with(UnknownToolMiddleware::new(Arc::clone(®istry))) - .core(registry) + .with_middleware(PermissionMiddleware::allow_all()) + .with_middleware(TimeoutMiddleware::from_secs(30)) + .with_middleware(UnknownToolMiddleware::new(Arc::clone(®istry))) + .with_core(registry) .build() .expect("valid"); @@ -877,9 +1011,9 @@ mod tests { }; let pipeline = ToolPipeline::builder() - .with(PermissionMiddleware::allow_all()) - .with(UnknownToolMiddleware::new(Arc::clone(®istry)).with_threshold(0.3)) - .core(registry) + .with_middleware(PermissionMiddleware::allow_all()) + .with_middleware(UnknownToolMiddleware::new(Arc::clone(®istry)).with_threshold(0.3)) + .with_core(registry) .build() .expect("valid"); @@ -922,11 +1056,11 @@ mod tests { let reached = Arc::new(AtomicBool::new(false)); let pipeline = ToolPipeline::builder() - .with(PermissionMiddleware::deny_all()) - .with(ReachTracker { + .with_middleware(PermissionMiddleware::deny_all()) + .with_middleware(ReachTracker { reached: Arc::clone(&reached), }) - .core(test_registry()) + .with_core(test_registry()) .build() .expect("valid"); @@ -1033,8 +1167,8 @@ mod tests { async fn test_output_limit_short_text_passes_through() { let registry = long_output_registry(); let pipeline = ToolPipeline::builder() - .with(OutputLimitMiddleware::new(1000)) - .core(Arc::clone(®istry)) + .with_middleware(OutputLimitMiddleware::new(1000)) + .with_core(Arc::clone(®istry)) .build() .expect("valid"); @@ -1048,8 +1182,8 @@ mod tests { let registry = long_output_registry(); let limit = 100; let pipeline = ToolPipeline::builder() - .with(OutputLimitMiddleware::new(limit)) - .core(Arc::clone(®istry)) + .with_middleware(OutputLimitMiddleware::new(limit)) + .with_core(Arc::clone(®istry)) .build() .expect("valid"); @@ -1063,8 +1197,8 @@ mod tests { let registry = long_output_registry(); let limit = 50; let pipeline = ToolPipeline::builder() - .with(OutputLimitMiddleware::new(limit)) - .core(Arc::clone(®istry)) + .with_middleware(OutputLimitMiddleware::new(limit)) + .with_core(Arc::clone(®istry)) .build() .expect("valid"); @@ -1080,8 +1214,8 @@ mod tests { let registry = long_output_registry(); let limit = 5; let pipeline = ToolPipeline::builder() - .with(OutputLimitMiddleware::new(limit)) - .core(Arc::clone(®istry)) + .with_middleware(OutputLimitMiddleware::new(limit)) + .with_core(Arc::clone(®istry)) .build() .expect("valid"); let mut ctx = test_ctx("long_output"); @@ -1097,8 +1231,8 @@ mod tests { async fn test_output_limit_multipart_text_parts_are_truncated() { let registry = long_output_registry(); let pipeline = ToolPipeline::builder() - .with(OutputLimitMiddleware::new(3)) - .core(Arc::clone(®istry)) + .with_middleware(OutputLimitMiddleware::new(3)) + .with_core(Arc::clone(®istry)) .build() .expect("valid"); @@ -1149,8 +1283,8 @@ mod tests { // pass through unchanged. let registry = long_output_registry(); let pipeline = ToolPipeline::builder() - .with(OutputLimitMiddleware::new(100)) - .core(Arc::clone(®istry)) + .with_middleware(OutputLimitMiddleware::new(100)) + .with_core(Arc::clone(®istry)) .build() .expect("valid"); @@ -1182,9 +1316,9 @@ mod tests { // OutputLimitMiddleware should still truncate it. let registry = test_registry(); // has EchoTool let pipeline = ToolPipeline::builder() - .with(PermissionMiddleware::deny_all()) - .with(OutputLimitMiddleware::new(10)) - .core(Arc::clone(®istry)) + .with_middleware(PermissionMiddleware::deny_all()) + .with_middleware(OutputLimitMiddleware::new(10)) + .with_core(Arc::clone(®istry)) .build() .expect("valid"); @@ -1199,8 +1333,8 @@ mod tests { async fn test_output_limit_preserves_duration() { let registry = long_output_registry(); let pipeline = ToolPipeline::builder() - .with(OutputLimitMiddleware::new(10)) - .core(Arc::clone(®istry)) + .with_middleware(OutputLimitMiddleware::new(10)) + .with_core(Arc::clone(®istry)) .build() .expect("valid"); @@ -1217,8 +1351,8 @@ mod tests { async fn test_output_limit_preserves_resolved_tool_name() { let registry = long_output_registry(); let pipeline = ToolPipeline::builder() - .with(OutputLimitMiddleware::new(10)) - .core(Arc::clone(®istry)) + .with_middleware(OutputLimitMiddleware::new(10)) + .with_core(Arc::clone(®istry)) .build() .expect("valid"); @@ -1230,9 +1364,9 @@ mod tests { async fn test_output_limit_stacked_with_timeout() { let registry = long_output_registry(); let pipeline = ToolPipeline::builder() - .with(TimeoutMiddleware::from_secs(120)) - .with(OutputLimitMiddleware::new(20)) - .core(Arc::clone(®istry)) + .with_middleware(TimeoutMiddleware::from_secs(120)) + .with_middleware(OutputLimitMiddleware::new(20)) + .with_core(Arc::clone(®istry)) .build() .expect("valid"); @@ -1246,8 +1380,8 @@ mod tests { async fn test_output_limit_zero_chars_truncates_everything() { let registry = long_output_registry(); let pipeline = ToolPipeline::builder() - .with(OutputLimitMiddleware::new(0)) - .core(Arc::clone(®istry)) + .with_middleware(OutputLimitMiddleware::new(0)) + .with_core(Arc::clone(®istry)) .build() .expect("valid"); @@ -1265,8 +1399,8 @@ mod tests { // Set max_chars = 8: 5 chars < 8, but 15 bytes > 8. let registry = long_output_registry(); let pipeline = ToolPipeline::builder() - .with(OutputLimitMiddleware::new(8)) - .core(Arc::clone(®istry)) + .with_middleware(OutputLimitMiddleware::new(8)) + .with_core(Arc::clone(®istry)) .build() .expect("valid"); let ctx = ToolDispatchContext { @@ -1386,8 +1520,8 @@ mod tests { let mut registry = ToolRegistry::new(); registry.register(ThreePartTextTool); let pipeline = ToolPipeline::builder() - .with(OutputLimitMiddleware::new(6)) - .core(Arc::new(registry)) + .with_middleware(OutputLimitMiddleware::new(6)) + .with_core(Arc::new(registry)) .build() .expect("valid"); @@ -1436,8 +1570,8 @@ mod tests { let mut registry = ToolRegistry::new(); registry.register(ThreePartTextTool); let pipeline = ToolPipeline::builder() - .with(OutputLimitMiddleware::new(20)) - .core(Arc::new(registry)) + .with_middleware(OutputLimitMiddleware::new(20)) + .with_core(Arc::new(registry)) .build() .expect("valid"); @@ -1470,8 +1604,8 @@ mod tests { let mut registry = ToolRegistry::new(); registry.register(ThreePartTextTool); let pipeline = ToolPipeline::builder() - .with(OutputLimitMiddleware::new(8)) - .core(Arc::new(registry)) + .with_middleware(OutputLimitMiddleware::new(8)) + .with_core(Arc::new(registry)) .build() .expect("valid"); diff --git a/src/middleware/memoize.rs b/src/middleware/memoize.rs index bba8269..b809f32 100644 --- a/src/middleware/memoize.rs +++ b/src/middleware/memoize.rs @@ -17,7 +17,7 @@ //! //! # Registration //! -//! Register *before* `.core(registry)` so the middleware can short-circuit +//! Register *before* `.with_core(registry)` so the middleware can short-circuit //! on a hit before the inner dispatch fires: //! //! ```rust,ignore @@ -26,13 +26,13 @@ //! //! let extractor: Arc = /* … */; //! let pipeline = ToolPipeline::builder() -//! .with(MemoizingMiddleware::new( +//! .with_middleware(MemoizingMiddleware::new( //! vec!["Read".into(), "Grep".into()], //! vec!["Write".into(), "Edit".into()], //! extractor, //! 5, // ttl_turns //! )) -//! .core(registry) +//! .with_core(registry) //! .build()?; //! ``` @@ -539,13 +539,13 @@ mod tests { let call_count = Arc::new(std::sync::atomic::AtomicUsize::new(0)); let registry = Arc::new(ToolRegistry::new()); let pipeline = ToolPipeline::builder() - .with(mw) - .with(FixedOutputMiddleware { + .with_middleware(mw) + .with_middleware(FixedOutputMiddleware { output, is_error, call_count: call_count.clone(), }) - .core(registry) + .with_core(registry) .build() .expect("pipeline builds"); (pipeline, call_count) @@ -1083,14 +1083,14 @@ mod tests { let mw = make_middleware(Arc::new(PathFromInput), 10); let registry = Arc::new(ToolRegistry::new()); let pipeline = ToolPipeline::builder() - .with(mw) - .with(RoutingFixedOutputMiddleware { + .with_middleware(mw) + .with_middleware(RoutingFixedOutputMiddleware { read_output: ToolContent::from_string("file contents"), write_output: ToolContent::from_string("permission denied"), write_is_error: true, call_count: Arc::new(std::sync::atomic::AtomicUsize::new(0)), }) - .core(registry) + .with_core(registry) .build() .expect("pipeline builds"); @@ -1121,14 +1121,14 @@ mod tests { let mw = make_middleware(Arc::new(PathFromInput), 10); let registry = Arc::new(ToolRegistry::new()); let pipeline = ToolPipeline::builder() - .with(mw) - .with(RoutingFixedOutputMiddleware { + .with_middleware(mw) + .with_middleware(RoutingFixedOutputMiddleware { read_output: ToolContent::from_string("file contents"), write_output: ToolContent::from_string("wrote"), write_is_error: false, call_count: Arc::new(std::sync::atomic::AtomicUsize::new(0)), }) - .core(registry) + .with_core(registry) .build() .expect("pipeline builds"); diff --git a/src/middleware/output_limit.rs b/src/middleware/output_limit.rs index 7714011..e08a20c 100644 --- a/src/middleware/output_limit.rs +++ b/src/middleware/output_limit.rs @@ -21,8 +21,8 @@ use std::pin::Pin; /// use loopctl::middleware::OutputLimitMiddleware; /// /// let pipeline = ToolPipeline::builder() -/// .with(OutputLimitMiddleware::new(10_000)) -/// .core(registry) +/// .with_middleware(OutputLimitMiddleware::new(10_000)) +/// .with_core(registry) /// .build()?; /// ``` pub struct OutputLimitMiddleware { diff --git a/src/middleware/unknown_tool.rs b/src/middleware/unknown_tool.rs index 3a922c5..cf79e3d 100644 --- a/src/middleware/unknown_tool.rs +++ b/src/middleware/unknown_tool.rs @@ -499,8 +499,8 @@ mod tests { async fn dispatch_appends_suggestion_for_typo() { let registry = make_registry(); let pipeline = ToolPipeline::builder() - .with(UnknownToolMiddleware::new(Arc::clone(®istry))) - .core(registry) + .with_middleware(UnknownToolMiddleware::new(Arc::clone(®istry))) + .with_core(registry) .build() .expect("valid pipeline"); @@ -522,11 +522,11 @@ mod tests { async fn dispatch_suggestion_uses_post_dispatch_tool_name() { let registry = make_registry(); let pipeline = ToolPipeline::builder() - .with(UnknownToolMiddleware::new(Arc::clone(®istry))) - .with(RenameMiddleware { + .with_middleware(UnknownToolMiddleware::new(Arc::clone(®istry))) + .with_middleware(RenameMiddleware { new_name: "read_fil".into(), }) - .core(registry) + .with_core(registry) .build() .expect("valid pipeline"); @@ -547,8 +547,8 @@ mod tests { async fn dispatch_known_tool_no_suggestion() { let registry = make_registry(); let pipeline = ToolPipeline::builder() - .with(UnknownToolMiddleware::new(Arc::clone(®istry))) - .core(registry) + .with_middleware(UnknownToolMiddleware::new(Arc::clone(®istry))) + .with_core(registry) .build() .expect("valid pipeline"); diff --git a/src/middleware/verify.rs b/src/middleware/verify.rs index 5a82a71..6caa209 100644 --- a/src/middleware/verify.rs +++ b/src/middleware/verify.rs @@ -14,7 +14,7 @@ //! //! # Registration //! -//! Register `VerifyMiddleware` before `.core(registry)` in the pipeline +//! Register `VerifyMiddleware` before `.with_core(registry)` in the pipeline //! so it wraps the result on the way out. To bound diagnostics size, //! pair it with [`OutputLimitMiddleware`](super::OutputLimitMiddleware) //! registered *after* `VerifyMiddleware`, so the limit wraps the @@ -125,8 +125,8 @@ pub struct VerifyResult { /// use loopctl::middleware::{NoopVerifier, Verifier, VerifyMiddleware}; /// /// let pipeline = ToolPipeline::builder() -/// .with(VerifyMiddleware::new(Arc::new(NoopVerifier), vec!["Write".into()])) -/// .core(registry) +/// .with_middleware(VerifyMiddleware::new(Arc::new(NoopVerifier), vec!["Write".into()])) +/// .with_core(registry) /// .build()?; /// ``` pub struct NoopVerifier; @@ -165,7 +165,7 @@ impl Verifier for NoopVerifier { /// /// # Registration /// -/// Register *before* `.core(registry)` so the middleware wraps the +/// Register *before* `.with_core(registry)` so the middleware wraps the /// result on the way out: /// /// ```rust,ignore @@ -174,8 +174,8 @@ impl Verifier for NoopVerifier { /// /// let verifier: Arc = /* … */; /// let pipeline = ToolPipeline::builder() -/// .with(VerifyMiddleware::new(verifier, vec!["Write".into(), "Edit".into()])) -/// .core(registry) +/// .with_middleware(VerifyMiddleware::new(verifier, vec!["Write".into(), "Edit".into()])) +/// .with_core(registry) /// .build()?; /// ``` /// @@ -367,9 +367,9 @@ mod tests { ) -> ToolPipeline { let registry = Arc::new(ToolRegistry::new()); ToolPipeline::builder() - .with(verify) - .with(FixedOutputMiddleware { output, is_error }) - .core(registry) + .with_middleware(verify) + .with_middleware(FixedOutputMiddleware { output, is_error }) + .with_core(registry) .build() .expect("pipeline builds") } diff --git a/src/presets.rs b/src/presets.rs index 80df88a..9e2eedb 100644 --- a/src/presets.rs +++ b/src/presets.rs @@ -99,22 +99,22 @@ impl ConstrainedProfile { /// (cached alongside the result) and then through the output cap /// (truncated if the combined output exceeds the cap). /// - /// No `.core()` is set — pass the result to + /// No `.with_core()` is set — pass the result to /// [`BareLoop::set_pipeline`], which attaches the tool registry. #[must_use] pub fn pipeline_builder() -> crate::middleware::ToolPipelineBuilder { ToolPipeline::builder() - .with(VerifyMiddleware::new( + .with_middleware(VerifyMiddleware::new( Arc::new(NoopVerifier), WRITE_TOOLS.iter().map(|s| (*s).to_string()).collect(), )) - .with(MemoizingMiddleware::new( + .with_middleware(MemoizingMiddleware::new( MEMOIZED_TOOLS.iter().map(|s| (*s).to_string()).collect(), WRITE_TOOLS.iter().map(|s| (*s).to_string()).collect(), Arc::new(NoopPathExtractor), MEMOIZE_TTL_TURNS, )) - .with(OutputLimitMiddleware::new(OUTPUT_CAP_CHARS)) + .with_middleware(OutputLimitMiddleware::new(OUTPUT_CAP_CHARS)) } /// [`RequestOptions`] requesting strict tool-call decoding. @@ -123,7 +123,7 @@ impl ConstrainedProfile { /// the provider on every turn. #[must_use] pub fn request_options() -> RequestOptions { - RequestOptions::new().tool_constraint(ToolConstraint::Strict) + RequestOptions::new().with_tool_constraint(ToolConstraint::Strict) } /// Apply the profile's pipeline and goal-reminder contributor to a diff --git a/src/provider/grammar.rs b/src/provider/grammar.rs index f1e97e7..d6498ae 100644 --- a/src/provider/grammar.rs +++ b/src/provider/grammar.rs @@ -163,7 +163,7 @@ mod tests { let client = crate::provider::OpenAiClient::from_env().expect("OPENAI_* env"); let schemas = sample_schemas(); let grammar = std::sync::Arc::new(JsonSchemaGrammar::from_schemas(&schemas)); - let opts = RequestOptions::new().tool_constraint(ToolConstraint::Grammar(grammar)); + let opts = RequestOptions::new().with_tool_constraint(ToolConstraint::Grammar(grammar)); // A small fixed corpus of prompts that should each produce exactly // one valid tool call. The 99% bar is over this corpus. @@ -179,7 +179,7 @@ mod tests { for prompt in corpus { let stream = client.stream_messages_with_options( crate::api::StreamRequest::new(vec![crate::message::Message::user(prompt)]) - .tools_opt(Some(schemas.clone())), + .with_tools(Some(schemas.clone())), opts.clone(), ); let events: Vec> = diff --git a/src/structured.rs b/src/structured.rs index 790e5f0..eed7b4d 100644 --- a/src/structured.rs +++ b/src/structured.rs @@ -282,7 +282,7 @@ impl RequestOptions { /// Create empty options with no response format set. /// /// Equivalent to [`RequestOptions::default`]. Use - /// [`response_format`](Self::response_format) to chain a format + /// [`with_response_format`](Self::with_response_format) to chain a format /// builder-style. #[must_use] pub fn new() -> Self { @@ -294,7 +294,7 @@ impl RequestOptions { /// When set, the provider constrains the model's output to the schema. /// When left `None` (the default), the model's output is unconstrained. #[must_use] - pub fn response_format(mut self, rf: ResponseFormat) -> Self { + pub fn with_response_format(mut self, rf: ResponseFormat) -> Self { self.response_format = Some(rf); self } @@ -305,7 +305,7 @@ impl RequestOptions { /// [`ToolConstraint::Strict`] makes malformed tool calls structurally /// impossible via the provider's native strict mode. #[must_use] - pub fn tool_constraint(mut self, c: ToolConstraint) -> Self { + pub fn with_tool_constraint(mut self, c: ToolConstraint) -> Self { self.tool_constraint = c; self } @@ -583,7 +583,7 @@ pub async fn request_structured, system: Option, ) -> Result { - let opts = RequestOptions::new().response_format(ResponseFormat::from_type::()); + let opts = RequestOptions::new().with_response_format(ResponseFormat::from_type::()); let request = crate::api::StreamRequest { messages, system, @@ -655,7 +655,7 @@ mod tests { assert!(opts.response_format.is_none()); let rf = ResponseFormat::from_type::(); - let opts = RequestOptions::new().response_format(rf); + let opts = RequestOptions::new().with_response_format(rf); assert!(opts.response_format.is_some()); assert_eq!(opts.response_format.as_ref().unwrap().name, "action"); } @@ -762,7 +762,8 @@ mod tests { #[tokio::test] async fn default_client_rejects_response_format() { let client = PlainMockClient; - let opts = RequestOptions::new().response_format(ResponseFormat::from_type::()); + let opts = + RequestOptions::new().with_response_format(ResponseFormat::from_type::()); let request = crate::api::StreamRequest::new(vec![]); let result = client.create_message_with_options(request, opts).await; assert!( @@ -911,13 +912,13 @@ mod tests { #[test] fn request_options_tool_constraint_builder() { - let opts = RequestOptions::new().tool_constraint(ToolConstraint::Strict); + let opts = RequestOptions::new().with_tool_constraint(ToolConstraint::Strict); assert!(matches!(opts.tool_constraint, ToolConstraint::Strict)); // And response_format still composes on the same builder. let rf = ResponseFormat::from_type::(); let opts = RequestOptions::new() - .response_format(rf) - .tool_constraint(ToolConstraint::Strict); + .with_response_format(rf) + .with_tool_constraint(ToolConstraint::Strict); assert!(opts.response_format.is_some()); assert!(matches!(opts.tool_constraint, ToolConstraint::Strict)); } @@ -928,7 +929,7 @@ mod tests { // including tool_constraint — to be Clone. This test pins that by // cloning an options value that carries a constraint and asserting // both copies hold the same variant. - let opts = RequestOptions::new().tool_constraint(ToolConstraint::Strict); + let opts = RequestOptions::new().with_tool_constraint(ToolConstraint::Strict); let cloned = opts.clone(); assert!(matches!(opts.tool_constraint, ToolConstraint::Strict)); assert!(matches!(cloned.tool_constraint, ToolConstraint::Strict)); diff --git a/src/tool/shield.rs b/src/tool/shield.rs index 53d64b2..21563d7 100644 --- a/src/tool/shield.rs +++ b/src/tool/shield.rs @@ -942,12 +942,12 @@ impl ToolSafetyShield for UnixShield { /// use loopctl::tool::shield::{UnixShield, RiskPattern, CombinationRule}; /// /// let shield = UnixShield::builder() -/// .warn_threshold(0.3) -/// .block_threshold(0.6) -/// .pattern("PowerShell", vec![ +/// .with_warn_threshold(0.3) +/// .with_block_threshold(0.6) +/// .with_pattern("PowerShell", vec![ /// RiskPattern { name: "remove_item", score: 0.9, pattern: "Remove-Item" }, /// ]) -/// .combination_rule(CombinationRule { +/// .with_combination_rule(CombinationRule { /// description: "download then run", /// score: 0.8, /// triggers: &[("PowerShell", Some("Invoke-WebRequest")), ("PowerShell", Some("Invoke-Expression"))], @@ -957,26 +957,26 @@ impl ToolSafetyShield for UnixShield { pub struct UnixShieldBuilder { /// Aggregate score at or above which the built shield will return /// [`SafetyAction::Warn`]. Defaults to `0.4`; override via - /// [`warn_threshold`](Self::warn_threshold). + /// [`with_warn_threshold`](Self::with_warn_threshold). warn_threshold: f32, /// Aggregate score at or above which the built shield will return /// [`SafetyAction::Block`]. Defaults to `0.7`; override via - /// [`block_threshold`](Self::block_threshold). + /// [`with_block_threshold`](Self::with_block_threshold). block_threshold: f32, /// Per-tool single-turn risk patterns, keyed by tool name. /// /// Populated from [`UnixShield::unix_patterns`] by /// [`new`](Self::new); empty under [`blank`](Self::blank); extended - /// via [`pattern`](Self::pattern). + /// via [`with_pattern`](Self::with_pattern). patterns: HashMap<&'static str, Vec>, /// Combination rules for dangerous sequences. /// /// Populated from [`UnixShield::unix_combination_rules`] by /// [`new`](Self::new); empty under [`blank`](Self::blank); extended - /// via [`combination_rule`](Self::combination_rule). + /// via [`with_combination_rule`](Self::with_combination_rule). /// /// [`UnixShield::unix_combination_rules`]: crate::tool::shield::UnixShield::unix_combination_rules combination_rules: Vec, @@ -985,7 +985,7 @@ pub struct UnixShieldBuilder { impl UnixShieldBuilder { /// Create a builder initialised with the default Unix patterns. /// - /// Call [`pattern`](UnixShieldBuilder::pattern) to add additional + /// Call [`with_pattern`](UnixShieldBuilder::with_pattern) to add additional /// patterns, or use [`blank`](UnixShieldBuilder::blank) to start /// from scratch. #[must_use] @@ -1017,7 +1017,7 @@ impl UnixShieldBuilder { /// An aggregate score at or above this value produces a /// [`SafetyAction::Warn`] decision. #[must_use] - pub fn warn_threshold(mut self, threshold: f32) -> Self { + pub fn with_warn_threshold(mut self, threshold: f32) -> Self { self.warn_threshold = threshold.clamp(0.0, 1.0); self } @@ -1027,7 +1027,7 @@ impl UnixShieldBuilder { /// An aggregate score at or above this value produces a /// [`SafetyAction::Block`] decision. #[must_use] - pub fn block_threshold(mut self, threshold: f32) -> Self { + pub fn with_block_threshold(mut self, threshold: f32) -> Self { self.block_threshold = threshold.clamp(0.0, 1.0); self } @@ -1040,7 +1040,7 @@ impl UnixShieldBuilder { /// for a given `tool_name` also adds it to the built shield's /// [`watched_tools`](ToolSafetyShield::watched_tools) set. #[must_use] - pub fn pattern(mut self, tool_name: &'static str, patterns: Vec) -> Self { + pub fn with_pattern(mut self, tool_name: &'static str, patterns: Vec) -> Self { self.patterns.entry(tool_name).or_default().extend(patterns); self } @@ -1056,7 +1056,7 @@ impl UnixShieldBuilder { /// /// [`UnixShield::evaluate`]: crate::tool::shield::UnixShield::evaluate #[must_use] - pub fn combination_rule(mut self, rule: CombinationRule) -> Self { + pub fn with_combination_rule(mut self, rule: CombinationRule) -> Self { self.combination_rules.push(rule); self } @@ -1213,7 +1213,7 @@ mod tests { // Use a blank shield with a single rule to avoid false matches // from other built-in rules (e.g. "modify permissions then write"). let shield = UnixShieldBuilder::blank() - .combination_rule(CombinationRule { + .with_combination_rule(CombinationRule { description: "write then execute", score: 0.75, triggers: &[("Write", None), ("Bash", Some("chmod +x"))], @@ -1235,7 +1235,7 @@ mod tests { // Now test correct order: Write first, then Bash(chmod +x) as the // current call. let shield2 = UnixShieldBuilder::blank() - .combination_rule(CombinationRule { + .with_combination_rule(CombinationRule { description: "write then execute", score: 0.75, triggers: &[("Write", None), ("Bash", Some("chmod +x"))], @@ -1257,7 +1257,7 @@ mod tests { #[test] fn builder_custom_pattern() { let shield = UnixShieldBuilder::blank() - .pattern( + .with_pattern( "PowerShell", vec![RiskPattern { name: "remove_item", @@ -1281,8 +1281,8 @@ mod tests { #[test] fn builder_custom_thresholds() { let shield = UnixShield::builder() - .warn_threshold(0.1) - .block_threshold(0.2) + .with_warn_threshold(0.1) + .with_block_threshold(0.2) .build(); // sudo scores 0.6, which is above block_threshold of 0.2 let decision = shield.evaluate(&ctx("Bash", json!({ "command": "sudo ls" }), 0));