diff --git a/docs/spec/host-capability-traits-rfc.md b/docs/spec/host-capability-traits-rfc.md index 7c95ca2eb..dcd5bffbf 100644 --- a/docs/spec/host-capability-traits-rfc.md +++ b/docs/spec/host-capability-traits-rfc.md @@ -221,11 +221,19 @@ pub trait ExperienceStore: Send + Sync { } ``` -`ProgressEvent` is deliberately coarse (`Started`, `ToolCall`, `Token`, -`Finished`, `Error`). OpenHuman's `AgentProgress` is a **UI contract** consumed -by its frontend timeline, cost footer, and citation chips; it stays host-side -and is produced *from* `ProgressEvent`. If it drifts into this crate the -frontend breaks in ways unit tests will not catch. +`ProgressEvent` is deliberately coarse (`Started`, `ToolCall`, +`ToolCallFinished`, `Token`, `Finished`, `Error`). OpenHuman's `AgentProgress` +is a **UI contract** consumed by its frontend timeline, cost footer, and +citation chips; it stays host-side and is produced *from* `ProgressEvent`. If it +drifts into this crate the frontend breaks in ways unit tests will not catch. + +The test for admitting a variant is **whether only the runtime can know it**, +not a variant count. `ToolCallFinished` was added under that test (issue #88): +`ToolCall` opened a tool row and nothing closed it, so a host could not report a +tool's outcome truthfully — it would have had to leave every row running +forever or fabricate `success: true`, which puts wrong data in both the timeline +and the trace exporter. Whether a tool returned or failed is a runtime fact, in +the same category as `Started` and `Finished`. A chip or a footer is not. ### 3.10 `ModelResolver` @@ -240,6 +248,20 @@ pub trait ModelResolver: Send + Sync { } ``` +`ModelResolveRequest` carries `agent_id`, `role`, `is_team_lead` and +`model_pin`. The last was added by issue #89: `AgentDefinition.model` is +documented as the model an agent pins, and there was previously no field for it +on the request, leaving `role` as the only string a wiring author could reach +for. A host resolver reasonably reads `role` as a role vocabulary, so a model id +placed there becomes an unrecognised role and falls back to a default — the pin +dropped silently. `role` is a **host taxonomy**; `model_pin` is a **concrete +model id**; they must stay separate channels. + +The pin is **advisory**. The host decides whether it can honour it — the model +may be unconfigured, its credentials absent, or its provider down — because the +runtime has no view of any of that. A runtime that honoured pins itself would +route to models the host cannot call. + --- ## 4. What is deliberately *not* a trait diff --git a/src/harness/host/model_resolver.rs b/src/harness/host/model_resolver.rs index 285d614be..196b29b24 100644 --- a/src/harness/host/model_resolver.rs +++ b/src/harness/host/model_resolver.rs @@ -52,6 +52,11 @@ use crate::harness::model::ChatModel; /// putting them here would mean the runtime had already made the decision it is /// supposed to be delegating. /// +/// [`model_pin`](Self::model_pin) is consistent with that rule rather than an +/// exception to it. A pin is a *declared fact* — this agent's definition names +/// this model — not a conclusion the runtime reached. Passing it along still +/// leaves the host to decide whether to honour it; see the field docs. +/// /// Fields are public so a host can pattern-match without accessor ceremony; the /// builder methods exist for call sites that construct one inline. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Default)] @@ -82,24 +87,56 @@ pub struct ModelResolveRequest { /// hosts key on first (lead model vs subagent model). #[serde(default)] pub is_team_lead: bool, + + /// Exact model id the agent's definition pinned, if any. + /// + /// Distinct from [`role`](Self::role) and the distinction is the whole + /// point: a role is a **host taxonomy** ("chat", "background", "thinking"), + /// whereas this is a **concrete model id** (`claude-3-5-sonnet`, a BYOK id, + /// a local model name). Before this field existed, `role` was the only + /// string on the request, so a wiring author needing a pin honoured would + /// naturally put the model id there — and a host resolver reasonably + /// treating `role` as a role vocabulary would fail to recognise it and fall + /// back to a default. The pin would be dropped silently, which is precisely + /// the ambiguity this field removes. + /// + /// **Advisory, not binding.** The host decides whether it can honour the + /// pin — the model may be unconfigured, the credentials absent, the + /// provider down, or the id simply unknown — and is free to resolve + /// something else or return an error. Keeping that judgement host-side is + /// deliberate: the runtime has no view of credentials or provider health, + /// so a runtime that honoured pins itself would route to models the host + /// cannot actually call. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub model_pin: Option, } impl ModelResolveRequest { - /// A request for `agent_id` with no role and not a team lead. + /// A request for `agent_id` with no role, no model pin, and not a team lead. pub fn new(agent_id: impl Into) -> Self { Self { agent_id: agent_id.into(), role: None, is_team_lead: false, + model_pin: None, } } /// Sets the host-defined role. + /// + /// Pass a *role*, never a model id — see + /// [`with_model_pin`](Self::with_model_pin) for the latter. pub fn with_role(mut self, role: impl Into) -> Self { self.role = Some(role.into()); self } + /// Sets the exact model id the agent's definition pinned. + pub fn with_model_pin(mut self, model: impl Into) -> Self { + self.model_pin = Some(model.into()); + self + } + /// Marks this agent as the lead of its team. pub fn as_team_lead(mut self) -> Self { self.is_team_lead = true; @@ -118,6 +155,21 @@ impl ModelResolveRequest { .map(str::trim) .filter(|r| !r.is_empty()) } + + /// The pinned model id as a borrowed string, or `None` when unset. + /// + /// Blank-trims for the same reason [`role`](Self::role) does, and the + /// consequence here is worse: a definition with `model = ""` reaching a + /// resolver as `Some("")` would be looked up as a model literally named + /// empty-string, missing, and reported as an unroutable pin — turning a + /// cosmetic config blank into a failed turn instead of the intended + /// "no pin, route normally". + pub fn model_pin(&self) -> Option<&str> { + self.model_pin + .as_deref() + .map(str::trim) + .filter(|m| !m.is_empty()) + } } // ── ModelResolver ───────────────────────────────────────────────────────────── @@ -232,6 +284,7 @@ mod tests { assert_eq!(req.agent_id, "planner"); assert_eq!(req.role(), None); assert!(!req.is_team_lead); + assert_eq!(req.model_pin(), None); } #[test] @@ -243,6 +296,49 @@ mod tests { assert!(req.is_team_lead); } + #[test] + fn model_pin_is_a_separate_channel_from_role() { + // The bug #89 exists to prevent: with no pin field, a wiring author + // puts the model id in `role`, the host reads it as an unknown role and + // silently falls back to a default. The two must never collapse into + // one string. + let req = ModelResolveRequest::new("planner") + .with_role("researcher") + .with_model_pin("claude-3-5-sonnet"); + assert_eq!(req.role(), Some("researcher")); + assert_eq!(req.model_pin(), Some("claude-3-5-sonnet")); + } + + #[test] + fn a_pin_can_be_set_without_a_role() { + // The common case for a user-authored agent: it pins a model and + // declares no role at all. + let req = ModelResolveRequest::new("planner").with_model_pin("local-llama"); + assert_eq!(req.role(), None); + assert_eq!(req.model_pin(), Some("local-llama")); + } + + #[test] + fn blank_model_pin_reads_as_absent() { + // `model = ""` in a definition means "no pin", not a model named "". + // Reaching a resolver as Some("") would fail the lookup and turn a + // cosmetic config blank into an unroutable turn. + for blank in ["", " ", "\t\n"] { + let req = ModelResolveRequest::new("planner").with_model_pin(blank); + assert_eq!( + req.model_pin(), + None, + "blank pin {blank:?} must read as absent" + ); + } + } + + #[test] + fn model_pin_accessor_trims_surrounding_whitespace() { + let req = ModelResolveRequest::new("planner").with_model_pin(" gpt-4o "); + assert_eq!(req.model_pin(), Some("gpt-4o")); + } + #[test] fn blank_role_reads_as_absent() { // A host mapping a missing field to `Some("")` must not select a @@ -269,12 +365,14 @@ mod tests { assert!(req.agent_id.is_empty()); assert_eq!(req.role(), None); assert!(!req.is_team_lead); + assert_eq!(req.model_pin(), None); } #[test] fn request_round_trips_through_serde() { let req = ModelResolveRequest::new("planner") .with_role("researcher") + .with_model_pin("claude-3-5-sonnet") .as_team_lead(); let json = serde_json::to_string(&req).expect("serialize"); let back: ModelResolveRequest = serde_json::from_str(&json).expect("deserialize"); @@ -293,6 +391,19 @@ mod tests { assert_eq!(back, req); } + #[test] + fn absent_model_pin_is_omitted_and_restored() { + let req = ModelResolveRequest::new("planner"); + let json = serde_json::to_string(&req).expect("serialize"); + assert!( + !json.contains("model_pin"), + "unset model pin must not serialize: {json}" + ); + let back: ModelResolveRequest = serde_json::from_str(&json).expect("deserialize"); + assert_eq!(back, req); + assert_eq!(back.model_pin(), None); + } + #[test] fn request_deserializes_from_agent_id_alone() { // Hosts hand-writing this in config should not have to spell out @@ -333,9 +444,15 @@ mod tests { .resolve(&ModelResolveRequest::new("worker").with_role("scribe")) .await .expect("resolve worker"); + // A pin is advisory: a host with exactly one model is entitled to + // ignore it, and must not be expected to look the id up. + let pinned = resolver + .resolve(&ModelResolveRequest::new("pinned").with_model_pin("gpt-4o")) + .await + .expect("resolve pinned"); assert!( - Arc::ptr_eq(&lead, &worker), - "the fixed resolver must not route on agent id, role, or lead status" + Arc::ptr_eq(&lead, &worker) && Arc::ptr_eq(&lead, &pinned), + "the fixed resolver must not route on agent id, role, lead status, or model pin" ); } diff --git a/src/harness/host/progress_sink.rs b/src/harness/host/progress_sink.rs index 26e72fabf..c5c766da4 100644 --- a/src/harness/host/progress_sink.rs +++ b/src/harness/host/progress_sink.rs @@ -38,9 +38,9 @@ //! //! # Why [`ProgressEvent`] stays coarse //! -//! Five variants, and it should stay at five. A host's user-facing progress -//! model — timeline entries, cost footers, citation chips, avatars, retry -//! badges — is a **host contract** owned by that host's UI, and it is +//! Six variants, and the bar for a seventh is high. A host's user-facing +//! progress model — timeline entries, cost footers, citation chips, avatars, +//! retry badges — is a **host contract** owned by that host's UI, and it is //! *produced from* these events by host-side adapter code. It must not migrate //! into this enum. //! @@ -51,6 +51,25 @@ //! chip, missing footer, stale timeline row — which unit tests in *either* //! repository will not catch, because both sides still compile and both sides //! still pass. Keep presentation on the host side of the seam. +//! +//! # The admission test: runtime facts, not presentation +//! +//! "Coarse" is not a variant budget, it is a category rule, and the rule is +//! whether **only the runtime can know it**. `Started`, `ToolCall`, +//! `ToolCallFinished`, `Token`, `Finished` and `Error` are all facts about what +//! the engine did; no host can derive any of them from the outside. A chip, a +//! footer or a badge is computed *from* those facts by host code, and belongs +//! there. Ask that question of a proposed variant before counting variants. +//! +//! [`ProgressEvent::ToolCallFinished`] was admitted under exactly that test. +//! This module previously declared that there was intentionally no completion +//! variant, on the reasoning that tool results are large and can carry +//! untrusted text — true of the *payload*, but it left `ToolCall` opening a +//! timeline row that nothing could ever close. `Started`/`Finished` bracket a +//! run; tool calls now bracket the same way. The asymmetry was an oversight +//! rather than a boundary, and the payload concern is answered by policy on the +//! field (see [`ToolCallFinished::output`](Self::ToolCallFinished)) instead of +//! by withholding the outcome. use std::sync::{Arc, Mutex}; @@ -80,7 +99,7 @@ use crate::harness::usage::Usage; /// # Relationship to `AgentEvent` /// /// [`AgentEvent`](crate::harness::events::AgentEvent) is the authoritative, -/// fine-grained event stream, and these five variants are a deliberately coarse +/// fine-grained event stream, and these six variants are a deliberately coarse /// projection of it (`Started` ← `RunStarted`, `Token` ← `ModelDelta`, and so /// on). `AgentEvent` is the source of truth; `ProgressEvent` is derived. /// @@ -111,11 +130,16 @@ pub enum ProgressEvent { /// A tool invocation started. /// - /// There is intentionally **no** completion variant and no arguments or - /// result payload. Tool results can be large and can carry untrusted or - /// sensitive text; a progress side channel is the wrong place to fan them - /// out. A host that wants to render outcomes classifies them itself from - /// the turn result. + /// Closed by at most one [`ToolCallFinished`](Self::ToolCallFinished) + /// carrying the same `call`. **At most**, not exactly one: a run that fails + /// or is cancelled mid-tool emits `Error` and no closing event, and events + /// may be dropped under backpressure. A host must therefore tear down open + /// tool rows when [`is_terminal`](Self::is_terminal) fires, rather than + /// waiting for a close that may never arrive. + /// + /// Call arguments are deliberately absent. They can be large and can carry + /// untrusted or sensitive text, and a host does not need them here — it + /// owns the tool being invoked and already has them. ToolCall { /// The run this event belongs to. run: RunId, @@ -125,6 +149,40 @@ pub enum ProgressEvent { tool: String, }, + /// A tool invocation finished, successfully or not. + /// + /// Correlates with the opening [`ToolCall`](Self::ToolCall) by `call`. This + /// is a runtime fact, not a presentation concern: only the engine knows + /// whether the tool returned or failed, so no host can derive it. Without + /// it a row opened by `ToolCall` could never be closed truthfully — see the + /// admission test in the module docs. + /// + /// **Never synthesise this event.** The tempting shortcut is to infer + /// completion from the arrival of the next event and assume success. That + /// establishes *that* a tool finished but not *how*, and a fabricated + /// `success: true` writes wrong data into both the host's timeline and its + /// trace exporter. A row visibly stuck in a running state is a better + /// outcome than a confidently incorrect one: the first gets noticed, the + /// second does not. + ToolCallFinished { + /// The run this event belongs to. + run: RunId, + /// The individual call, matching the opening [`ToolCall`](Self::ToolCall). + call: CallId, + /// Whether the tool returned successfully. + success: bool, + /// Raw tool output, or empty when the runtime ran with payload capture + /// off. + /// + /// Deliberately carries **no** truncation or redaction policy: both are + /// host decisions, and a host must apply its own before rendering or + /// exporting this. Empty is ambiguous by construction — it means "not + /// captured" or "genuinely empty", and a host needing to tell those + /// apart must consult its own capture configuration rather than + /// inferring from this field. + output: String, + }, + /// A chunk of assistant output became available. /// /// Named `Token` for the RFC's sake, but `text` is whatever the provider @@ -174,6 +232,7 @@ impl ProgressEvent { match self { Self::Started { run, .. } | Self::ToolCall { run, .. } + | Self::ToolCallFinished { run, .. } | Self::Token { run, .. } | Self::Finished { run, .. } | Self::Error { run, .. } => run, @@ -186,6 +245,11 @@ impl ProgressEvent { /// Lets a host tear down per-run UI state without matching on every /// variant, so adding a mid-turn variant later cannot silently turn into a /// leaked progress row. + /// + /// [`ToolCallFinished`](Self::ToolCallFinished) is **not** terminal despite + /// the name — it ends a tool call, not the run, and a turn typically emits + /// several before `Finished`. Treating it as terminal would tear the run's + /// UI down at the first tool result. pub fn is_terminal(&self) -> bool { matches!(self, Self::Finished { .. } | Self::Error { .. }) } @@ -341,6 +405,15 @@ mod tests { } } + fn tool_call_finished(success: bool, output: &str) -> ProgressEvent { + ProgressEvent::ToolCallFinished { + run: run(), + call: CallId::new("call-1"), + success, + output: output.to_string(), + } + } + fn token(text: &str) -> ProgressEvent { ProgressEvent::Token { run: run(), @@ -368,7 +441,14 @@ mod tests { } fn all_variants() -> Vec { - vec![started(), tool_call(), token("hi"), finished(), error()] + vec![ + started(), + tool_call(), + tool_call_finished(true, "result"), + token("hi"), + finished(), + error(), + ] } // ── Value-type invariants ──────────────────────────────────────────────── @@ -389,6 +469,52 @@ mod tests { assert!(error().is_terminal()); } + #[test] + fn tool_completion_is_not_terminal() { + // A turn emits one of these per tool call, several before `Finished`. + // If this ever reads as terminal a host tears its run UI down at the + // first tool result, which looks like a truncated turn rather than a + // bug in this predicate. + assert!(!tool_call_finished(true, "ok").is_terminal()); + assert!(!tool_call_finished(false, "boom").is_terminal()); + } + + #[test] + fn tool_completion_correlates_with_its_opening_call() { + // The `call` id is the only thing joining the two events. If they ever + // stop matching, a host closes the wrong timeline row and the bug + // surfaces as a mis-rendered tool, not as a failure here. + let ( + ProgressEvent::ToolCall { call: opened, .. }, + ProgressEvent::ToolCallFinished { call: closed, .. }, + ) = (tool_call(), tool_call_finished(true, "ok")) + else { + panic!("constructors changed shape"); + }; + assert_eq!(opened, closed); + } + + #[test] + fn failure_is_representable_and_distinct_from_success() { + // The whole point of #88: a host must be able to report that a tool + // failed. If these ever compare equal, `success` has stopped carrying + // information and every failed tool renders as a successful one. + assert_ne!( + tool_call_finished(true, "same"), + tool_call_finished(false, "same") + ); + } + + #[test] + fn uncaptured_output_is_representable() { + // Empty output is a legitimate state (payload capture off), not a + // reason to withhold the outcome — success must still round-trip. + let ev = tool_call_finished(false, ""); + let json = serde_json::to_string(&ev).expect("serialize"); + let back: ProgressEvent = serde_json::from_str(&json).expect("deserialize"); + assert_eq!(back, ev); + } + #[test] fn events_round_trip_through_serde() { for ev in all_variants() { @@ -403,6 +529,13 @@ mod tests { let json = serde_json::to_value(tool_call()).expect("serialize"); assert_eq!(json["kind"], "tool_call"); assert_eq!(json["tool"], "search"); + + // Distinct tag from `tool_call` — these cross a serde boundary into + // host code, so a collision would silently merge open and close. + let json = serde_json::to_value(tool_call_finished(false, "boom")).expect("serialize"); + assert_eq!(json["kind"], "tool_call_finished"); + assert_eq!(json["success"], false); + assert_eq!(json["output"], "boom"); } #[test] @@ -448,7 +581,7 @@ mod tests { sink.emit(ev).await; } assert_eq!(sink.events(), all_variants()); - assert_eq!(sink.len(), 5); + assert_eq!(sink.len(), all_variants().len()); assert!(!sink.is_empty()); }