From 5ca98aa636a8cfc33706f48520c0fa598794b5b2 Mon Sep 17 00:00:00 2001 From: Steven Enamakel Date: Thu, 13 Aug 2026 15:58:52 +0300 Subject: [PATCH] feat(host): close the tool-call bracket and route the model pin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent gaps in the host capability seam, both found by review on tinyhumansai/openhuman#5396 and both blocking that repo's Phase 4 repointing. ToolCallFinished (#88) ---------------------- `ProgressEvent::ToolCall` opened a tool but nothing closed it: `Started` and `Finished` bracket a run, and tool calls did not bracket at all. A host adapter therefore had no way to report a tool's outcome. That left two bad options — leave every timeline row running forever, or infer completion from the next event and assume success, which writes a fabricated `success: true` into both the timeline and the trace exporter. Wrong data is worse than missing data here: a row stuck running gets noticed, a confident lie does not. The module previously declared the omission deliberate, reasoning that tool results are large and can carry untrusted text. That is true of the payload, not of the outcome, so the payload concern is answered with policy on the `output` field — no truncation, no redaction, both explicitly host decisions — rather than by withholding whether the tool succeeded. Restates coarseness as the category test it always was: a variant is admissible when only the runtime can know it. All six variants pass that test; a chip, a footer or a retry badge does not, and stays host-side. `ToolCallFinished` is deliberately not terminal — a turn emits several before `Finished`, and treating it as terminal would tear a run's UI down at the first tool result. model_pin (#89) --------------- `AgentDefinition.model` records the model an agent pins, and `ModelResolveRequest` had no field to carry it, 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. Separate channels, so the ambiguity cannot arise. The pin is advisory: the host decides whether it can honour it, since the runtime has no view of credentials or provider health and would otherwise route to models the host cannot call. Blank-trims like `role()`, because `model = ""` means "no pin" and reaching a resolver as `Some("")` would turn a cosmetic config blank into a failed turn. Both additions are backward compatible — `model_pin` is `#[serde(default, skip_serializing_if)]`, and `ProgressEvent` gains a variant rather than changing one. Closes #88 Closes #89 Co-authored-by: Medulla --- docs/spec/host-capability-traits-rfc.md | 32 ++++- src/harness/host/model_resolver.rs | 123 ++++++++++++++++++- src/harness/host/progress_sink.rs | 155 ++++++++++++++++++++++-- 3 files changed, 291 insertions(+), 19 deletions(-) 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()); }