diff --git a/Cargo.lock b/Cargo.lock index cc57ddd..720be5a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -428,6 +428,7 @@ dependencies = [ "agentkit-tools-core", "agentkit-tools-derive", "async-trait", + "getrandom 0.4.3", "mlua", "runlet", "schemars 1.2.2", diff --git a/benchmarks/compose-bench/src/scenario.rs b/benchmarks/compose-bench/src/scenario.rs index f739fee..f9cda9a 100644 --- a/benchmarks/compose-bench/src/scenario.rs +++ b/benchmarks/compose-bench/src/scenario.rs @@ -339,6 +339,7 @@ mod tests { fn test_context() -> OwnedToolContext { OwnedToolContext { + failure_observer: None, session_id: SessionId::new("s"), turn_id: TurnId::new("t"), metadata: MetadataMap::new(), diff --git a/book/src/ch18-task-management.md b/book/src/ch18-task-management.md index 76100bb..b698358 100644 --- a/book/src/ch18-task-management.md +++ b/book/src/ch18-task-management.md @@ -185,6 +185,34 @@ Correlate the two events by `call_id`. Hosts that need a fully reconstructable t └────────────────────────────────┘ ``` +## Typed failures and cancellation observations + +Native failed outcomes emit `TaskEvent::Failed`, not `Completed`. Cancellation +emits `TaskEvent::Cancelled`; its snapshot retains the terminal classification +and any validated diagnostic metadata in `failure`. `ToolError::is_cancelled()` +recognizes both the legacy unit variant and typed diagnostic cancellation. + +Each task also owns an isolated observation slot. Host producers can clone +`context.failure_observer()` and publish positive effects observations, a final +retry summary, or a host fatal receipt. The manager seals the slot before aborting +cancelled work. Already-published facts survive cleanup; missing facts remain +unknown. `failure_observations` on the task snapshot is deliberately separate from +the native child's diagnostic metadata: these may describe different scopes. + +Foreground results, background loop updates, manual results, and detached +notifications retain these facts in reserved result metadata. Use +`agentkit_task_manager::tool_failure_info` and `task_failure_observations` to read +the typed projections. Request metadata cannot forge these host-owned fields; +successful results have failure-only fields removed. Retry observations never +make a child invocation safe to replay. + +When interrupting a turn, the loop drains the manager's real frozen cancellation +results before synthesizing missing results. A custom manager that delegates +interruption must also delegate `take_interrupted_task_updates`. + +The complete wire, Runlet catch/rethrow, and downstream compatibility contract is +in [Typed failure transport](https://github.com/danielkov/agentkit/blob/main/docs/typed-failure-transport.md). + ## Choosing a routing strategy | Scenario | Recommended routing | Why | @@ -199,3 +227,5 @@ Correlate the two events by `call_id`. Hosts that need a fully reconstructable t > **Example:** [`openrouter-parallel-agent`](https://github.com/danielkov/agentkit/tree/main/examples/openrouter-parallel-agent) uses `AsyncTaskManager` with `ForegroundThenDetachAfter` routing for shell tools and foreground routing for filesystem tools. The `TaskManagerHandle` event stream is printed to stderr. > > **Crate:** [`agentkit-task-manager`](https://github.com/danielkov/agentkit/tree/main/crates/agentkit-task-manager) — depends on [`agentkit-tools-core`](https://github.com/danielkov/agentkit/tree/main/crates/agentkit-tools-core), [`agentkit-core`](https://github.com/danielkov/agentkit/tree/main/crates/agentkit-core), and [tokio](https://tokio.rs). + +Individual approval closure awaits `TaskManager::close_suspended_task` and delivers its frozen terminal result without a duplicate queue entry. `LoopDriver::cancel_pending_approval_for` is therefore async; callers must await it. Retained-task manager wrappers must delegate this method, `take_terminal_task_result`, and the scoped interruption drain. Approved continuation start failure consumes an already-selected terminal winner before synthesizing an error; terminal-result transfer never closes a suspended task. Queued updates preserve originating session/task identity, and unsurfaced background approval cancellation preserves its loop/manual delivery policy. diff --git a/crates/agentkit-core/src/failure.rs b/crates/agentkit-core/src/failure.rs new file mode 100644 index 0000000..c924030 --- /dev/null +++ b/crates/agentkit-core/src/failure.rs @@ -0,0 +1,371 @@ +//! Closed, payload-free diagnostic facts. These observations never authorize replay. + +use crate::retry::{ + ProviderClassification, ProviderFailure, ProviderFailureReason, ProviderRoute, RetryAccounting, + UpstreamErrorKind, +}; +use serde::{Deserialize, Deserializer, Serialize}; +use std::time::Duration; + +/// Maximum encoded metadata accepted at an untrusted transport boundary. +pub const MAX_FAILURE_METADATA_BYTES: usize = 4096; + +/// Static validation failure; rejected input is never included in its display. +#[derive(Clone, Copy, Debug, PartialEq, Eq, thiserror::Error)] +#[error("invalid failure metadata")] +pub struct FailureMetadataDecodeError; + +/// Host-issued receipt identifier. Grammar validation is not host authentication. +#[derive(Clone, Debug, PartialEq, Eq, Serialize)] +#[serde(transparent)] +pub struct HostReceiptId(String); +impl HostReceiptId { + pub fn new(value: impl AsRef) -> Result { + let value = value.as_ref(); + if value.is_empty() + || value.len() > 128 + || !value + .bytes() + .all(|c| c.is_ascii_alphanumeric() || c == b'_' || c == b'-') + { + return Err(FailureMetadataDecodeError); + } + Ok(Self(value.to_owned())) + } + pub fn as_str(&self) -> &str { + &self.0 + } +} +impl<'de> Deserialize<'de> for HostReceiptId { + fn deserialize>(d: D) -> Result { + struct IdVisitor; + impl serde::de::Visitor<'_> for IdVisitor { + type Value = HostReceiptId; + fn expecting(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter.write_str("bounded host receipt identifier") + } + fn visit_str(self, value: &str) -> Result { + HostReceiptId::new(value).map_err(serde::de::Error::custom) + } + } + d.deserialize_str(IdVisitor) + .map_err(|_| serde::de::Error::custom(FailureMetadataDecodeError)) + } +} + +/// Retention at emission time, not a promise of eternal availability. +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum FatalStorage { + Stored, + /// Only when a retrievable in-memory record actually exists. + MemoryOnly, + Unavailable, +} + +/// Allocate once at the emitting host boundary; never reconstruct from a path. +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct HostFatalReceipt { + pub session_id: HostReceiptId, + pub event_id: HostReceiptId, + pub storage: FatalStorage, +} + +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum FailureCode { + ChildFailed, + ChildTransportFailed, + ChildCancelled, + ProviderFailed, + HostFailed, + #[default] + Unknown, +} + +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum ObservationSource { + #[default] + Unknown, + /// Reports during a dispatched child prompt, not local invocation receipts. + AcpNotifications, + /// Cumulative within this live session owner, not earlier persisted history. + LocalSession, +} + +/// Recovery-owned positive facts. False means not observed; completion does not +/// imply success, commit, rollback, or completion of every invocation. Producers +/// retain true flags monotonically within one scope; do not merge foreign scopes. +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[serde(default, deny_unknown_fields)] +pub struct PossibleEffects { + pub source: ObservationSource, + pub assistant_output_observed: bool, + pub tool_emission_observed: bool, + pub tool_execution_start_reported: bool, + pub tool_execution_completion_reported: bool, + #[serde(deserialize_with = "incomplete")] + observation_incomplete: bool, +} +fn incomplete<'de, D: Deserializer<'de>>(d: D) -> Result { + if bool::deserialize(d)? { + Ok(true) + } else { + Err(serde::de::Error::custom(FailureMetadataDecodeError)) + } +} +impl Default for PossibleEffects { + fn default() -> Self { + Self { + source: ObservationSource::Unknown, + assistant_output_observed: false, + tool_emission_observed: false, + tool_execution_start_reported: false, + tool_execution_completion_reported: false, + observation_incomplete: true, + } + } +} +impl PossibleEffects { + pub fn observation_incomplete(&self) -> bool { + self.observation_incomplete + } +} + +/// Versioned finite diagnostic leaf. No provider messages, paths, arbitrary JSON, +/// recursive errors, or host control capabilities belong in this durable value. +#[derive(Clone, Debug, PartialEq, Eq, Serialize)] +pub struct FailureMetadataV1 { + version: u8, + code: FailureCode, + #[serde(skip_serializing_if = "Option::is_none")] + retry: Option, + #[serde(skip_serializing_if = "Option::is_none")] + receipt: Option, + #[serde(skip_serializing_if = "Option::is_none")] + effects: Option, +} +impl Default for FailureMetadataV1 { + fn default() -> Self { + Self::new(FailureCode::Unknown) + } +} +impl FailureMetadataV1 { + pub fn new(code: FailureCode) -> Self { + Self { + version: 1, + code, + retry: None, + receipt: None, + effects: None, + } + } + pub fn code(&self) -> FailureCode { + self.code + } + pub fn retry(&self) -> Option<&ProviderFailure> { + self.retry.as_ref() + } + pub fn receipt(&self) -> Option<&HostFatalReceipt> { + self.receipt.as_ref() + } + pub fn effects(&self) -> Option<&PossibleEffects> { + self.effects.as_ref() + } + pub fn with_retry( + mut self, + retry: ProviderFailure, + ) -> Result { + if retry + .upstream + .http_status + .is_some_and(|s| !(100..=599).contains(&s)) + { + return Err(FailureMetadataDecodeError); + } + self.retry = Some(retry); + Ok(self) + } + pub fn with_receipt(mut self, receipt: HostFatalReceipt) -> Self { + self.receipt = Some(receipt); + self + } + pub fn with_effects(mut self, effects: PossibleEffects) -> Self { + self.effects = Some(effects); + self + } + /// Use at untrusted boundaries BEFORE general JSON parsing. Fixed wire objects + /// bound structural depth; serde's recursion limit also rejects nested input. + pub fn from_slice(bytes: &[u8]) -> Result { + if bytes.len() > MAX_FAILURE_METADATA_BYTES { + return Err(FailureMetadataDecodeError); + } + serde_json::from_slice(bytes).map_err(|_| FailureMetadataDecodeError) + } +} + +// Strict wire layout only: canonical retry value enums and standalone serde stay +// unchanged. The new boundary closes nested objects instead of weakening legacy +// compatibility or maintaining a competing retry classification contract. +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +struct WireMetadata { + version: u8, + #[serde(default)] + code: FailureCode, + retry: Option, + receipt: Option, + effects: Option, +} +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +struct WireDuration { + secs: u64, + nanos: u32, +} +impl WireDuration { + fn checked(self) -> Result { + if self.nanos >= 1_000_000_000 { + return Err(FailureMetadataDecodeError); + } + Ok(Duration::new(self.secs, self.nanos)) + } +} +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +struct WireAccounting { + attempts: u64, + completed_backoff: WireDuration, + elapsed: WireDuration, +} +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +struct WireUpstream { + error_type: UpstreamErrorKind, + code: UpstreamErrorKind, + http_status: Option, +} +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +struct WireRetry { + route: ProviderRoute, + reason: ProviderFailureReason, + last_attempt_reason: Option, + upstream: WireUpstream, + accounting: WireAccounting, +} +impl WireRetry { + fn checked(self) -> Result { + Ok(ProviderFailure { + route: self.route, + reason: self.reason, + last_attempt_reason: self.last_attempt_reason, + upstream: ProviderClassification { + error_type: self.upstream.error_type, + code: self.upstream.code, + http_status: self.upstream.http_status, + }, + accounting: RetryAccounting { + attempts: self.accounting.attempts, + completed_backoff: self.accounting.completed_backoff.checked()?, + elapsed: self.accounting.elapsed.checked()?, + }, + }) + } +} +impl<'de> Deserialize<'de> for FailureMetadataV1 { + fn deserialize>(d: D) -> Result { + let wire = WireMetadata::deserialize(d) + .map_err(|_| serde::de::Error::custom(FailureMetadataDecodeError))?; + if wire.version != 1 { + return Err(serde::de::Error::custom(FailureMetadataDecodeError)); + } + let mut value = Self::new(wire.code); + value.receipt = wire.receipt; + value.effects = wire.effects; + if let Some(retry) = wire.retry { + value = value + .with_retry(retry.checked().map_err(serde::de::Error::custom)?) + .map_err(serde::de::Error::custom)?; + } + Ok(value) + } +} + +/// Separately scoped, task-owned facts. Never merge these into a child's native +/// diagnostic identity. Optional final leaves are independent of terminal kind. +#[derive(Clone, Debug, Default, PartialEq, Eq, Serialize)] +pub struct FailureObservations { + #[serde(skip_serializing_if = "Option::is_none")] + receipt: Option, + #[serde(skip_serializing_if = "Option::is_none")] + retry: Option, + #[serde(skip_serializing_if = "Option::is_none")] + effects: Option, +} +impl FailureObservations { + /// Validate an already-parsed object without cloning arbitrary input trees. + pub fn from_value(value: &serde_json::Value) -> Result { + Self::deserialize(value).map_err(|_| FailureMetadataDecodeError) + } + pub fn receipt(&self) -> Option<&HostFatalReceipt> { + self.receipt.as_ref() + } + pub fn retry(&self) -> Option<&ProviderFailure> { + self.retry.as_ref() + } + pub fn effects(&self) -> Option<&PossibleEffects> { + self.effects.as_ref() + } + pub fn is_empty(&self) -> bool { + self.receipt.is_none() && self.retry.is_none() && self.effects.is_none() + } + pub fn with_receipt(mut self, value: HostFatalReceipt) -> Self { + self.receipt = Some(value); + self + } + pub fn with_effects(mut self, value: PossibleEffects) -> Self { + self.effects = Some(value); + self + } + pub fn with_retry( + mut self, + value: ProviderFailure, + ) -> Result { + FailureMetadataV1::default().with_retry(value)?; + self.retry = Some(value); + Ok(self) + } + pub fn from_slice(bytes: &[u8]) -> Result { + if bytes.len() > MAX_FAILURE_METADATA_BYTES { + return Err(FailureMetadataDecodeError); + } + serde_json::from_slice(bytes).map_err(|_| FailureMetadataDecodeError) + } +} +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +struct WireObservations { + receipt: Option, + retry: Option, + effects: Option, +} +impl<'de> Deserialize<'de> for FailureObservations { + fn deserialize>(d: D) -> Result { + let wire = WireObservations::deserialize(d) + .map_err(|_| serde::de::Error::custom(FailureMetadataDecodeError))?; + let mut value = Self { + receipt: wire.receipt, + effects: wire.effects, + retry: None, + }; + if let Some(retry) = wire.retry { + value = value + .with_retry(retry.checked().map_err(serde::de::Error::custom)?) + .map_err(serde::de::Error::custom)?; + } + Ok(value) + } +} diff --git a/crates/agentkit-core/src/lib.rs b/crates/agentkit-core/src/lib.rs index eed1ce8..45c0d14 100644 --- a/crates/agentkit-core/src/lib.rs +++ b/crates/agentkit-core/src/lib.rs @@ -32,6 +32,9 @@ //! assert_eq!(transcript[0].kind, ItemKind::System); //! ``` +pub mod failure; +pub mod retry; + use std::collections::BTreeMap; use std::fmt; use std::sync::Arc; diff --git a/crates/agentkit-core/src/retry.rs b/crates/agentkit-core/src/retry.rs new file mode 100644 index 0000000..67495c7 --- /dev/null +++ b/crates/agentkit-core/src/retry.rs @@ -0,0 +1,125 @@ +//! Sanitized, payload-free model retry observations. These are not effects provenance. + +use std::time::Duration; + +use serde::{Deserialize, Serialize}; + +/// Static provider route; never an endpoint URL or account identifier. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +#[non_exhaustive] +pub enum ProviderRoute { + #[default] + Unknown, + OpenAiResponses, + OpenAiChatGptResponses, +} + +/// Allowlisted provider type/code values. Unknown strings are never retained. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +#[non_exhaustive] +pub enum UpstreamErrorKind { + ServiceUnavailableError, + ServerIsOverloaded, + ServerError, + RateLimitError, + RateLimitExceeded, + TemporarilyUnavailable, + AuthenticationError, + InvalidApiKey, + InvalidAuthentication, + Unauthorized, + InvalidRequestError, + PermissionDenied, + InsufficientQuota, + ContentPolicyViolation, + #[default] + #[serde(other)] + Unknown, +} + +/// Sanitized source classification, kept separate from the local stopping reason. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] +pub struct ProviderClassification { + pub error_type: UpstreamErrorKind, + pub code: UpstreamErrorKind, + /// Source HTTP status, if present. No headers or response body are retained. + pub http_status: Option, +} + +/// Local reason for a failed attempt or logical request. +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +#[non_exhaustive] +pub enum ProviderFailureReason { + HttpStatus, + Transport, + ResponseFailed, + Protocol, + InvalidRequest, + Authentication, + AttemptTimeout, + IdleTimeout, + RetryExhausted, + RetryBudget, + RetryDisabled, + ReplayUnsafe, + Cancelled, +} + +/// Per-logical-request accounting, independent of policy retry count. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] +pub struct RetryAccounting { + /// Actual HTTP sends started, including a resend after authentication refresh. + /// Preflight/authentication failures can have zero attempts. + pub attempts: u64, + /// Sum of requested durations of fully completed backoff waits. Interrupted + /// waits contribute zero, even when they consumed wall-clock time. + pub completed_backoff: Duration, + /// Monotonic elapsed time since before initial authentication/preflight. + pub elapsed: Duration, +} + +/// A nonterminal snapshot emitted before a retry wait or reactive refresh. +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] +pub struct RetryProgress { + pub route: ProviderRoute, + pub reason: ProviderFailureReason, + pub upstream: ProviderClassification, + /// Attempts already started. The planned next send is `attempts + 1`, but + /// cancellation/preflight failure can prevent it from ever starting. + pub accounting: RetryAccounting, + pub next_delay: Duration, +} + +/// Payload-free terminal model failure. Display and Debug contain only typed data. +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize, thiserror::Error)] +#[error("provider request failed ({reason:?}; attempts: {attempts})", attempts = .accounting.attempts)] +pub struct ProviderFailure { + pub route: ProviderRoute, + pub reason: ProviderFailureReason, + /// Last failed request-attempt category, retained across local budget/limit stops. + /// None when no request attempt failed (for example initial authentication). + pub last_attempt_reason: Option, + pub upstream: ProviderClassification, + pub accounting: RetryAccounting, +} + +/// Observational lifecycle; never a second model result or a tool-effects record. +/// +/// Correlate through the enclosing `ObservedEvent.session_id` and current +/// `AgentEvent::TurnStarted`. Direct session consumers own that association. +/// Stable fatal event IDs belong to the host, not to this payload. +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[non_exhaustive] +pub enum ProviderRetryEvent { + Scheduled(RetryProgress), + /// Emitted once on explicit failure/cancellation, including zero-send failures. + Stopped(ProviderFailure), + /// Clears retry activity without introducing another successful model result. + Succeeded { + route: ProviderRoute, + accounting: RetryAccounting, + }, +} diff --git a/crates/agentkit-core/tests/failure.rs b/crates/agentkit-core/tests/failure.rs new file mode 100644 index 0000000..5f79578 --- /dev/null +++ b/crates/agentkit-core/tests/failure.rs @@ -0,0 +1,173 @@ +use agentkit_core::failure::*; +use agentkit_core::retry::*; +use serde_json::json; +use std::time::Duration; + +fn sample() -> FailureMetadataV1 { + let mut effects = PossibleEffects::default(); + effects.source = ObservationSource::AcpNotifications; + effects.tool_execution_start_reported = true; + FailureMetadataV1::new(FailureCode::ChildFailed) + .with_receipt(HostFatalReceipt { + session_id: HostReceiptId::new("session-1").unwrap(), + event_id: HostReceiptId::new("e-123_456").unwrap(), + storage: FatalStorage::Unavailable, + }) + .with_effects(effects) + .with_retry(ProviderFailure { + route: ProviderRoute::OpenAiResponses, + reason: ProviderFailureReason::RetryExhausted, + last_attempt_reason: Some(ProviderFailureReason::ResponseFailed), + upstream: ProviderClassification::default(), + accounting: RetryAccounting { + attempts: u64::MAX, + completed_backoff: Duration::MAX, + elapsed: Duration::MAX, + }, + }) + .unwrap() +} + +#[test] +fn current_writer_roundtrips_exact_bounded_values() { + let sample = sample(); + let bytes = serde_json::to_vec(&sample).unwrap(); + assert!(bytes.len() < MAX_FAILURE_METADATA_BYTES); + assert_eq!(FailureMetadataV1::from_slice(&bytes).unwrap(), sample); + let value = serde_json::to_value(sample).unwrap(); + assert_eq!(value["version"], 1); + assert_eq!( + value["retry"]["accounting"]["completed_backoff"]["nanos"], + 999_999_999 + ); + assert_eq!(value["receipt"]["storage"], "unavailable"); +} + +#[test] +fn absent_facts_are_unknown_and_effects_remain_incomplete() { + let sample = FailureMetadataV1::from_slice(br#"{"version":1}"#).unwrap(); + assert_eq!(sample, FailureMetadataV1::default()); + assert_eq!( + serde_json::to_value(sample).unwrap(), + json!({"version":1,"code":"unknown"}) + ); + let effects: PossibleEffects = serde_json::from_value(json!({})).unwrap(); + assert!(effects.observation_incomplete()); + assert!(!effects.tool_execution_start_reported); +} + +#[test] +fn identifiers_validate_without_echoing_rejected_input() { + for value in [ + "", + "../private", + "customer@example.com", + "line\nbreak", + "é", + &"x".repeat(129), + ] { + assert_eq!( + HostReceiptId::new(value).unwrap_err().to_string(), + "invalid failure metadata" + ); + assert!(serde_json::from_value::(json!(value)).is_err()); + } + assert!(HostReceiptId::new("x".repeat(128)).is_ok()); +} + +#[test] +fn every_new_nested_object_is_closed_and_errors_are_static() { + let original = serde_json::to_value(sample()).unwrap(); + for path in [ + "", + "/retry", + "/retry/upstream", + "/retry/accounting", + "/retry/accounting/elapsed", + "/receipt", + "/effects", + ] { + let mut malformed = original.clone(); + malformed + .pointer_mut(path) + .unwrap() + .as_object_mut() + .unwrap() + .insert("PRIVATE_TOKEN".into(), json!("PRIVATE_BODY")); + let error = + FailureMetadataV1::from_slice(&serde_json::to_vec(&malformed).unwrap()).unwrap_err(); + assert_eq!( + format!("{error:?}: {error}"), + "FailureMetadataDecodeError: invalid failure metadata" + ); + } +} + +#[test] +fn malformed_versions_enums_counts_and_complete_claims_are_rejected() { + for (path, bad) in [ + ("/version", json!(2)), + ("/code", json!("private-code")), + ("/retry/route", json!("https://private")), + ("/retry/upstream/http_status", json!(99)), + ("/retry/upstream/http_status", json!(600)), + ("/retry/accounting/attempts", json!(-1)), + ("/retry/accounting/attempts", json!(1.5)), + ("/retry/accounting/elapsed/nanos", json!(1_000_000_000)), + ("/effects/observation_incomplete", json!(false)), + ("/receipt/storage", json!("pending")), + ] { + let mut value = serde_json::to_value(sample()).unwrap(); + *value + .pointer_mut(path) + .unwrap_or_else(|| panic!("missing path {path}")) = bad; + assert!( + FailureMetadataV1::from_slice(&serde_json::to_vec(&value).unwrap()).is_err(), + "{path}" + ); + } +} + +#[test] +fn unknown_provider_spellings_normalize_without_retaining_private_strings() { + let mut value = serde_json::to_value(sample()).unwrap(); + value["retry"]["upstream"]["code"] = json!("PRIVATE_PROVIDER_TEXT"); + let decoded = FailureMetadataV1::from_slice(&serde_json::to_vec(&value).unwrap()).unwrap(); + assert_eq!( + decoded.retry().unwrap().upstream.code, + UpstreamErrorKind::Unknown + ); + assert!(!format!("{decoded:?}").contains("PRIVATE")); + assert!(!serde_json::to_string(&decoded).unwrap().contains("PRIVATE")); +} + +#[test] +fn oversized_deep_duplicate_and_mixed_unrecognized_inputs_fail_closed() { + for bytes in [ + vec![b' '; MAX_FAILURE_METADATA_BYTES + 1], + br#"{"version":1,"version":2}"#.to_vec(), + br#"{"version":1,"effects":{"source":"unknown","replay_safe":true}}"#.to_vec(), + format!("{}0{}", "[".repeat(200), "]".repeat(200)).into_bytes(), + ] { + assert!(FailureMetadataV1::from_slice(&bytes).is_err()); + } +} + +#[test] +fn standalone_legacy_retry_serde_stays_permissive() { + let mut retry = serde_json::to_value(sample().retry().unwrap()).unwrap(); + retry["future_field"] = json!(true); + assert!(serde_json::from_value::(retry).is_ok()); +} + +#[test] +fn direct_metadata_deserialize_errors_do_not_quote_rejected_fields_or_variants() { + for value in [ + json!({"version":1,"PRIVATE_FIELD":"PRIVATE_BODY"}), + json!({"version":1,"code":"PRIVATE_CODE"}), + json!({"version":1,"effects":{"source":"PRIVATE_SOURCE"}}), + ] { + let error = serde_json::from_value::(value).unwrap_err(); + assert!(!error.to_string().contains("PRIVATE")); + } +} diff --git a/crates/agentkit-integration-tests/tests/snapshots/approval_flow_deny.ron b/crates/agentkit-integration-tests/tests/snapshots/approval_flow_deny.ron index d147a3b..4b4c4b0 100644 --- a/crates/agentkit-integration-tests/tests/snapshots/approval_flow_deny.ron +++ b/crates/agentkit-integration-tests/tests/snapshots/approval_flow_deny.ron @@ -168,7 +168,11 @@ SessionRecording( call_id: ToolCallId("call-danger"), output: Text("policy says no"), is_error: true, - metadata: {}, + metadata: { + "agentkit.tool.failure": { + "kind": "execution_failed", + }, + }, )), ], metadata: {}, @@ -275,7 +279,11 @@ SessionRecording( call_id: ToolCallId("call-danger"), output: Text("policy says no"), is_error: true, - metadata: {}, + metadata: { + "agentkit.tool.failure": { + "kind": "execution_failed", + }, + }, )), ], metadata: {}, diff --git a/crates/agentkit-integration-tests/tests/snapshots/mcp_progress.ron b/crates/agentkit-integration-tests/tests/snapshots/mcp_progress.ron index e126c00..8cea650 100644 --- a/crates/agentkit-integration-tests/tests/snapshots/mcp_progress.ron +++ b/crates/agentkit-integration-tests/tests/snapshots/mcp_progress.ron @@ -174,6 +174,9 @@ SessionRecording( output: Text("tool not found: multiply"), is_error: true, metadata: { + "agentkit.tool.failure": { + "kind": "not_found", + }, "agentkit.tool.not_started": true, }, )), @@ -310,6 +313,9 @@ SessionRecording( output: Text("tool not found: multiply"), is_error: true, metadata: { + "agentkit.tool.failure": { + "kind": "not_found", + }, "agentkit.tool.not_started": true, }, )), diff --git a/crates/agentkit-integration-tests/tests/snapshots/permission_deny.ron b/crates/agentkit-integration-tests/tests/snapshots/permission_deny.ron index 48b4aa9..23b5a80 100644 --- a/crates/agentkit-integration-tests/tests/snapshots/permission_deny.ron +++ b/crates/agentkit-integration-tests/tests/snapshots/permission_deny.ron @@ -127,6 +127,9 @@ SessionRecording( output: Text("tool permission denied: PermissionDenial { code: CustomPolicyDenied, message: \"policy says no\", metadata: {} }"), is_error: true, metadata: { + "agentkit.tool.failure": { + "kind": "permission_denied", + }, "agentkit.tool.failure_kind": "permission_denied", "agentkit.tool.not_started": true, }, @@ -223,6 +226,9 @@ SessionRecording( output: Text("tool permission denied: PermissionDenial { code: CustomPolicyDenied, message: \"policy says no\", metadata: {} }"), is_error: true, metadata: { + "agentkit.tool.failure": { + "kind": "permission_denied", + }, "agentkit.tool.failure_kind": "permission_denied", "agentkit.tool.not_started": true, }, diff --git a/crates/agentkit-integration-tests/tests/snapshots/tool_execution_failed.ron b/crates/agentkit-integration-tests/tests/snapshots/tool_execution_failed.ron index 734ba7a..e3df4e2 100644 --- a/crates/agentkit-integration-tests/tests/snapshots/tool_execution_failed.ron +++ b/crates/agentkit-integration-tests/tests/snapshots/tool_execution_failed.ron @@ -126,7 +126,11 @@ SessionRecording( call_id: ToolCallId("call-fail"), output: Text("tool execution failed: kaboom"), is_error: true, - metadata: {}, + metadata: { + "agentkit.tool.failure": { + "kind": "execution_failed", + }, + }, )), ], metadata: {}, @@ -219,7 +223,11 @@ SessionRecording( call_id: ToolCallId("call-fail"), output: Text("tool execution failed: kaboom"), is_error: true, - metadata: {}, + metadata: { + "agentkit.tool.failure": { + "kind": "execution_failed", + }, + }, )), ], metadata: {}, diff --git a/crates/agentkit-loop/src/lib.rs b/crates/agentkit-loop/src/lib.rs index ec8aef7..b451056 100644 --- a/crates/agentkit-loop/src/lib.rs +++ b/crates/agentkit-loop/src/lib.rs @@ -1797,6 +1797,7 @@ where cancellation: cancellation.clone(), }; OwnedToolContext { + failure_observer: None, session_id, turn_id, metadata, @@ -2600,33 +2601,46 @@ where let outcome = match start { Ok(outcome) => outcome, Err(error) => { - self.append_tool_result_item(Item { - id: None, - kind: ItemKind::Tool, - parts: vec![Part::ToolResult(ToolResultPart { - call_id: pending.call.id.clone(), - output: ToolOutput::Text(format!( - "approved task failed to start: {error}" - )), - is_error: true, - metadata: pending.call.metadata.clone(), - })], - metadata: MetadataMap::new(), - usage: None, - finish_reason: None, - created_at: None, - }); - let turn_id = pending.tool_request.turn_id.clone(); - if let Err(cleanup_error) = - self.task_manager.on_turn_interrupted(&turn_id).await + if let Some(resolution) = self + .task_manager + .take_terminal_task_result(&pending.task_id) + .await + .map_err(|error| { + LoopError::Tool(ToolError::Internal(error.to_string())) + })? { - tracing::debug!( - %cleanup_error, - %turn_id, - "failed to clean up turn after approved task start error" - ); + TaskStartOutcome::Ready(Box::new(resolution)) + } else { + self.append_tool_result_item(Item { + id: None, + kind: ItemKind::Tool, + parts: vec![Part::ToolResult(ToolResultPart { + call_id: pending.call.id.clone(), + output: ToolOutput::Text(format!( + "approved task failed to start: {error}" + )), + is_error: true, + metadata: untrusted_call_metadata( + pending.call.metadata.clone(), + ), + })], + metadata: MetadataMap::new(), + usage: None, + finish_reason: None, + created_at: None, + }); + let turn_id = pending.tool_request.turn_id.clone(); + if let Err(cleanup_error) = + self.task_manager.on_turn_interrupted(&turn_id).await + { + tracing::debug!( + %cleanup_error, + %turn_id, + "failed to clean up turn after approved task start error" + ); + } + return Err(error); } - return Err(error); } }; match outcome { @@ -2665,22 +2679,47 @@ where } } ApprovalDecision::Deny { reason } => { - self.append_tool_result_item(Item { - id: None, - kind: ItemKind::Tool, - parts: vec![Part::ToolResult(ToolResultPart { - call_id: pending.call.id.clone(), - output: ToolOutput::Text( - reason.unwrap_or_else(|| "approval denied".into()), - ), - is_error: true, - metadata: pending.call.metadata.clone(), - })], - metadata: MetadataMap::new(), - usage: None, - finish_reason: None, - created_at: None, - }); + let reason = reason.unwrap_or_else(|| "approval denied".into()); + let selected = self + .task_manager + .close_suspended_task( + &pending.task_id, + &pending.request.id, + ToolError::ExecutionFailed(reason.clone()), + ) + .await + .map_err(|error| LoopError::Tool(ToolError::Internal(error.to_string())))?; + if let Some(TaskResolution::Item(mut item)) = selected { + // Keep the existing denial text while retaining typed facts. + for part in &mut item.parts { + if let Part::ToolResult(result) = part + && agentkit_task_manager::tool_failure_info(result) + .ok() + .flatten() + .is_some_and(|info| { + info.kind != agentkit_tools_core::ToolFailureKind::Cancelled + }) + { + result.output = ToolOutput::Text(reason.clone()); + } + } + self.append_tool_result_item(item); + } else { + self.append_tool_result_item(Item { + id: None, + kind: ItemKind::Tool, + parts: vec![Part::ToolResult(ToolResultPart { + call_id: pending.call.id.clone(), + output: ToolOutput::Text(reason), + is_error: true, + metadata: untrusted_call_metadata(pending.call.metadata.clone()), + })], + metadata: MetadataMap::new(), + usage: None, + finish_reason: None, + created_at: None, + }); + } } } @@ -2846,14 +2885,34 @@ where /// /// This clears the blocking approval and appends an error tool result so /// the transcript remains provider-valid if the host continues the turn. - pub fn cancel_pending_approval_for(&mut self, call_id: ToolCallId) -> Result<(), LoopError> { - let Some(pending) = self.drain_pending_approval_for(&call_id) else { + pub async fn cancel_pending_approval_for( + &mut self, + call_id: ToolCallId, + ) -> Result<(), LoopError> { + let Some(pending) = self.pending_approvals.get(&call_id).cloned() else { return Err(LoopError::InvalidState(format!( "no approval request is pending for call {}", call_id.0 ))); }; + let selected = self + .task_manager + .close_suspended_task(&pending.task_id, &pending.request.id, ToolError::Cancelled) + .await + .map_err(|error| LoopError::Tool(ToolError::Internal(error.to_string())))?; + let pending = self + .drain_pending_approval_for(&call_id) + .expect("pending approval retained across closure"); let turn_id = pending.presentation_turn_id.clone(); + if let Some(TaskResolution::Item(mut item)) = selected { + item.metadata.extend(interrupted_metadata("tool")); + for part in &mut item.parts { + if let Part::ToolResult(result) = part { + result.metadata.extend(interrupted_metadata("tool")); + } + } + self.append_tool_result_item(item); + } self.reject_drained_approvals(vec![pending]); if self.pending_approvals.is_empty() && self.active_tool_round.is_none() { let _ = self.finish_cancelled(turn_id, Vec::new())?; @@ -3205,10 +3264,62 @@ where pending } + fn collect_interrupted_task_updates(&mut self, extra_call_ids: &[ToolCallId]) { + let mut call_ids: Vec<_> = unanswered_tool_calls(&self.transcript) + .into_iter() + .map(|call| call.id) + .collect(); + call_ids.extend_from_slice(extra_call_ids); + for update in self + .task_manager + .take_interrupted_task_updates(&self.session_id, &call_ids) + { + match update { + TurnTaskUpdate::Detached(snapshot) => { + self.append_detach_placeholder(snapshot.call_id, &snapshot.tool_name) + } + TurnTaskUpdate::Resolution(resolution) => { + if let TaskResolution::Item(mut item) = *resolution { + let mut cancelled = false; + for part in &mut item.parts { + if let Part::ToolResult(result) = part + && agentkit_task_manager::tool_failure_info(result) + .ok() + .flatten() + .is_some_and(|failure| { + failure.kind + == agentkit_tools_core::ToolFailureKind::Cancelled + }) + { + result.metadata.extend(interrupted_metadata("tool")); + cancelled = true; + } + } + if cancelled { + item.metadata.extend(interrupted_metadata("tool")); + } + self.append_tool_result_item(item); + } + } + } + } + } + fn reject_drained_approvals(&mut self, pending: Vec) { + let call_ids: Vec<_> = pending + .iter() + .map(|pending| pending.call.id.clone()) + .collect(); + self.collect_interrupted_task_updates(&call_ids); for pending in pending { self.emit(AgentEvent::ApprovalResolved { approved: false }); - self.append_tool_result_item(cancelled_approval_item(pending)); + if self.background_call_ids.contains(&pending.call.id) + || unanswered_tool_calls(&self.transcript) + .iter() + .any(|call| call.id == pending.call.id) + { + self.append_tool_result_item(cancelled_approval_item(pending)); + } } } @@ -3229,6 +3340,7 @@ where /// are converted to notifications; calls that never started or were /// cancelled in the foreground are not retained as detached work. fn close_interrupted_tool_calls(&mut self) { + self.collect_interrupted_task_updates(&[]); for call in unanswered_tool_calls(&self.transcript) { let call_id = call.id.clone(); let completes_in_background = self.background_call_ids.contains(&call_id); @@ -4191,6 +4303,19 @@ fn interrupted_tool_result_item(call: ToolCallPart) -> Item { } } +// Synthesized fallback results have no host-observed diagnostic facts. +fn untrusted_call_metadata(mut metadata: MetadataMap) -> MetadataMap { + for key in [ + agentkit_task_manager::TOOL_RESULT_FAILURE_METADATA_KEY, + agentkit_task_manager::TOOL_RESULT_FAILURE_OBSERVATIONS_METADATA_KEY, + agentkit_task_manager::TOOL_RESULT_FAILURE_KIND_METADATA_KEY, + agentkit_task_manager::TOOL_RESULT_NOT_STARTED_METADATA_KEY, + ] { + metadata.remove(key); + } + metadata +} + fn cancelled_approval_item(pending: PendingApprovalToolCall) -> Item { Item { id: None, @@ -4199,7 +4324,7 @@ fn cancelled_approval_item(pending: PendingApprovalToolCall) -> Item { call_id: pending.call.id, output: ToolOutput::Text("approval cancelled".into()), is_error: true, - metadata: pending.call.metadata, + metadata: untrusted_call_metadata(pending.call.metadata), })], metadata: MetadataMap::new(), usage: None, @@ -4372,6 +4497,7 @@ fn validate_transcript_invariants(transcript: &[Item]) -> Result<(), LoopError> #[cfg(test)] mod tests { + mod failure_transport_tests; use std::collections::VecDeque; use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; use std::sync::{Arc as StdArc, Mutex as StdMutex}; @@ -4538,6 +4664,33 @@ mod tests { self.inner.on_turn_interrupted(turn_id).await } + fn take_interrupted_task_updates( + &self, + session_id: &SessionId, + call_ids: &[ToolCallId], + ) -> Vec { + self.inner + .take_interrupted_task_updates(session_id, call_ids) + } + + async fn take_terminal_task_result( + &self, + task_id: &TaskId, + ) -> Result, TaskManagerError> { + self.inner.take_terminal_task_result(task_id).await + } + + async fn close_suspended_task( + &self, + task_id: &TaskId, + approval_id: &agentkit_core::ApprovalId, + error: ToolError, + ) -> Result, TaskManagerError> { + self.inner + .close_suspended_task(task_id, approval_id, error) + .await + } + fn handle(&self) -> TaskManagerHandle { self.inner.handle() } @@ -5703,7 +5856,9 @@ mod tests { async fn wait_until_completed(handle: &TaskManagerHandle) { timeout(Duration::from_secs(1), async { - while handle.list_completed().await.is_empty() { + while handle.list_completed().await.is_empty() + && handle.list_suspended().await.is_empty() + { tokio::task::yield_now().await; } }) @@ -7942,7 +8097,7 @@ mod tests { } other => panic!("unexpected loop step: {other:?}"), }; - driver.cancel_pending_approval_for(call_id).unwrap(); + driver.cancel_pending_approval_for(call_id).await.unwrap(); assert!(driver.lifecycle.active_turn.is_none()); assert!(driver.pending_approvals.is_empty()); diff --git a/crates/agentkit-loop/src/retry.rs b/crates/agentkit-loop/src/retry.rs index d92a55a..aed0d88 100644 --- a/crates/agentkit-loop/src/retry.rs +++ b/crates/agentkit-loop/src/retry.rs @@ -1,128 +1,6 @@ -//! Sanitized, payload-free model retry observations. These are not effects provenance. +//! Loop-owned observer for canonical core retry values. -use std::time::Duration; - -use serde::{Deserialize, Serialize}; - -/// Static provider route; never an endpoint URL or account identifier. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] -#[serde(rename_all = "snake_case")] -#[non_exhaustive] -pub enum ProviderRoute { - #[default] - Unknown, - OpenAiResponses, - OpenAiChatGptResponses, -} - -/// Allowlisted provider type/code values. Unknown strings are never retained. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] -#[serde(rename_all = "snake_case")] -#[non_exhaustive] -pub enum UpstreamErrorKind { - ServiceUnavailableError, - ServerIsOverloaded, - ServerError, - RateLimitError, - RateLimitExceeded, - TemporarilyUnavailable, - AuthenticationError, - InvalidApiKey, - InvalidAuthentication, - Unauthorized, - InvalidRequestError, - PermissionDenied, - InsufficientQuota, - ContentPolicyViolation, - #[default] - #[serde(other)] - Unknown, -} - -/// Sanitized source classification, kept separate from the local stopping reason. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] -pub struct ProviderClassification { - pub error_type: UpstreamErrorKind, - pub code: UpstreamErrorKind, - /// Source HTTP status, if present. No headers or response body are retained. - pub http_status: Option, -} - -/// Local reason for a failed attempt or logical request. -#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] -#[serde(rename_all = "snake_case")] -#[non_exhaustive] -pub enum ProviderFailureReason { - HttpStatus, - Transport, - ResponseFailed, - Protocol, - InvalidRequest, - Authentication, - AttemptTimeout, - IdleTimeout, - RetryExhausted, - RetryBudget, - RetryDisabled, - ReplayUnsafe, - Cancelled, -} - -/// Per-logical-request accounting, independent of policy retry count. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] -pub struct RetryAccounting { - /// Actual HTTP sends started, including a resend after authentication refresh. - /// Preflight/authentication failures can have zero attempts. - pub attempts: u64, - /// Sum of requested durations of fully completed backoff waits. Interrupted - /// waits contribute zero, even when they consumed wall-clock time. - pub completed_backoff: Duration, - /// Monotonic elapsed time since before initial authentication/preflight. - pub elapsed: Duration, -} - -/// A nonterminal snapshot emitted before a retry wait or reactive refresh. -#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] -pub struct RetryProgress { - pub route: ProviderRoute, - pub reason: ProviderFailureReason, - pub upstream: ProviderClassification, - /// Attempts already started. The planned next send is `attempts + 1`, but - /// cancellation/preflight failure can prevent it from ever starting. - pub accounting: RetryAccounting, - pub next_delay: Duration, -} - -/// Payload-free terminal model failure. Display and Debug contain only typed data. -#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize, thiserror::Error)] -#[error("provider request failed ({reason:?}; attempts: {attempts})", attempts = .accounting.attempts)] -pub struct ProviderFailure { - pub route: ProviderRoute, - pub reason: ProviderFailureReason, - /// Last failed request-attempt category, retained across local budget/limit stops. - /// None when no request attempt failed (for example initial authentication). - pub last_attempt_reason: Option, - pub upstream: ProviderClassification, - pub accounting: RetryAccounting, -} - -/// Observational lifecycle; never a second model result or a tool-effects record. -/// -/// Correlate through the enclosing `ObservedEvent.session_id` and current -/// `AgentEvent::TurnStarted`. Direct session consumers own that association. -/// Stable fatal event IDs belong to the host, not to this payload. -#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] -#[non_exhaustive] -pub enum ProviderRetryEvent { - Scheduled(RetryProgress), - /// Emitted once on explicit failure/cancellation, including zero-send failures. - Stopped(ProviderFailure), - /// Clears retry activity without introducing another successful model result. - Succeeded { - route: ProviderRoute, - accounting: RetryAccounting, - }, -} +pub use agentkit_core::retry::*; /// Synchronous, queue-free observer installed before `begin_turn`. /// @@ -145,6 +23,18 @@ impl RetryObserver for F { mod tests { use super::*; use crate::AgentEvent; + use std::time::Duration; + + #[test] + fn core_and_loop_retry_values_have_identical_types() { + let accounting: agentkit_core::retry::RetryAccounting = RetryAccounting::default(); + let _: crate::RetryAccounting = accounting; + let event: agentkit_core::retry::ProviderRetryEvent = ProviderRetryEvent::Succeeded { + route: ProviderRoute::Unknown, + accounting, + }; + let _: crate::ProviderRetryEvent = event; + } #[test] fn event_roundtrip_and_legacy_shape_remain_compatible() { diff --git a/crates/agentkit-loop/src/tests/failure_transport_tests.rs b/crates/agentkit-loop/src/tests/failure_transport_tests.rs new file mode 100644 index 0000000..ed6bd59 --- /dev/null +++ b/crates/agentkit-loop/src/tests/failure_transport_tests.rs @@ -0,0 +1,456 @@ +use super::*; +use agentkit_core::TurnId; +use agentkit_core::failure::{ + FailureCode, FailureMetadataV1, FatalStorage, HostFatalReceipt, HostReceiptId, + ObservationSource, PossibleEffects, +}; +use agentkit_tools_core::{DiagnosticFailureKind, DiagnosticToolFailure}; + +struct WinnerExecutor { + mode: &'static str, + entered: StdArc, +} +#[async_trait] +impl ToolExecutor for WinnerExecutor { + fn specs(&self) -> Vec { + vec![ToolSpec::new( + "echo", + "controlled terminal", + json!({"type":"object"}), + )] + } + async fn execute( + &self, + request: ToolRequest, + ctx: &mut ToolContext<'_>, + ) -> ToolExecutionOutcome { + let publisher = ctx.failure_observer().unwrap(); + let mut effects = PossibleEffects::default(); + effects.source = ObservationSource::LocalSession; + effects.tool_execution_start_reported = true; + publisher.publish_effects(effects).unwrap(); + publisher + .publish_receipt(HostFatalReceipt { + session_id: HostReceiptId::new("local-session").unwrap(), + event_id: HostReceiptId::new("local-event").unwrap(), + storage: FatalStorage::Unavailable, + }) + .unwrap(); + publisher + .publish_retry(ProviderFailure { + route: ProviderRoute::Unknown, + reason: ProviderFailureReason::Cancelled, + last_attempt_reason: None, + upstream: ProviderClassification::default(), + accounting: RetryAccounting::default(), + }) + .unwrap(); + self.entered.store(true, Ordering::SeqCst); + if self.mode == "handle_cancel" { + std::future::pending::<()>().await; + } + if self.mode == "approval" { + return ToolExecutionOutcome::Interrupted( + agentkit_tools_core::ToolInterruption::ApprovalRequired(ApprovalRequest { + task_id: None, + call_id: Some(request.call_id), + id: "approval:winner".into(), + request_kind: "tool.test".into(), + reason: agentkit_tools_core::ApprovalReason::SensitivePath, + summary: "approve".into(), + metadata: MetadataMap::new(), + }), + ); + } + if self.mode == "success" { + return ToolExecutionOutcome::Completed(ToolResult::new(ToolResultPart::success( + request.call_id, + ToolOutput::Text("selected success".into()), + ))); + } + let mut child_effects = PossibleEffects::default(); + child_effects.source = ObservationSource::AcpNotifications; + child_effects.assistant_output_observed = true; + ToolExecutionOutcome::Failed(ToolError::diagnostic(DiagnosticToolFailure { + kind: if self.mode == "native_cancel" { + DiagnosticFailureKind::Cancelled + } else { + DiagnosticFailureKind::ExecutionFailed + }, + metadata: FailureMetadataV1::new(FailureCode::ChildFailed).with_effects(child_effects), + })) + } +} + +/// This wrapper does not return from start_task until a real terminal event is +/// enqueued, then cancels the turn before LoopDriver can call wait_for_turn. +struct CancelAfterSelection { + inner: AsyncTaskManager, + controller: CancellationController, + selected: StdArc>>, + entered: StdArc, + handle_cancel: bool, +} +#[async_trait] +impl TaskManager for CancelAfterSelection { + async fn start_task( + &self, + request: TaskLaunchRequest, + ctx: TaskStartContext, + ) -> Result { + let outcome = self.inner.start_task(request, ctx).await?; + let handle = self.inner.handle(); + let TaskEvent::Started(snapshot) = wait_for_task_event(&handle).await else { + panic!() + }; + if self.handle_cancel { + wait_until_entered(&self.entered).await; + handle.cancel(snapshot.id).await?; + } + *self.selected.lock().unwrap() = Some(wait_for_task_event(&handle).await); + self.controller.interrupt(); + Ok(outcome) + } + async fn wait_for_turn( + &self, + turn: &TurnId, + cancellation: Option, + ) -> Result, TaskManagerError> { + self.inner.wait_for_turn(turn, cancellation).await + } + async fn take_pending_loop_updates(&self) -> Result { + self.inner.take_pending_loop_updates().await + } + async fn on_turn_interrupted(&self, turn: &TurnId) -> Result<(), TaskManagerError> { + self.inner.on_turn_interrupted(turn).await + } + fn take_interrupted_task_updates( + &self, + session: &SessionId, + calls: &[ToolCallId], + ) -> Vec { + self.inner.take_interrupted_task_updates(session, calls) + } + fn handle(&self) -> TaskManagerHandle { + self.inner.handle() + } +} + +#[tokio::test] +async fn real_loop_turn_cancel_keeps_already_selected_success_failure_and_cancellation() { + for mode in ["success", "failed", "native_cancel", "handle_cancel"] { + let controller = CancellationController::new(); + let selected = StdArc::new(StdMutex::new(None)); + let events = StdArc::new(StdMutex::new(Vec::new())); + let entered = StdArc::new(AtomicBool::new(false)); + let agent = Agent::builder() + .model(FakeAdapter) + .tool_executor(WinnerExecutor { + mode, + entered: entered.clone(), + }) + .task_manager(CancelAfterSelection { + inner: AsyncTaskManager::new(), + controller: controller.clone(), + selected: selected.clone(), + entered, + handle_cancel: mode == "handle_cancel", + }) + .cancellation(controller.handle()) + .observer(RecordingObserver { + events: events.clone(), + }) + .build() + .unwrap(); + let mut driver = agent + .start(SessionConfig::new("winner-session")) + .await + .unwrap(); + driver + .submit_input(vec![Item::text(ItemKind::User, "ping")]) + .unwrap(); + assert!( + matches!(run_until_finished(&mut driver).await, LoopStep::Finished(turn) if turn.finish_reason == FinishReason::Cancelled) + ); + let snapshot = driver.snapshot(); + validate_transcript_invariants(&snapshot.transcript).unwrap(); + let results: Vec<_> = snapshot + .transcript + .iter() + .flat_map(|item| &item.parts) + .filter_map(|part| match part { + Part::ToolResult(result) => Some(result), + _ => None, + }) + .collect(); + assert_eq!(results.len(), 1, "{mode}"); + let event = selected.lock().unwrap().clone().unwrap(); + let task = match event { + TaskEvent::Completed(task, selected) => { + assert_eq!(mode, "success"); + assert_eq!(results[0], &selected); + task + } + TaskEvent::Failed(task, _) => { + assert_eq!(mode, "failed"); + task + } + TaskEvent::Cancelled(task) => { + assert!(mode == "native_cancel" || mode == "handle_cancel"); + task + } + _ => panic!("not terminal"), + }; + assert_eq!( + agentkit_task_manager::tool_failure_info(results[0]).unwrap(), + task.failure + ); + assert_eq!( + agentkit_task_manager::task_failure_observations(results[0]).unwrap(), + task.failure_observations + ); + if mode != "success" { + let observations = task.failure_observations.unwrap(); + assert!(observations.receipt().is_some()); + assert!(observations.retry().is_some()); + assert!( + observations + .effects() + .unwrap() + .tool_execution_start_reported + ); + } + assert_eq!( + events + .lock() + .unwrap() + .iter() + .filter(|event| matches!(event, AgentEvent::ToolResultReceived(_))) + .count(), + 1 + ); + assert!( + driver + .task_manager + .take_interrupted_task_updates(&snapshot.session_id, &[ToolCallId::new("call-1")]) + .is_empty() + ); + } +} + +#[tokio::test] +async fn real_loop_pending_approval_cancel_prefers_frozen_facts_over_synthetic_result() { + let controller = CancellationController::new(); + let events = StdArc::new(StdMutex::new(Vec::new())); + let manager = AsyncTaskManager::new(); + let handle = manager.handle(); + let agent = Agent::builder() + .model(FakeAdapter) + .tool_executor(WinnerExecutor { + mode: "approval", + entered: StdArc::new(AtomicBool::new(false)), + }) + .task_manager(manager) + .cancellation(controller.handle()) + .observer(RecordingObserver { + events: events.clone(), + }) + .build() + .unwrap(); + let mut driver = agent + .start(SessionConfig::new("approval-session")) + .await + .unwrap(); + driver + .submit_input(vec![Item::text(ItemKind::User, "ping")]) + .unwrap(); + assert!(matches!( + driver.next().await.unwrap(), + LoopStep::Interrupt(LoopInterrupt::ApprovalRequest(_)) + )); + assert_eq!(handle.list_suspended().await.len(), 1); + controller.interrupt(); + assert!( + matches!(driver.next().await.unwrap(), LoopStep::Finished(turn) if turn.finish_reason == FinishReason::Cancelled) + ); + let transcript = driver.snapshot().transcript; + validate_transcript_invariants(&transcript).unwrap(); + let results: Vec<_> = transcript + .iter() + .flat_map(|item| &item.parts) + .filter_map(|part| match part { + Part::ToolResult(result) => Some(result), + _ => None, + }) + .collect(); + assert_eq!(results.len(), 1); + assert!( + agentkit_task_manager::task_failure_observations(results[0]) + .unwrap() + .unwrap() + .effects() + .unwrap() + .tool_execution_start_reported + ); + assert_eq!( + agentkit_task_manager::tool_failure_info(results[0]) + .unwrap() + .unwrap() + .kind, + agentkit_tools_core::ToolFailureKind::Cancelled + ); + assert_eq!( + events + .lock() + .unwrap() + .iter() + .filter(|event| matches!(event, AgentEvent::ToolResultReceived(_))) + .count(), + 1 + ); +} + +#[tokio::test] +async fn individual_approval_closure_terminalizes_manager_and_preserves_facts() { + for inline in [false, true] { + for mode in ["cancel", "deny", "cancel_won", "approve_after_cancel"] { + let manager: Arc = if inline { + Arc::new(agentkit_task_manager::SimpleTaskManager::new()) + } else { + Arc::new(AsyncTaskManager::new()) + }; + let handle = manager.handle(); + let mut builder = Agent::builder() + .model(FakeAdapter) + .tool_executor(WinnerExecutor { + mode: "approval", + entered: StdArc::new(AtomicBool::new(false)), + }); + builder.task_manager = Some(manager); + let agent = builder.build().unwrap(); + let mut driver = agent + .start(SessionConfig::new("closure-session")) + .await + .unwrap(); + driver + .submit_input(vec![Item::text(ItemKind::User, "ping")]) + .unwrap(); + let LoopStep::Interrupt(LoopInterrupt::ApprovalRequest(_)) = + driver.next().await.unwrap() + else { + panic!() + }; + let suspended = handle.list_suspended().await; + assert_eq!(suspended.len(), 1); + assert!( + driver + .task_manager + .take_terminal_task_result(&suspended[0].id) + .await + .unwrap() + .is_none() + ); + assert_eq!(handle.list_suspended().await.len(), 1); + + if mode == "cancel_won" || mode == "approve_after_cancel" { + handle.cancel(suspended[0].id.clone()).await.unwrap(); + } + if mode == "cancel" { + driver + .cancel_pending_approval_for("call-1".into()) + .await + .unwrap(); + } else { + driver + .resolve_approval_for( + "call-1".into(), + if mode == "approve_after_cancel" { + ApprovalDecision::Approve + } else { + ApprovalDecision::Deny { + reason: Some("denied".into()), + } + }, + ) + .unwrap(); + driver.next().await.unwrap(); + } + assert!(handle.list_suspended().await.is_empty()); + assert_eq!(handle.list_completed().await.len(), 1); + assert!( + driver + .task_manager + .take_terminal_task_result(&suspended[0].id) + .await + .unwrap() + .is_none() + ); + + let transcript = driver.snapshot().transcript; + validate_transcript_invariants(&transcript).unwrap(); + let results: Vec<_> = transcript + .iter() + .flat_map(|item| &item.parts) + .filter_map(|part| match part { + Part::ToolResult(result) => Some(result), + _ => None, + }) + .collect(); + assert!(matches!( + wait_for_task_event(&handle).await, + TaskEvent::Started(_) + )); + let terminal = wait_for_task_event(&handle).await; + let task = match &terminal { + TaskEvent::Cancelled(task) | TaskEvent::Failed(task, _) => task, + _ => panic!("expected terminal"), + }; + assert_eq!( + task.failure_observations, + agentkit_task_manager::task_failure_observations(results[0]).unwrap() + ); + assert!( + tokio::time::timeout(std::time::Duration::from_millis(10), handle.next_event()) + .await + .is_err() + ); + assert_eq!(results.len(), 1); + assert!( + agentkit_task_manager::task_failure_observations(results[0]) + .unwrap() + .is_some() + ); + assert_eq!( + agentkit_task_manager::tool_failure_info(results[0]) + .unwrap() + .unwrap() + .kind, + if mode == "deny" { + agentkit_tools_core::ToolFailureKind::ExecutionFailed + } else { + agentkit_tools_core::ToolFailureKind::Cancelled + } + ); + } + } +} + +#[test] +fn synthetic_approval_metadata_rejects_reserved_request_facts() { + let input = [ + ( + agentkit_task_manager::TOOL_RESULT_FAILURE_METADATA_KEY.into(), + json!({"kind":"cancelled"}), + ), + ( + agentkit_task_manager::TOOL_RESULT_FAILURE_OBSERVATIONS_METADATA_KEY.into(), + json!({"receipt":"forged"}), + ), + ("application".into(), json!(1)), + ] + .into(); + assert_eq!( + untrusted_call_metadata(input), + [("application".into(), json!(1))].into() + ); +} diff --git a/crates/agentkit-mcp/tests/in_memory.rs b/crates/agentkit-mcp/tests/in_memory.rs index d9af832..60d05b6 100644 --- a/crates/agentkit-mcp/tests/in_memory.rs +++ b/crates/agentkit-mcp/tests/in_memory.rs @@ -385,6 +385,7 @@ async fn tool_adapter_propagates_call_through_running_service() { let metadata = MetadataMap::new(); let mut ctx = ToolContext { + failure_observer: None, capability: CapabilityContext { session_id: None, turn_id: None, @@ -431,6 +432,7 @@ async fn tool_adapter_error_responder_receives_typed_invocation_error() { let adapter = McpToolAdapter::new(&server_id, connection.clone(), descriptor); let metadata = MetadataMap::new(); let mut ctx = ToolContext { + failure_observer: None, capability: CapabilityContext { session_id: None, turn_id: None, diff --git a/crates/agentkit-task-manager/Cargo.toml b/crates/agentkit-task-manager/Cargo.toml index 198826a..5b23440 100644 --- a/crates/agentkit-task-manager/Cargo.toml +++ b/crates/agentkit-task-manager/Cargo.toml @@ -16,5 +16,4 @@ async-trait.workspace = true thiserror.workspace = true tokio = { workspace = true, features = ["sync", "time"] } -[dev-dependencies] serde_json.workspace = true diff --git a/crates/agentkit-task-manager/src/lib.rs b/crates/agentkit-task-manager/src/lib.rs index 5705032..fcd2cd7 100644 --- a/crates/agentkit-task-manager/src/lib.rs +++ b/crates/agentkit-task-manager/src/lib.rs @@ -14,6 +14,86 @@ use thiserror::Error; use tokio::sync::{Mutex, Notify, mpsc, oneshot}; use tokio::task::JoinHandle; +/// Host-owned typed terminal facts; never accepted from request metadata. +pub const TOOL_RESULT_FAILURE_OBSERVATIONS_METADATA_KEY: &str = + "agentkit.tool.failure_observations"; + +pub const TOOL_RESULT_FAILURE_METADATA_KEY: &str = "agentkit.tool.failure"; + +fn strip_failure_metadata(metadata: &mut MetadataMap) { + for key in [ + TOOL_RESULT_FAILURE_OBSERVATIONS_METADATA_KEY, + TOOL_RESULT_FAILURE_METADATA_KEY, + TOOL_RESULT_FAILURE_KIND_METADATA_KEY, + TOOL_RESULT_NOT_STARTED_METADATA_KEY, + ] { + metadata.remove(key); + } +} + +/// Read the closed typed projection. Stored JSON still requires host provenance +/// validation at external transport boundaries; this helper validates shape only. +pub fn tool_failure_info( + result: &ToolResultPart, +) -> Result< + Option, + agentkit_core::failure::FailureMetadataDecodeError, +> { + result + .metadata + .get(TOOL_RESULT_FAILURE_METADATA_KEY) + .map(agentkit_tools_core::ToolFailureInfo::from_value) + .transpose() +} + +pub fn task_failure_observations( + result: &ToolResultPart, +) -> Result< + Option, + agentkit_core::failure::FailureMetadataDecodeError, +> { + result + .metadata + .get(TOOL_RESULT_FAILURE_OBSERVATIONS_METADATA_KEY) + .map(agentkit_core::failure::FailureObservations::from_value) + .transpose() +} + +fn attach_observations( + resolution: &mut TaskResolution, + observations: Option<&agentkit_core::failure::FailureObservations>, +) { + if let Some(observations) = observations + && let TaskResolution::Item(item) = resolution + { + for part in &mut item.parts { + if let agentkit_core::Part::ToolResult(result) = part { + result.metadata.insert( + TOOL_RESULT_FAILURE_OBSERVATIONS_METADATA_KEY.into(), + serde_json::to_value(observations).expect("finite task observations"), + ); + } + } + } +} + +fn write_failure_metadata(metadata: &mut MetadataMap, error: &ToolError, not_started: bool) { + strip_failure_metadata(metadata); + metadata.insert( + TOOL_RESULT_FAILURE_METADATA_KEY.into(), + serde_json::to_value(error.failure_info()).expect("finite failure facts"), + ); + if error.is_permission_denied() { + metadata.insert( + TOOL_RESULT_FAILURE_KIND_METADATA_KEY.into(), + TOOL_RESULT_FAILURE_KIND_PERMISSION_DENIED.into(), + ); + } + if not_started { + metadata.insert(TOOL_RESULT_NOT_STARTED_METADATA_KEY.into(), true.into()); + } +} + pub const TOOL_RESULT_FAILURE_KIND_METADATA_KEY: &str = "agentkit.tool.failure_kind"; pub const TOOL_RESULT_FAILURE_KIND_PERMISSION_DENIED: &str = "permission_denied"; /// Marks a synthetic error result whose tool never began executing (failed @@ -48,6 +128,10 @@ pub struct TaskSnapshot { pub tool_name: String, pub kind: TaskKind, pub metadata: MetadataMap, + /// Selected terminal facts; cancellation may have no observed diagnostics. + pub failure: Option, + /// Frozen task-owned scope, deliberately separate from native child facts. + pub failure_observations: Option, } #[derive(Clone, Debug, PartialEq)] @@ -82,7 +166,7 @@ pub enum TaskStartOutcome { #[derive(Clone, Debug, PartialEq)] pub enum TurnTaskUpdate { Resolution(Box), - Detached(TaskSnapshot), + Detached(Box), } #[derive(Clone, Debug, Default, PartialEq)] @@ -146,6 +230,10 @@ pub enum TaskManagerError { NotRunning(TaskId), #[error("task is already running in the background: {0}")] AlreadyBackground(TaskId), + #[error("task is already running: {0}")] + AlreadyRunning(TaskId), + #[error("invalid task continuation: {0}")] + InvalidContinuation(TaskId), #[error("task manager internal error: {0}")] Internal(String), } @@ -204,6 +292,39 @@ pub trait TaskManager: Send + Sync { async fn on_turn_interrupted(&self, turn_id: &TurnId) -> Result<(), TaskManagerError>; + /// Drain the real frozen foreground cancellation results before a loop + /// synthesizes results for remaining unanswered calls. Custom managers that + /// delegate interruption must also delegate this method. + fn take_interrupted_task_updates( + &self, + _session_id: &agentkit_core::SessionId, + _call_ids: &[ToolCallId], + ) -> Vec { + Vec::new() + } + + /// Transfer an already-selected terminal result for a failed continuation. + /// This never closes a live or suspended task. Retained-task wrappers must + /// delegate it so approval cannot replace a winning cancellation. + async fn take_terminal_task_result( + &self, + _task_id: &TaskId, + ) -> Result, TaskManagerError> { + Ok(None) + } + + /// Terminalize an approval already delivered to the caller and transfer its + /// frozen result directly, without enqueuing a second delivery. Wrappers + /// must delegate this method alongside interruption and its scoped drain. + async fn close_suspended_task( + &self, + _task_id: &TaskId, + _approval_id: &agentkit_core::ApprovalId, + _error: ToolError, + ) -> Result, TaskManagerError> { + Ok(None) + } + fn handle(&self) -> TaskManagerHandle; } @@ -214,6 +335,7 @@ trait TaskManagerControl: Send + Sync { async fn detach(&self, task_id: TaskId) -> Result<(), TaskManagerError>; async fn list_running(&self) -> Vec; async fn list_completed(&self) -> Vec; + async fn list_suspended(&self) -> Vec; async fn drain_ready_items(&self) -> Vec; async fn set_continue_policy( &self, @@ -275,6 +397,11 @@ impl TaskManagerHandle { self.inner.set_delivery_mode(task_id, mode).await } + /// Invocations awaiting approval are suspended, not terminally completed. + pub async fn list_suspended(&self) -> Vec { + self.inner.list_suspended().await + } + /// Wait until all running tasks have completed. pub async fn wait_for_idle(&self) { self.inner.wait_for_idle().await @@ -306,25 +433,171 @@ impl TaskManager for SimpleTaskManager { request: TaskLaunchRequest, ctx: TaskStartContext, ) -> Result { + let mut request = request; + strip_failure_metadata(&mut request.request.metadata); + let mut ctx = ctx; + strip_failure_metadata(&mut ctx.tool_context.metadata); let task_id = request .task_id .clone() .unwrap_or_else(|| self.state.next_task_id()); - let outcome = match &request.kind { - TaskLaunchKind::Approved(approved) => { - ctx.executor - .execute_approved_owned(request.request.clone(), approved, ctx.tool_context) - .await + let generation = Arc::new(()); + let cancel_signal = Arc::new(Notify::new()); + let observations; + { + let mut tasks = self + .state + .tasks + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + observations = if let Some(record) = tasks.get(&task_id) { + if record.running { + return Err(TaskManagerError::AlreadyRunning(task_id)); + } + let Some(saved) = &record.suspended else { + return Err(TaskManagerError::InvalidContinuation(task_id)); + }; + if !continuation_matches(saved, &request) { + return Err(TaskManagerError::InvalidContinuation(task_id)); + } + observation_slot(record.snapshot.failure_observations.as_ref()) + } else { + if matches!(&request.kind, TaskLaunchKind::Approved(_)) { + return Err(TaskManagerError::InvalidContinuation(task_id)); + } + observation_slot(None) + }; + let snapshot = TaskSnapshot { + id: task_id.clone(), + turn_id: request.request.turn_id.clone(), + call_id: request.request.call_id.clone(), + tool_name: request.request.tool_name.to_string(), + kind: TaskKind::Foreground, + metadata: request.request.metadata.clone(), + failure: None, + failure_observations: None, + }; + tasks.insert( + task_id.clone(), + TaskRecord { + cancel_signal: Some(cancel_signal.clone()), + generation: generation.clone(), + session_id: request.request.session_id.clone(), + suspended: None, + observations: observations.clone(), + snapshot: snapshot.clone(), + continue_policy: ContinuePolicy::NotifyOnly, + delivery_mode: DeliveryMode::ToLoop, + running: true, + invocation_admitted: true, + completed: false, + join: None, + }, + ); + let _ = self.state.events_tx.send(TaskEvent::Started(snapshot)); + } + // Registered synchronously before the first await. Dropping the inline + // future seals all producer clones and retains a cancellation result. + let mut owner = InlineOwner { + state: self.state.clone(), + task_id: task_id.clone(), + generation: generation.clone(), + armed: true, + }; + ctx.tool_context.failure_observer = Some(observations.publisher()); + let invoke = async { + match &request.kind { + TaskLaunchKind::Approved(approval) => { + ctx.executor + .execute_approved_owned(request.request.clone(), approval, ctx.tool_context) + .await + } + TaskLaunchKind::Plain => { + ctx.executor + .execute_owned(request.request.clone(), ctx.tool_context) + .await + } } - TaskLaunchKind::Plain => { - ctx.executor - .execute_owned(request.request.clone(), ctx.tool_context) - .await + }; + let outcome = tokio::select! { + outcome = invoke => outcome, + _ = cancel_signal.notified() => ToolExecutionOutcome::Failed(ToolError::Cancelled), + }; + let mut tasks = self + .state + .tasks + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let record = tasks + .get_mut(&task_id) + .expect("inline owner retains registered record"); + if !Arc::ptr_eq(&record.generation, &generation) { + return Err(TaskManagerError::InvalidContinuation(task_id)); + } + if !record.running { + let resolution = cancellation_resolution(&record.snapshot); + let _ = drain_interrupted_updates( + &self.state.interrupted_updates, + &record.session_id, + std::slice::from_ref(&record.snapshot.call_id), + ); + owner.armed = false; + return Ok(TaskStartOutcome::Ready(Box::new(resolution))); + } + let not_started = matches!(&outcome, ToolExecutionOutcome::FailedBeforeInvocation(_)); + let error = match &outcome { + ToolExecutionOutcome::Failed(error) + | ToolExecutionOutcome::FailedBeforeInvocation(error) => Some(error.clone()), + _ => None, + }; + let mut resolution = map_outcome_to_resolution(Some(task_id), request.request, outcome); + record.running = false; + record.completed = !matches!(&resolution, TaskResolution::Approval(_)); + record.suspended = match &resolution { + TaskResolution::Approval(task) => { + Some(SuspendedTask::new(&task.tool_request, &task.approval)) } + _ => None, + }; + record.snapshot.failure = error.as_ref().map(ToolError::failure_info); + if not_started { + record + .snapshot + .metadata + .insert(TOOL_RESULT_NOT_STARTED_METADATA_KEY.into(), true.into()); + } + let frozen = observations.seal(); + record.snapshot.failure_observations = if error.is_some() || record.suspended.is_some() { + frozen + } else { + None }; - Ok(TaskStartOutcome::Ready(Box::new( - map_outcome_to_resolution(Some(task_id), request.request, outcome), - ))) + if error.is_some() { + attach_observations( + &mut resolution, + record.snapshot.failure_observations.as_ref(), + ); + } + if let Some(error) = error { + let event = if error.is_cancelled() { + TaskEvent::Cancelled(record.snapshot.clone()) + } else { + TaskEvent::Failed(record.snapshot.clone(), error) + }; + let _ = self.state.events_tx.send(event); + } else if let TaskResolution::Item(item) = &resolution { + for part in &item.parts { + if let agentkit_core::Part::ToolResult(result) = part { + let _ = self.state.events_tx.send(TaskEvent::Completed( + record.snapshot.clone(), + result.clone(), + )); + } + } + } + owner.armed = false; + self.state.notify.notify_waiters(); + Ok(TaskStartOutcome::Ready(Box::new(resolution))) } async fn wait_for_turn( @@ -339,10 +612,83 @@ impl TaskManager for SimpleTaskManager { Ok(PendingLoopUpdates::default()) } - async fn on_turn_interrupted(&self, _turn_id: &TurnId) -> Result<(), TaskManagerError> { + async fn on_turn_interrupted(&self, turn_id: &TurnId) -> Result<(), TaskManagerError> { + let ids: Vec<_> = self + .state + .tasks + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .values() + .filter(|record| { + record.snapshot.turn_id == *turn_id + && (record.running || record.suspended.is_some()) + }) + .map(|record| record.snapshot.id.clone()) + .collect(); + for id in ids { + self.state.cancel_inline(&id, None)?; + } Ok(()) } + fn take_interrupted_task_updates( + &self, + session_id: &agentkit_core::SessionId, + call_ids: &[ToolCallId], + ) -> Vec { + drain_interrupted_updates(&self.state.interrupted_updates, session_id, call_ids) + } + + async fn take_terminal_task_result( + &self, + task_id: &TaskId, + ) -> Result, TaskManagerError> { + let tasks = self + .state + .tasks + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + if tasks.get(task_id).is_some_and(|record| record.completed) { + Ok(take_interrupted_resolution( + &self.state.interrupted_updates, + task_id, + )) + } else { + Ok(None) + } + } + + async fn close_suspended_task( + &self, + task_id: &TaskId, + approval_id: &agentkit_core::ApprovalId, + error: ToolError, + ) -> Result, TaskManagerError> { + let mut tasks = self + .state + .tasks + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let record = tasks + .get_mut(task_id) + .ok_or_else(|| TaskManagerError::NotFound(task_id.clone()))?; + if record.completed { + return Ok(take_interrupted_resolution( + &self.state.interrupted_updates, + task_id, + )); + } + let result = close_suspended_record(record, approval_id, &error)?; + if result.is_some() { + let _ = self + .state + .events_tx + .send(failure_event(record.snapshot.clone(), error)); + self.state.notify.notify_waiters(); + } + Ok(result) + } + fn handle(&self) -> TaskManagerHandle { TaskManagerHandle { inner: self.state.clone(), @@ -350,13 +696,86 @@ impl TaskManager for SimpleTaskManager { } } -#[derive(Default)] struct HandleState { next_task_index: AtomicU64, events_rx: Mutex>>, + events_tx: mpsc::UnboundedSender, + tasks: std::sync::Mutex>, + interrupted_updates: std::sync::Mutex>, + notify: Notify, +} +impl Default for HandleState { + fn default() -> Self { + let (events_tx, events_rx) = mpsc::unbounded_channel(); + Self { + next_task_index: AtomicU64::new(0), + events_rx: Mutex::new(Some(events_rx)), + events_tx, + tasks: std::sync::Mutex::new(BTreeMap::new()), + interrupted_updates: std::sync::Mutex::new(Vec::new()), + notify: Notify::new(), + } + } +} +struct InlineOwner { + state: Arc, + task_id: TaskId, + generation: Arc<()>, + armed: bool, +} +impl Drop for InlineOwner { + fn drop(&mut self) { + if self.armed { + let _ = self + .state + .cancel_inline(&self.task_id, Some(&self.generation)); + } + } } impl HandleState { + fn cancel_inline( + &self, + task_id: &TaskId, + generation: Option<&Arc<()>>, + ) -> Result<(), TaskManagerError> { + let mut tasks = self + .tasks + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let record = tasks + .get_mut(task_id) + .ok_or_else(|| TaskManagerError::NotFound(task_id.clone()))?; + if generation.is_some_and(|generation| !Arc::ptr_eq(generation, &record.generation)) + || (!record.running && record.suspended.is_none()) + { + return Ok(()); + } + record.running = false; + record.completed = true; + record.suspended = None; + record.snapshot.failure = Some(ToolError::Cancelled.failure_info()); + record.snapshot.failure_observations = record.observations.seal(); + self.interrupted_updates + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .push(InterruptedUpdate { + task_id: task_id.clone(), + session_id: record.session_id.clone(), + update: TurnTaskUpdate::Resolution(Box::new(cancellation_resolution( + &record.snapshot, + ))), + }); + let _ = self + .events_tx + .send(TaskEvent::Cancelled(record.snapshot.clone())); + if let Some(signal) = &record.cancel_signal { + signal.notify_one(); + } + self.notify.notify_waiters(); + Ok(()) + } + fn next_task_id(&self) -> TaskId { let next = self.next_task_index.fetch_add(1, Ordering::SeqCst) + 1; TaskId::new(format!("task-{}", next)) @@ -374,7 +793,7 @@ impl TaskManagerControl for HandleState { } async fn cancel(&self, task_id: TaskId) -> Result<(), TaskManagerError> { - Err(TaskManagerError::NotFound(task_id)) + self.cancel_inline(&task_id, None) } async fn detach(&self, task_id: TaskId) -> Result<(), TaskManagerError> { @@ -382,11 +801,33 @@ impl TaskManagerControl for HandleState { } async fn list_running(&self) -> Vec { - Vec::new() + self.tasks + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .values() + .filter(|record| record.running) + .map(|record| record.snapshot.clone()) + .collect() } async fn list_completed(&self) -> Vec { - Vec::new() + self.tasks + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .values() + .filter(|record| record.completed) + .map(|record| record.snapshot.clone()) + .collect() + } + + async fn list_suspended(&self) -> Vec { + self.tasks + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .values() + .filter(|record| record.suspended.is_some()) + .map(|record| record.snapshot.clone()) + .collect() } async fn drain_ready_items(&self) -> Vec { @@ -409,7 +850,23 @@ impl TaskManagerControl for HandleState { Err(TaskManagerError::NotFound(task_id)) } - async fn wait_for_idle(&self) {} + async fn wait_for_idle(&self) { + loop { + let notified = self.notify.notified(); + tokio::pin!(notified); + notified.as_mut().enable(); + if !self + .tasks + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .values() + .any(|record| record.running) + { + return; + } + notified.await; + } + } } pub struct AsyncTaskManager { @@ -423,6 +880,7 @@ impl AsyncTaskManager { Self { inner: Arc::new(AsyncInner { state: Mutex::new(AsyncState::default()), + interrupted_updates: std::sync::Mutex::new(Vec::new()), host_event_tx: event_tx, host_event_rx: Mutex::new(event_rx), notify: Notify::new(), @@ -448,21 +906,140 @@ struct AsyncState { next_task_index: u64, tasks: BTreeMap, per_turn_running: BTreeMap, - per_turn_updates: BTreeMap>, - pending_loop_updates: VecDeque, + per_turn_updates: BTreeMap>, + pending_loop_updates: VecDeque<(TaskId, TaskResolution)>, manual_ready_items: Vec, } +#[derive(Clone)] +struct SuspendedTask { + session_id: agentkit_core::SessionId, + turn_id: TurnId, + call_id: ToolCallId, + tool_name: agentkit_tools_core::ToolName, + approval_id: agentkit_core::ApprovalId, +} +impl SuspendedTask { + fn new(request: &ToolRequest, approval: &ApprovalRequest) -> Self { + Self { + session_id: request.session_id.clone(), + turn_id: request.turn_id.clone(), + call_id: request.call_id.clone(), + tool_name: request.tool_name.clone(), + approval_id: approval.id.clone(), + } + } +} +fn continuation_matches(saved: &SuspendedTask, launch: &TaskLaunchRequest) -> bool { + // Approval explicitly supports host-patched input. Correlate immutable + // logical-call identity, not arguments or caller metadata. + matches!(&launch.kind, TaskLaunchKind::Approved(approval) if approval.id == saved.approval_id) + && launch.request.session_id == saved.session_id + && launch.request.turn_id == saved.turn_id + && launch.request.call_id == saved.call_id + && launch.request.tool_name == saved.tool_name +} + +fn observation_slot( + previous: Option<&agentkit_core::failure::FailureObservations>, +) -> agentkit_tools_core::FailureObservationSlot { + let slot = agentkit_tools_core::FailureObservationSlot::new(); + if let Some(previous) = previous { + let publisher = slot.publisher(); + if let Some(value) = previous.effects() { + publisher + .publish_effects(*value) + .expect("validated continuation effects"); + } + if let Some(value) = previous.receipt() { + publisher + .publish_receipt(value.clone()) + .expect("validated continuation receipt"); + } + if let Some(value) = previous.retry() { + publisher + .publish_retry(*value) + .expect("validated continuation retry"); + } + } + slot +} + struct TaskRecord { + cancel_signal: Option>, + suspended: Option, + session_id: agentkit_core::SessionId, + generation: Arc<()>, + observations: agentkit_tools_core::FailureObservationSlot, snapshot: TaskSnapshot, continue_policy: ContinuePolicy, delivery_mode: DeliveryMode, running: bool, + invocation_admitted: bool, completed: bool, join: Option>, } +struct InterruptedUpdate { + task_id: TaskId, + session_id: agentkit_core::SessionId, + update: TurnTaskUpdate, +} + +fn update_call_id(update: &TurnTaskUpdate) -> Option<&ToolCallId> { + match update { + TurnTaskUpdate::Detached(snapshot) => Some(&snapshot.call_id), + TurnTaskUpdate::Resolution(resolution) => match resolution.as_ref() { + TaskResolution::Approval(task) => Some(&task.tool_request.call_id), + TaskResolution::Item(item) => item.parts.iter().find_map(|part| match part { + agentkit_core::Part::ToolResult(result) => Some(&result.call_id), + _ => None, + }), + }, + } +} + +fn take_interrupted_resolution( + queue: &std::sync::Mutex>, + task_id: &TaskId, +) -> Option { + let mut queue = queue + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let index = queue.iter().position(|entry| { + &entry.task_id == task_id && matches!(&entry.update, TurnTaskUpdate::Resolution(_)) + })?; + match queue.remove(index).update { + TurnTaskUpdate::Resolution(resolution) => Some(*resolution), + _ => unreachable!(), + } +} + +fn drain_interrupted_updates( + queue: &std::sync::Mutex>, + session_id: &agentkit_core::SessionId, + call_ids: &[ToolCallId], +) -> Vec { + let mut queue = queue + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let mut selected = Vec::new(); + let mut retained = Vec::new(); + for entry in std::mem::take(&mut *queue) { + if &entry.session_id == session_id + && update_call_id(&entry.update).is_some_and(|id| call_ids.contains(id)) + { + selected.push(entry.update); + } else { + retained.push(entry); + } + } + *queue = retained; + selected +} + struct AsyncInner { + interrupted_updates: std::sync::Mutex>, state: Mutex, host_event_tx: mpsc::UnboundedSender, host_event_rx: Mutex>, @@ -470,27 +1047,71 @@ struct AsyncInner { } impl AsyncInner { + fn take_terminal_result( + &self, + state: &mut AsyncState, + task_id: &TaskId, + ) -> Result, TaskManagerError> { + if !state + .tasks + .get(task_id) + .is_some_and(|record| record.completed) + { + return Ok(None); + } + let turn_id = state.tasks[task_id].snapshot.turn_id.clone(); + if let Some(queue) = state.per_turn_updates.get_mut(&turn_id) + && let Some(index) = queue.iter().position(|entry| { + &entry.task_id == task_id && matches!(&entry.update, TurnTaskUpdate::Resolution(_)) + }) + && let Some(entry) = queue.remove(index) + && let TurnTaskUpdate::Resolution(resolution) = entry.update + { + return Ok(Some(*resolution)); + } + if let Some(index) = state + .pending_loop_updates + .iter() + .position(|(id, _)| id == task_id) + { + return Ok(state + .pending_loop_updates + .remove(index) + .map(|(_, resolution)| resolution)); + } + Ok(take_interrupted_resolution( + &self.interrupted_updates, + task_id, + )) + } + async fn next_task_id(&self) -> TaskId { let mut state = self.state.lock().await; state.next_task_index += 1; TaskId::new(format!("task-{}", state.next_task_index)) } - async fn detach_running_foreground(&self, task_id: &TaskId) -> Result<(), TaskManagerError> { + async fn detach_running_foreground( + &self, + task_id: &TaskId, + generation: Option<&Arc<()>>, + ) -> Result<(), TaskManagerError> { let mut state = self.state.lock().await; - let snapshot = { + let (session_id, snapshot) = { let record = state .tasks .get_mut(task_id) .ok_or_else(|| TaskManagerError::NotFound(task_id.clone()))?; - if !record.running { + if !record.running + || generation.is_some_and(|generation| !Arc::ptr_eq(generation, &record.generation)) + { return Err(TaskManagerError::NotRunning(task_id.clone())); } if record.snapshot.kind == TaskKind::Background { return Err(TaskManagerError::AlreadyBackground(task_id.clone())); } record.snapshot.kind = TaskKind::Background; - record.snapshot.clone() + (record.session_id.clone(), record.snapshot.clone()) }; if let Some(count) = state.per_turn_running.get_mut(&snapshot.turn_id) { @@ -503,7 +1124,11 @@ impl AsyncInner { .per_turn_updates .entry(snapshot.turn_id.clone()) .or_default() - .push_back(TurnTaskUpdate::Detached(snapshot.clone())); + .push_back(InterruptedUpdate { + task_id: task_id.clone(), + session_id, + update: TurnTaskUpdate::Detached(Box::new(snapshot.clone())), + }); let _ = self.host_event_tx.send(TaskEvent::Detached(snapshot)); self.notify.notify_waiters(); Ok(()) @@ -511,27 +1136,87 @@ impl AsyncInner { async fn interrupt_turn(&self, turn_id: &TurnId) { let mut state = self.state.lock().await; + // Transfer already-selected foreground winners before cancelling the + // remaining tasks. A turn cancellation must not replace a queued result. + if let Some(queued) = state.per_turn_updates.remove(turn_id) { + let mut closed = self + .interrupted_updates + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + closed.extend(queued.into_iter().filter(|entry| !matches!(&entry.update, TurnTaskUpdate::Resolution(resolution) if matches!(resolution.as_ref(), TaskResolution::Approval(_))))); + } let interrupted: Vec = state .tasks .iter() .filter_map(|(id, record)| { (record.snapshot.turn_id == *turn_id - && record.snapshot.kind == TaskKind::Foreground - && record.running) - .then_some(id.clone()) + && ((record.snapshot.kind == TaskKind::Foreground && record.running) + || record.suspended.is_some())) + .then_some(id.clone()) }) .collect(); + // An approval still in this queue has not been handed to LoopDriver. + // Its background terminal result must retain its normal destination. + let unsurfaced: Vec<_> = state + .pending_loop_updates + .iter() + .filter_map(|(_, resolution)| match resolution { + TaskResolution::Approval(task) => Some(task.task_id.clone()), + _ => None, + }) + .collect(); + state.pending_loop_updates.retain(|(_, resolution)| !matches!(resolution, TaskResolution::Approval(task) if interrupted.contains(&task.task_id))); + let mut aborts = Vec::new(); for task_id in interrupted { if let Some(record) = state.tasks.get_mut(&task_id) { record.running = false; + record.suspended = None; + record.completed = true; + if !record.invocation_admitted { + record + .snapshot + .metadata + .insert(TOOL_RESULT_NOT_STARTED_METADATA_KEY.into(), true.into()); + } + record.snapshot.failure = Some(ToolError::Cancelled.failure_info()); + record.snapshot.failure_observations = record.observations.seal(); if let Some(join) = record.join.take() { - join.abort(); + aborts.push(join); } let snapshot = record.snapshot.clone(); + let session_id = record.session_id.clone(); + let delivery_mode = record.delivery_mode; + let continue_policy = record.continue_policy; + let resolution = cancellation_resolution(&snapshot); + if snapshot.kind == TaskKind::Background && delivery_mode == DeliveryMode::Manual { + if let TaskResolution::Item(item) = resolution { + state.manual_ready_items.push(item); + } + } else if snapshot.kind == TaskKind::Background && unsurfaced.contains(&task_id) { + state + .pending_loop_updates + .push_back((task_id.clone(), resolution)); + if continue_policy == ContinuePolicy::RequestContinue { + let _ = self.host_event_tx.send(TaskEvent::ContinueRequested); + } + } else { + self.interrupted_updates + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .push(InterruptedUpdate { + task_id: task_id.clone(), + session_id, + update: TurnTaskUpdate::Resolution(Box::new(resolution)), + }); + } let _ = self.host_event_tx.send(TaskEvent::Cancelled(snapshot)); } } state.per_turn_running.remove(turn_id); + drop(state); + for join in aborts { + join.abort(); + } self.notify.notify_waiters(); } } @@ -543,6 +1228,8 @@ impl TaskManager for AsyncTaskManager { request: TaskLaunchRequest, ctx: TaskStartContext, ) -> Result { + let mut request = request; + strip_failure_metadata(&mut request.request.metadata); let route = self.routing.route(&request.request); let task_id = match request.task_id.clone() { Some(existing) => existing, @@ -559,15 +1246,51 @@ impl TaskManager for AsyncTaskManager { tool_name: request.request.tool_name.to_string(), kind: initial_kind, metadata: request.request.metadata.clone(), + failure: None, + failure_observations: None, }; let mut state = self.inner.state.lock().await; + let (observations, continue_policy, delivery_mode) = + if let Some(record) = state.tasks.get(&task_id) { + if record.running { + return Err(TaskManagerError::AlreadyRunning(task_id)); + } + let Some(saved) = &record.suspended else { + return Err(TaskManagerError::InvalidContinuation(task_id)); + }; + if !continuation_matches(saved, &request) { + return Err(TaskManagerError::InvalidContinuation(task_id)); + } + ( + observation_slot(record.snapshot.failure_observations.as_ref()), + record.continue_policy, + record.delivery_mode, + ) + } else { + if matches!(&request.kind, TaskLaunchKind::Approved(_)) { + return Err(TaskManagerError::InvalidContinuation(task_id)); + } + ( + observation_slot(None), + ContinuePolicy::NotifyOnly, + DeliveryMode::ToLoop, + ) + }; + let initial_kind = snapshot.kind; + let generation = Arc::new(()); state.tasks.insert( task_id.clone(), TaskRecord { + cancel_signal: None, + generation: generation.clone(), + session_id: request.request.session_id.clone(), + suspended: None, + observations: observations.clone(), snapshot: snapshot.clone(), - continue_policy: ContinuePolicy::NotifyOnly, - delivery_mode: DeliveryMode::ToLoop, + continue_policy, + delivery_mode, running: true, + invocation_admitted: false, completed: false, join: None, }, @@ -578,7 +1301,8 @@ impl TaskManager for AsyncTaskManager { .entry(snapshot.turn_id.clone()) .or_default() += 1; } - drop(state); + // Registration, Started publication, and worker ownership have no + // cancellation point between them. let _ = self .inner .host_event_tx @@ -590,20 +1314,42 @@ impl TaskManager for AsyncTaskManager { let turn_id = snapshot.turn_id.clone(); let kind = request.kind.clone(); let exec_request = request.request.clone(); - let owned_ctx = ctx.tool_context.clone(); + let mut owned_ctx = ctx.tool_context.clone(); + owned_ctx.failure_observer = Some(observations.publisher()); + strip_failure_metadata(&mut owned_ctx.metadata); + let generation_for_future = generation.clone(); let executor = ctx.executor.clone(); - let route_copy = route; + let route_copy = if initial_kind == TaskKind::Background { + RoutingDecision::Background + } else { + route + }; let (start_tx, start_rx) = oneshot::channel(); let join = tokio::spawn(async move { if start_rx.await.is_err() { return; } + { + let mut state = inner.state.lock().await; + let Some(record) = state.tasks.get_mut(&task_id_for_future) else { + return; + }; + if !record.running || !Arc::ptr_eq(&record.generation, &generation_for_future) { + return; + } + // Admission is not an effects observation. False does prove the + // executor has not been entered when cancellation wins early. + record.invocation_admitted = true; + } if let RoutingDecision::ForegroundThenDetachAfter(duration) = route_copy { let inner = inner.clone(); let task_id = task_id_for_future.clone(); + let generation = generation_for_future.clone(); tokio::spawn(async move { tokio::time::sleep(duration).await; - let _ = inner.detach_running_foreground(&task_id).await; + let _ = inner + .detach_running_foreground(&task_id, Some(&generation)) + .await; }); } @@ -620,7 +1366,13 @@ impl TaskManager for AsyncTaskManager { } }; - let resolution = + let not_started = matches!(&outcome, ToolExecutionOutcome::FailedBeforeInvocation(_)); + let terminal_error = match &outcome { + ToolExecutionOutcome::Failed(error) + | ToolExecutionOutcome::FailedBeforeInvocation(error) => Some(error.clone()), + _ => None, + }; + let mut resolution = map_outcome_to_resolution(Some(task_id_for_future.clone()), exec_request, outcome); let completed_result = match &resolution { TaskResolution::Item(item) => item.parts.iter().find_map(|part| match part { @@ -630,17 +1382,46 @@ impl TaskManager for AsyncTaskManager { TaskResolution::Approval(_) => None, }; - let (snapshot, should_request_continue) = { + { let mut state = inner.state.lock().await; let Some(record) = state.tasks.get_mut(&task_id_for_future) else { return; }; + // Cancellation and completion compete for this single transition. + if !record.running || !Arc::ptr_eq(&record.generation, &generation_for_future) { + return; + } record.running = false; - record.completed = true; + record.completed = !matches!(&resolution, TaskResolution::Approval(_)); + record.suspended = match &resolution { + TaskResolution::Approval(task) => { + Some(SuspendedTask::new(&task.tool_request, &task.approval)) + } + _ => None, + }; + record.snapshot.failure = terminal_error.as_ref().map(ToolError::failure_info); + if not_started { + record + .snapshot + .metadata + .insert(TOOL_RESULT_NOT_STARTED_METADATA_KEY.into(), true.into()); + } + let frozen = record.observations.seal(); + record.snapshot.failure_observations = + if terminal_error.is_some() || record.suspended.is_some() { + frozen + } else { + None + }; + attach_observations( + &mut resolution, + record.snapshot.failure_observations.as_ref(), + ); let snapshot = record.snapshot.clone(); let continue_policy = record.continue_policy; let delivery_mode = record.delivery_mode; let current_kind = snapshot.kind; + let session_id = record.session_id.clone(); if current_kind == TaskKind::Foreground { if let Some(count) = state.per_turn_running.get_mut(&turn_id) { @@ -653,14 +1434,22 @@ impl TaskManager for AsyncTaskManager { .per_turn_updates .entry(turn_id.clone()) .or_default() - .push_back(TurnTaskUpdate::Resolution(Box::new(resolution.clone()))); + .push_back(InterruptedUpdate { + task_id: task_id_for_future.clone(), + session_id, + update: TurnTaskUpdate::Resolution(Box::new(resolution.clone())), + }); } else { match &resolution { TaskResolution::Item(_) if delivery_mode == DeliveryMode::ToLoop => { - state.pending_loop_updates.push_back(resolution.clone()); + state + .pending_loop_updates + .push_back((task_id_for_future.clone(), resolution.clone())); } TaskResolution::Approval(_) if delivery_mode == DeliveryMode::ToLoop => { - state.pending_loop_updates.push_back(resolution.clone()); + state + .pending_loop_updates + .push_back((task_id_for_future.clone(), resolution.clone())); } TaskResolution::Item(item) => { state.manual_ready_items.push(item.clone()); @@ -669,27 +1458,34 @@ impl TaskManager for AsyncTaskManager { } } - ( - snapshot, - current_kind == TaskKind::Background - && delivery_mode == DeliveryMode::ToLoop - && continue_policy == ContinuePolicy::RequestContinue, - ) - }; - - if let Some(result) = completed_result { - let _ = event_tx.send(TaskEvent::Completed(snapshot.clone(), result)); - } - if should_request_continue { - let _ = event_tx.send(TaskEvent::ContinueRequested); + // Enqueue immutable lifecycle events in the same transaction; + // no observer callbacks run while the task lock is held. + if let Some(error) = terminal_error { + let event = if error.is_cancelled() { + TaskEvent::Cancelled(snapshot.clone()) + } else { + TaskEvent::Failed(snapshot.clone(), error) + }; + let _ = event_tx.send(event); + } else if let Some(result) = completed_result { + let _ = event_tx.send(TaskEvent::Completed(snapshot.clone(), result)); + } + if current_kind == TaskKind::Background + && delivery_mode == DeliveryMode::ToLoop + && continue_policy == ContinuePolicy::RequestContinue + { + let _ = event_tx.send(TaskEvent::ContinueRequested); + } } inner.notify.notify_waiters(); }); - let mut state = self.inner.state.lock().await; + // Still holding the registration lock: dropping start_task at an await + // cannot strand a registered task without its worker handle. let mut join = Some(join); if let Some(record) = state.tasks.get_mut(&task_id) && record.running + && Arc::ptr_eq(&record.generation, &generation) { record.join = join.take(); } @@ -719,7 +1515,7 @@ impl TaskManager for AsyncTaskManager { if let Some(queue) = state.per_turn_updates.get_mut(turn_id) && let Some(update) = queue.pop_front() { - return Ok(Some(update)); + return Ok(Some(update.update)); } if state .per_turn_running @@ -757,7 +1553,11 @@ impl TaskManager for AsyncTaskManager { async fn take_pending_loop_updates(&self) -> Result { let mut state = self.inner.state.lock().await; Ok(PendingLoopUpdates { - resolutions: std::mem::take(&mut state.pending_loop_updates), + resolutions: state + .pending_loop_updates + .drain(..) + .map(|(_, resolution)| resolution) + .collect(), }) } @@ -783,6 +1583,52 @@ impl TaskManager for AsyncTaskManager { Ok(()) } + fn take_interrupted_task_updates( + &self, + session_id: &agentkit_core::SessionId, + call_ids: &[ToolCallId], + ) -> Vec { + drain_interrupted_updates(&self.inner.interrupted_updates, session_id, call_ids) + } + + async fn take_terminal_task_result( + &self, + task_id: &TaskId, + ) -> Result, TaskManagerError> { + let mut state = self.inner.state.lock().await; + self.inner.take_terminal_result(&mut state, task_id) + } + + async fn close_suspended_task( + &self, + task_id: &TaskId, + approval_id: &agentkit_core::ApprovalId, + error: ToolError, + ) -> Result, TaskManagerError> { + let mut state = self.inner.state.lock().await; + let record = state + .tasks + .get_mut(task_id) + .ok_or_else(|| TaskManagerError::NotFound(task_id.clone()))?; + if record.completed { + return self.inner.take_terminal_result(&mut state, task_id); + } + let result = close_suspended_record(record, approval_id, &error)?; + if result.is_some() { + let snapshot = record.snapshot.clone(); + if let Some(queue) = state.per_turn_updates.get_mut(&snapshot.turn_id) { + queue.retain(|entry| !matches!(&entry.update, TurnTaskUpdate::Resolution(resolution) if matches!(resolution.as_ref(), TaskResolution::Approval(task) if &task.task_id == task_id))); + } + state.pending_loop_updates.retain(|(_, resolution)| !matches!(resolution, TaskResolution::Approval(task) if &task.task_id == task_id)); + let _ = self + .inner + .host_event_tx + .send(failure_event(snapshot, error)); + self.inner.notify.notify_waiters(); + } + Ok(result) + } + fn handle(&self) -> TaskManagerHandle { TaskManagerHandle { inner: self.inner.clone(), @@ -802,12 +1648,28 @@ impl TaskManagerControl for AsyncInner { .tasks .get_mut(&task_id) .ok_or_else(|| TaskManagerError::NotFound(task_id.clone()))?; - if let Some(join) = record.join.take() { - join.abort(); + if !record.running && record.suspended.is_none() { + return Ok(()); } + let session_id = record.session_id.clone(); + let was_running = record.running; + record.suspended = None; + let join = record.join.take(); record.running = false; + if !record.invocation_admitted { + record + .snapshot + .metadata + .insert(TOOL_RESULT_NOT_STARTED_METADATA_KEY.into(), true.into()); + } + record.completed = true; + record.snapshot.failure = Some(ToolError::Cancelled.failure_info()); + record.snapshot.failure_observations = record.observations.seal(); let snapshot = record.snapshot.clone(); - if record.snapshot.kind == TaskKind::Foreground + let delivery_mode = record.delivery_mode; + let continue_policy = record.continue_policy; + if was_running + && record.snapshot.kind == TaskKind::Foreground && let Some(count) = state.per_turn_running.get_mut(&snapshot.turn_id) { *count = count.saturating_sub(1); @@ -815,13 +1677,45 @@ impl TaskManagerControl for AsyncInner { state.per_turn_running.remove(&snapshot.turn_id); } } + if let Some(queue) = state.per_turn_updates.get_mut(&snapshot.turn_id) { + queue.retain(|entry| !matches!(&entry.update, TurnTaskUpdate::Resolution(resolution) if matches!(resolution.as_ref(), TaskResolution::Approval(task) if task.task_id == task_id))); + } + state.pending_loop_updates.retain(|(_, resolution)| !matches!(resolution, TaskResolution::Approval(task) if task.task_id == task_id)); + let resolution = cancellation_resolution(&snapshot); + if snapshot.kind == TaskKind::Foreground { + state + .per_turn_updates + .entry(snapshot.turn_id.clone()) + .or_default() + .push_back(InterruptedUpdate { + task_id: task_id.clone(), + session_id, + update: TurnTaskUpdate::Resolution(Box::new(resolution)), + }); + } else if delivery_mode == DeliveryMode::ToLoop { + state + .pending_loop_updates + .push_back((task_id.clone(), resolution)); + } else if let TaskResolution::Item(item) = resolution { + state.manual_ready_items.push(item); + } + let request_continue = snapshot.kind == TaskKind::Background + && delivery_mode == DeliveryMode::ToLoop + && continue_policy == ContinuePolicy::RequestContinue; let _ = self.host_event_tx.send(TaskEvent::Cancelled(snapshot)); + if request_continue { + let _ = self.host_event_tx.send(TaskEvent::ContinueRequested); + } + drop(state); + if let Some(join) = join { + join.abort(); + } self.notify.notify_waiters(); Ok(()) } async fn detach(&self, task_id: TaskId) -> Result<(), TaskManagerError> { - self.detach_running_foreground(&task_id).await + self.detach_running_foreground(&task_id, None).await } async fn list_running(&self) -> Vec { @@ -844,6 +1738,17 @@ impl TaskManagerControl for AsyncInner { .collect() } + async fn list_suspended(&self) -> Vec { + self.state + .lock() + .await + .tasks + .values() + .filter(|record| record.suspended.is_some()) + .map(|record| record.snapshot.clone()) + .collect() + } + async fn drain_ready_items(&self) -> Vec { let mut state = self.state.lock().await; std::mem::take(&mut state.manual_ready_items) @@ -890,21 +1795,83 @@ impl TaskManagerControl for AsyncInner { } } +fn close_suspended_record( + record: &mut TaskRecord, + approval_id: &agentkit_core::ApprovalId, + error: &ToolError, +) -> Result, TaskManagerError> { + let Some(saved) = &record.suspended else { + return Ok(None); + }; + if &saved.approval_id != approval_id { + return Err(TaskManagerError::InvalidContinuation( + record.snapshot.id.clone(), + )); + } + record.suspended = None; + record.completed = true; + record.snapshot.failure = Some(error.failure_info()); + record.snapshot.failure_observations = record.observations.seal(); + Ok(Some(failure_resolution(&record.snapshot, error.clone()))) +} + +fn failure_event(snapshot: TaskSnapshot, error: ToolError) -> TaskEvent { + if error.is_cancelled() { + TaskEvent::Cancelled(snapshot) + } else { + TaskEvent::Failed(snapshot, error) + } +} + +fn cancellation_resolution(snapshot: &TaskSnapshot) -> TaskResolution { + failure_resolution(snapshot, ToolError::Cancelled) +} + +fn failure_resolution(snapshot: &TaskSnapshot, error: ToolError) -> TaskResolution { + let request = ToolRequest { + call_id: snapshot.call_id.clone(), + tool_name: snapshot.tool_name.as_str().into(), + input: serde_json::Value::Null, + session_id: "".into(), + turn_id: snapshot.turn_id.clone(), + metadata: snapshot.metadata.clone(), + }; + let outcome = if snapshot + .metadata + .get(TOOL_RESULT_NOT_STARTED_METADATA_KEY) + .and_then(|v| v.as_bool()) + == Some(true) + { + ToolExecutionOutcome::FailedBeforeInvocation(error) + } else { + ToolExecutionOutcome::Failed(error) + }; + let mut resolution = map_outcome_to_resolution(Some(snapshot.id.clone()), request, outcome); + attach_observations(&mut resolution, snapshot.failure_observations.as_ref()); + resolution +} + fn map_outcome_to_resolution( task_id: Option, - request: ToolRequest, + mut request: ToolRequest, outcome: ToolExecutionOutcome, ) -> TaskResolution { + strip_failure_metadata(&mut request.metadata); + let not_started = matches!(&outcome, ToolExecutionOutcome::FailedBeforeInvocation(_)); match outcome { - ToolExecutionOutcome::Completed(result) => TaskResolution::Item(Item { - id: None, - kind: agentkit_core::ItemKind::Tool, - parts: vec![agentkit_core::Part::ToolResult(result.result)], - metadata: result.metadata, - usage: None, - finish_reason: None, - created_at: None, - }), + ToolExecutionOutcome::Completed(mut result) => { + strip_failure_metadata(&mut result.result.metadata); + strip_failure_metadata(&mut result.metadata); + TaskResolution::Item(Item { + id: None, + kind: agentkit_core::ItemKind::Tool, + parts: vec![agentkit_core::Part::ToolResult(result.result)], + metadata: result.metadata, + usage: None, + finish_reason: None, + created_at: None, + }) + } ToolExecutionOutcome::Interrupted( agentkit_tools_core::ToolInterruption::ApprovalRequired(mut approval), ) => { @@ -916,38 +1883,10 @@ fn map_outcome_to_resolution( approval, }) } - ToolExecutionOutcome::FailedBeforeInvocation(error) => { + ToolExecutionOutcome::FailedBeforeInvocation(error) + | ToolExecutionOutcome::Failed(error) => { let mut metadata = request.metadata; - metadata.insert(TOOL_RESULT_NOT_STARTED_METADATA_KEY.into(), true.into()); - if matches!(error, ToolError::PermissionDenied(_)) { - metadata.insert( - TOOL_RESULT_FAILURE_KIND_METADATA_KEY.into(), - TOOL_RESULT_FAILURE_KIND_PERMISSION_DENIED.into(), - ); - } - TaskResolution::Item(Item { - id: None, - kind: agentkit_core::ItemKind::Tool, - parts: vec![agentkit_core::Part::ToolResult(ToolResultPart { - call_id: request.call_id, - output: agentkit_core::ToolOutput::Text(error.to_string()), - is_error: true, - metadata, - })], - metadata: MetadataMap::new(), - usage: None, - finish_reason: None, - created_at: None, - }) - } - ToolExecutionOutcome::Failed(error) => { - let mut metadata = request.metadata; - if matches!(error, ToolError::PermissionDenied(_)) { - metadata.insert( - TOOL_RESULT_FAILURE_KIND_METADATA_KEY.into(), - TOOL_RESULT_FAILURE_KIND_PERMISSION_DENIED.into(), - ); - } + write_failure_metadata(&mut metadata, &error, not_started); TaskResolution::Item(Item { id: None, kind: agentkit_core::ItemKind::Tool, @@ -968,6 +1907,7 @@ fn map_outcome_to_resolution( #[cfg(test)] mod tests { + mod failure_tests; use std::collections::BTreeMap; use std::sync::Arc as StdArc; use std::sync::atomic::{AtomicBool, Ordering as AtomicOrdering}; @@ -998,6 +1938,13 @@ mod tests { #[derive(Clone)] enum TestBehavior { + ObserveBlock { + entered: StdArc, + release: StdArc, + publisher: + StdArc>>, + outcome: ToolExecutionOutcome, + }, Block { entered: StdArc, release: StdArc, @@ -1048,6 +1995,24 @@ mod tests { _ctx: &mut agentkit_tools_core::ToolContext<'_>, ) -> ToolExecutionOutcome { match self.behaviors.get(request.tool_name.0.as_str()) { + Some(TestBehavior::ObserveBlock { + entered, + release, + publisher, + outcome, + }) => { + let observer = _ctx + .failure_observer() + .expect("manager installed publisher"); + let mut effects = agentkit_core::failure::PossibleEffects::default(); + effects.source = agentkit_core::failure::ObservationSource::LocalSession; + effects.tool_execution_start_reported = true; + observer.publish_effects(effects).unwrap(); + *publisher.lock().unwrap() = Some(observer); + entered.store(true, AtomicOrdering::SeqCst); + release.notified().await; + outcome.clone() + } Some(TestBehavior::Block { entered, release, @@ -1127,6 +2092,7 @@ mod tests { TaskStartContext { executor, tool_context: OwnedToolContext { + failure_observer: None, session_id: SessionId::new("session-1"), turn_id: turn_id.clone(), metadata: MetadataMap::new(), diff --git a/crates/agentkit-task-manager/src/tests/failure_tests.rs b/crates/agentkit-task-manager/src/tests/failure_tests.rs new file mode 100644 index 0000000..103179a --- /dev/null +++ b/crates/agentkit-task-manager/src/tests/failure_tests.rs @@ -0,0 +1,944 @@ +use super::*; +use agentkit_core::failure::{FailureCode, FailureMetadataV1, ObservationSource, PossibleEffects}; +use agentkit_tools_core::{ + DiagnosticFailureKind, DiagnosticToolFailure, ObservationPublishError, ToolFailureKind, +}; + +fn native(cancelled: bool) -> ToolError { + let mut effects = PossibleEffects::default(); + effects.source = ObservationSource::AcpNotifications; + effects.assistant_output_observed = true; + ToolError::diagnostic(DiagnosticToolFailure { + kind: if cancelled { + DiagnosticFailureKind::Cancelled + } else { + DiagnosticFailureKind::ExecutionFailed + }, + metadata: FailureMetadataV1::new(FailureCode::ChildFailed).with_effects(effects), + }) +} +fn result(resolution: TaskResolution) -> ToolResultPart { + let TaskResolution::Item(item) = resolution else { + panic!("expected item") + }; + item.parts + .into_iter() + .find_map(|part| match part { + Part::ToolResult(result) => Some(result), + _ => None, + }) + .unwrap() +} +fn item_result(item: Item) -> ToolResultPart { + result(TaskResolution::Item(item)) +} + +#[test] +fn failed_and_preinvocation_projections_replace_reserved_caller_keys() { + for before in [false, true] { + for error in [ + native(false), + native(true), + ToolError::Unavailable("legacy".into()), + ] { + let mut request = make_request("test", "turn", "call"); + request.metadata = [ + ( + TOOL_RESULT_FAILURE_METADATA_KEY.into(), + json!({"kind":"permission_denied"}), + ), + ( + TOOL_RESULT_FAILURE_OBSERVATIONS_METADATA_KEY.into(), + json!({"effects":{"source":"local_session"}}), + ), + ( + TOOL_RESULT_FAILURE_KIND_METADATA_KEY.into(), + json!("permission_denied"), + ), + (TOOL_RESULT_NOT_STARTED_METADATA_KEY.into(), json!(true)), + ("application".into(), json!(42)), + ] + .into(); + let outcome = if before { + ToolExecutionOutcome::FailedBeforeInvocation(error.clone()) + } else { + ToolExecutionOutcome::Failed(error.clone()) + }; + let result = result(map_outcome_to_resolution(None, request, outcome)); + assert!(result.is_error); + assert_eq!( + tool_failure_info(&result).unwrap(), + Some(error.failure_info()) + ); + assert_eq!( + result.metadata.get(TOOL_RESULT_NOT_STARTED_METADATA_KEY), + before.then_some(&json!(true)) + ); + assert!( + !result + .metadata + .contains_key(TOOL_RESULT_FAILURE_KIND_METADATA_KEY) + ); + assert!(task_failure_observations(&result).unwrap().is_none()); + assert_eq!(result.metadata["application"], json!(42)); + } + } +} + +#[tokio::test] +async fn failure_cancellation_and_abort_preserve_isolated_frozen_facts_in_all_delivery_modes() { + for (background, manual) in [(false, false), (true, false), (true, true)] { + for terminal in ["failed", "cancelled", "abort"] { + let manager = AsyncTaskManager::new().routing(move |_: &ToolRequest| { + if background { + RoutingDecision::Background + } else { + RoutingDecision::Foreground + } + }); + let handle = manager.handle(); + let entered = StdArc::new(AtomicBool::new(false)); + let release = StdArc::new(Notify::new()); + let publisher = StdArc::new(std::sync::Mutex::new(None)); + let error = native(terminal == "cancelled"); + let executor: Arc = Arc::new(TestExecutor::new([( + "observed", + TestBehavior::ObserveBlock { + entered: entered.clone(), + release: release.clone(), + publisher: publisher.clone(), + outcome: ToolExecutionOutcome::Failed(error.clone()), + }, + )])); + let mut request = make_request("observed", "turn", "call"); + request + .metadata + .insert(TOOL_RESULT_FAILURE_METADATA_KEY.into(), json!("forged")); + let template_slot = agentkit_tools_core::FailureObservationSlot::new(); + let mut context = make_context(executor, &request.turn_id, None); + context.tool_context.failure_observer = Some(template_slot.publisher()); + let start = manager + .start_task(TaskLaunchRequest::plain(None, request.clone()), context) + .await + .unwrap(); + let TaskStartOutcome::Pending { task_id, .. } = start else { + panic!() + }; + assert!( + matches!(next_event(&handle).await, TaskEvent::Started(snapshot) if snapshot.failure.is_none() && !snapshot.metadata.contains_key(TOOL_RESULT_FAILURE_METADATA_KEY)) + ); + if manual { + handle + .set_delivery_mode(task_id.clone(), DeliveryMode::Manual) + .await + .unwrap(); + } + wait_until_entered(&entered).await; + assert!( + template_slot.snapshot().is_none(), + "manager must replace caller publisher" + ); + if terminal == "abort" { + handle.cancel(task_id.clone()).await.unwrap(); + } else { + release.notify_one(); + } + let event = next_event(&handle).await; + let snapshot = match event { + TaskEvent::Failed(snapshot, actual) => { + assert_eq!(terminal, "failed"); + assert_eq!(actual, error); + snapshot + } + TaskEvent::Cancelled(snapshot) => { + assert_ne!(terminal, "failed"); + snapshot + } + other => panic!("wrong terminal {other:?}"), + }; + let frozen = snapshot.failure_observations.clone().unwrap(); + assert_eq!( + frozen.effects().unwrap().source, + ObservationSource::LocalSession + ); + assert!(frozen.effects().unwrap().tool_execution_start_reported); + assert!(frozen.effects().unwrap().observation_incomplete()); + assert!(frozen.receipt().is_none()); + assert!(frozen.retry().is_none()); + if terminal == "abort" { + assert_eq!( + snapshot.failure.as_ref().unwrap().kind, + ToolFailureKind::Cancelled + ); + assert!(snapshot.failure.as_ref().unwrap().metadata.is_none()); + } else { + assert_eq!(snapshot.failure, Some(error.failure_info())); + assert_eq!( + snapshot + .failure + .as_ref() + .unwrap() + .metadata + .as_ref() + .unwrap() + .effects() + .unwrap() + .source, + ObservationSource::AcpNotifications + ); + } + let projected = if !background { + let Some(TurnTaskUpdate::Resolution(resolution)) = + manager.wait_for_turn(&request.turn_id, None).await.unwrap() + else { + panic!() + }; + result(*resolution) + } else if manual { + let mut items = handle.drain_ready_items().await; + assert_eq!(items.len(), 1); + item_result(items.remove(0)) + } else { + let mut updates = manager.take_pending_loop_updates().await.unwrap(); + assert_eq!(updates.resolutions.len(), 1); + result(updates.resolutions.pop_front().unwrap()) + }; + assert_eq!(tool_failure_info(&projected).unwrap(), snapshot.failure); + assert_eq!(task_failure_observations(&projected).unwrap(), Some(frozen)); + assert_eq!( + publisher + .lock() + .unwrap() + .as_ref() + .unwrap() + .publish_effects(PossibleEffects::default()), + Err(ObservationPublishError::Sealed) + ); + handle.cancel(task_id.clone()).await.unwrap(); + handle.cancel(task_id.clone()).await.unwrap(); + release.notify_one(); + assert!( + timeout(Duration::from_millis(20), handle.next_event()) + .await + .is_err() + ); + assert!(handle.list_running().await.is_empty()); + assert_eq!(handle.list_completed().await.len(), 1); + } + } +} + +#[tokio::test] +async fn success_after_observations_strips_failure_only_metadata_and_cannot_be_recancelled() { + let manager = AsyncTaskManager::new(); + let handle = manager.handle(); + let entered = StdArc::new(AtomicBool::new(false)); + let release = StdArc::new(Notify::new()); + let publisher = StdArc::new(std::sync::Mutex::new(None)); + let request = make_request("observed", "turn", "call"); + let forged: MetadataMap = [ + TOOL_RESULT_FAILURE_METADATA_KEY, + TOOL_RESULT_FAILURE_KIND_METADATA_KEY, + TOOL_RESULT_FAILURE_OBSERVATIONS_METADATA_KEY, + TOOL_RESULT_NOT_STARTED_METADATA_KEY, + ] + .map(|key| (key.into(), json!(true))) + .into(); + let executor: Arc = Arc::new(TestExecutor::new([( + "observed", + TestBehavior::ObserveBlock { + entered: entered.clone(), + release: release.clone(), + publisher, + outcome: ToolExecutionOutcome::Completed(ToolResult { + result: ToolResultPart { + call_id: request.call_id.clone(), + output: ToolOutput::Text("ok".into()), + is_error: false, + metadata: forged.clone(), + }, + duration: None, + metadata: forged, + }), + }, + )])); + manager + .start_task( + TaskLaunchRequest::plain(None, request.clone()), + make_context(executor, &request.turn_id, None), + ) + .await + .unwrap(); + let TaskEvent::Started(started) = next_event(&handle).await else { + panic!() + }; + wait_until_entered(&entered).await; + release.notify_one(); + let TaskEvent::Completed(snapshot, result) = next_event(&handle).await else { + panic!() + }; + assert!(snapshot.failure.is_none()); + assert!(snapshot.failure_observations.is_none()); + assert!(result.metadata.is_empty()); + handle.cancel(started.id).await.unwrap(); + assert!( + timeout(Duration::from_millis(20), handle.next_event()) + .await + .is_err() + ); + let Some(TurnTaskUpdate::Resolution(resolution)) = + manager.wait_for_turn(&request.turn_id, None).await.unwrap() + else { + panic!() + }; + let TaskResolution::Item(item) = *resolution else { + panic!() + }; + assert!(item.metadata.is_empty()); +} + +#[tokio::test] +async fn interruption_exposes_frozen_real_result_once_for_loop_synthesis() { + let manager = AsyncTaskManager::new(); + let handle = manager.handle(); + let entered = StdArc::new(AtomicBool::new(false)); + let release = StdArc::new(Notify::new()); + let publisher = StdArc::new(std::sync::Mutex::new(None)); + let request = make_request("observed", "turn", "call"); + let executor: Arc = Arc::new(TestExecutor::new([( + "observed", + TestBehavior::ObserveBlock { + entered: entered.clone(), + release, + publisher, + outcome: ToolExecutionOutcome::Failed(native(false)), + }, + )])); + manager + .start_task( + TaskLaunchRequest::plain(None, request.clone()), + make_context(executor, &request.turn_id, None), + ) + .await + .unwrap(); + next_event(&handle).await; + wait_until_entered(&entered).await; + manager.on_turn_interrupted(&request.turn_id).await.unwrap(); + assert!(matches!(next_event(&handle).await, TaskEvent::Cancelled(_))); + let mut updates = manager + .take_interrupted_task_updates(&request.session_id, std::slice::from_ref(&request.call_id)); + let mut items: Vec<_> = updates + .drain(..) + .filter_map(|update| match update { + TurnTaskUpdate::Resolution(resolution) => match *resolution { + TaskResolution::Item(item) => Some(item), + _ => None, + }, + _ => None, + }) + .collect(); + assert_eq!(items.len(), 1); + assert!( + task_failure_observations(&item_result(items.remove(0))) + .unwrap() + .is_some() + ); + assert!( + manager + .take_interrupted_task_updates( + &request.session_id, + std::slice::from_ref(&request.call_id) + ) + .is_empty() + ); + assert!( + manager + .wait_for_turn(&request.turn_id, None) + .await + .unwrap() + .is_none() + ); +} + +#[tokio::test] +async fn approved_generations_seal_old_publishers_and_reject_stale_detach() { + let manager = AsyncTaskManager::new(); + let handle = manager.handle(); + let entered = StdArc::new(AtomicBool::new(false)); + let release = StdArc::new(Notify::new()); + let publisher = StdArc::new(std::sync::Mutex::new(None)); + let request = make_request("observed", "turn", "call"); + let approval = ApprovalRequest { + task_id: None, + call_id: Some(request.call_id.clone()), + id: "approval:observed".into(), + request_kind: "tool.test".into(), + reason: ApprovalReason::SensitivePath, + summary: "approve".into(), + metadata: MetadataMap::new(), + }; + let executor: Arc = Arc::new(TestExecutor::new([( + "observed", + TestBehavior::ObserveBlock { + entered: entered.clone(), + release: release.clone(), + publisher: publisher.clone(), + outcome: ToolExecutionOutcome::Interrupted(ToolInterruption::ApprovalRequired( + approval.clone(), + )), + }, + )])); + let context = make_context(executor, &request.turn_id, None); + let TaskStartOutcome::Pending { task_id, .. } = manager + .start_task( + TaskLaunchRequest::plain(None, request.clone()), + context.clone(), + ) + .await + .unwrap() + else { + panic!() + }; + next_event(&handle).await; + wait_until_entered(&entered).await; + assert!(matches!( + manager + .start_task( + TaskLaunchRequest::plain(Some(task_id.clone()), request.clone()), + context.clone() + ) + .await, + Err(TaskManagerError::AlreadyRunning(_)) + )); + let old_generation = manager.inner.state.lock().await.tasks[&task_id] + .generation + .clone(); + let old_publisher = publisher.lock().unwrap().clone().unwrap(); + release.notify_one(); + assert!( + matches!(manager.wait_for_turn(&request.turn_id, None).await.unwrap(), Some(TurnTaskUpdate::Resolution(resolution)) if matches!(*resolution, TaskResolution::Approval(_))) + ); + assert!( + handle.list_completed().await.is_empty(), + "approval is not terminal completion" + ); + assert_eq!( + old_publisher.publish_effects(PossibleEffects::default()), + Err(ObservationPublishError::Sealed) + ); + entered.store(false, AtomicOrdering::SeqCst); + manager + .start_task( + TaskLaunchRequest::approved(Some(task_id.clone()), request.clone(), approval), + context, + ) + .await + .unwrap(); + next_event(&handle).await; + wait_until_entered(&entered).await; + assert!(matches!( + manager + .inner + .detach_running_foreground(&task_id, Some(&old_generation)) + .await, + Err(TaskManagerError::NotRunning(_)) + )); + handle.cancel(task_id).await.unwrap(); + assert!(matches!(next_event(&handle).await, TaskEvent::Cancelled(_))); +} + +#[tokio::test] +async fn cancellation_before_executor_admission_marks_not_started_and_freezes_empty_slot() { + // Deterministically exercise the state between record insertion and start-gate + // admission; no spawned executor has permission to run in this state. + let manager = AsyncTaskManager::new(); + let handle = manager.handle(); + let id = TaskId::new("pending-start"); + let slot = agentkit_tools_core::FailureObservationSlot::new(); + let snapshot = TaskSnapshot { + id: id.clone(), + turn_id: TurnId::new("turn"), + call_id: ToolCallId::new("call"), + tool_name: "test".into(), + kind: TaskKind::Foreground, + metadata: MetadataMap::new(), + failure: None, + failure_observations: None, + }; + { + let mut state = manager.inner.state.lock().await; + state.per_turn_running.insert(snapshot.turn_id.clone(), 1); + state.tasks.insert( + id.clone(), + TaskRecord { + cancel_signal: None, + suspended: None, + session_id: SessionId::new("session"), + generation: Arc::new(()), + observations: slot.clone(), + snapshot, + continue_policy: ContinuePolicy::NotifyOnly, + delivery_mode: DeliveryMode::ToLoop, + running: true, + invocation_admitted: false, + completed: false, + join: None, + }, + ); + } + handle.cancel(id).await.unwrap(); + let TaskEvent::Cancelled(snapshot) = next_event(&handle).await else { + panic!() + }; + assert!(snapshot.failure_observations.is_none()); + let Some(TurnTaskUpdate::Resolution(resolution)) = manager + .wait_for_turn(&snapshot.turn_id, None) + .await + .unwrap() + else { + panic!() + }; + let result = result(*resolution); + assert_eq!( + result.metadata[TOOL_RESULT_NOT_STARTED_METADATA_KEY], + json!(true) + ); + assert_eq!( + slot.publisher().publish_effects(PossibleEffects::default()), + Err(ObservationPublishError::Sealed) + ); +} + +#[tokio::test] +async fn inline_drop_seals_publisher_and_preserves_one_cancellation_projection() { + let manager = SimpleTaskManager::new(); + let handle = manager.handle(); + let entered = StdArc::new(AtomicBool::new(false)); + let release = StdArc::new(Notify::new()); + let publisher = StdArc::new(std::sync::Mutex::new(None)); + let request = make_request("observed", "turn", "inline-call"); + let executor: Arc = Arc::new(TestExecutor::new([( + "observed", + TestBehavior::ObserveBlock { + entered: entered.clone(), + release, + publisher: publisher.clone(), + outcome: ToolExecutionOutcome::Failed(native(false)), + }, + )])); + { + let start = manager.start_task( + TaskLaunchRequest::plain(None, request.clone()), + make_context(executor, &request.turn_id, None), + ); + tokio::pin!(start); + tokio::select! { _ = wait_until_entered(&entered) => {}, result = &mut start => panic!("completed early: {result:?}") } + } + assert!(matches!(next_event(&handle).await, TaskEvent::Started(_))); + let TaskEvent::Cancelled(snapshot) = next_event(&handle).await else { + panic!() + }; + assert!( + snapshot + .failure_observations + .unwrap() + .effects() + .unwrap() + .tool_execution_start_reported + ); + assert_eq!( + publisher + .lock() + .unwrap() + .as_ref() + .unwrap() + .publish_effects(PossibleEffects::default()), + Err(ObservationPublishError::Sealed) + ); + assert!(handle.list_running().await.is_empty()); + assert!( + manager + .take_interrupted_task_updates( + &SessionId::new("different-session"), + std::slice::from_ref(&request.call_id) + ) + .is_empty() + ); + assert_eq!( + manager + .take_interrupted_task_updates( + &request.session_id, + std::slice::from_ref(&request.call_id) + ) + .len(), + 1 + ); + assert!( + manager + .take_interrupted_task_updates(&request.session_id, &[request.call_id]) + .is_empty() + ); +} + +#[tokio::test] +async fn inline_approval_retains_facts_into_fresh_generation_and_suspended_cancel() { + for resume in [false, true] { + let manager = SimpleTaskManager::new(); + let handle = manager.handle(); + let entered = StdArc::new(AtomicBool::new(false)); + let release = StdArc::new(Notify::new()); + let publisher = StdArc::new(std::sync::Mutex::new(None)); + let request = make_request("observed", "turn", "inline-approval"); + let approval = ApprovalRequest { + task_id: None, + call_id: Some(request.call_id.clone()), + id: "approval:inline".into(), + request_kind: "tool.test".into(), + reason: ApprovalReason::SensitivePath, + summary: "approve".into(), + metadata: MetadataMap::new(), + }; + let executor: Arc = Arc::new(TestExecutor::new([( + "observed", + TestBehavior::ObserveBlock { + entered: entered.clone(), + release: release.clone(), + publisher: publisher.clone(), + outcome: ToolExecutionOutcome::Interrupted(ToolInterruption::ApprovalRequired( + approval, + )), + }, + )])); + release.notify_one(); + let TaskStartOutcome::Ready(resolution) = manager + .start_task( + TaskLaunchRequest::plain(None, request.clone()), + make_context(executor, &request.turn_id, None), + ) + .await + .unwrap() + else { + panic!() + }; + let TaskResolution::Approval(task) = *resolution else { + panic!() + }; + next_event(&handle).await; + assert!(handle.list_completed().await.is_empty()); + assert_eq!(handle.list_suspended().await.len(), 1); + let old = publisher.lock().unwrap().clone().unwrap(); + assert_eq!( + old.publish_effects(PossibleEffects::default()), + Err(ObservationPublishError::Sealed) + ); + if resume { + entered.store(false, AtomicOrdering::SeqCst); + let executor: Arc = Arc::new(TestExecutor::new([( + "observed", + TestBehavior::Block { + entered: entered.clone(), + release: StdArc::new(Notify::new()), + output: "not reached", + }, + )])); + let context = make_context(executor, &request.turn_id, None); + let mut wrong = request.clone(); + wrong.session_id = SessionId::new("other"); + assert!(matches!( + manager + .start_task( + TaskLaunchRequest::approved( + Some(task.task_id.clone()), + wrong, + task.approval.clone() + ), + context.clone() + ) + .await, + Err(TaskManagerError::InvalidContinuation(_)) + )); + { + let start = manager.start_task( + TaskLaunchRequest::approved( + Some(task.task_id.clone()), + request.clone(), + task.approval, + ), + context, + ); + tokio::pin!(start); + tokio::select! { _ = wait_until_entered(&entered) => {}, result = &mut start => panic!("completed early: {result:?}") } + } + assert!(matches!(next_event(&handle).await, TaskEvent::Started(_))); + } else { + handle.cancel(task.task_id.clone()).await.unwrap(); + } + let TaskEvent::Cancelled(snapshot) = next_event(&handle).await else { + panic!() + }; + assert!( + snapshot + .failure_observations + .unwrap() + .effects() + .unwrap() + .tool_execution_start_reported + ); + assert_eq!( + manager + .take_interrupted_task_updates(&request.session_id, &[request.call_id]) + .len(), + 1 + ); + handle.cancel(task.task_id).await.unwrap(); + assert!( + timeout(Duration::from_millis(10), handle.next_event()) + .await + .is_err() + ); + } +} + +#[tokio::test] +async fn dropping_launch_before_registration_cannot_strand_a_task() { + let manager = AsyncTaskManager::new(); + let handle = manager.handle(); + let state = manager.inner.state.lock().await; + let request = make_request("unknown", "turn", "pre-register"); + let executor: Arc = + Arc::new(TestExecutor::new(Vec::<(String, TestBehavior)>::new())); + { + let start = manager.start_task( + TaskLaunchRequest::plain(Some(TaskId::new("explicit")), request.clone()), + make_context(executor, &request.turn_id, None), + ); + tokio::pin!(start); + std::future::poll_fn(|cx| { + assert!(std::future::Future::poll(start.as_mut(), cx).is_pending()); + std::task::Poll::Ready(()) + }) + .await; + } + assert!(state.tasks.is_empty()); + drop(state); + assert!(handle.list_running().await.is_empty()); + assert!( + timeout(Duration::from_millis(10), handle.next_event()) + .await + .is_err() + ); +} + +#[tokio::test] +async fn concurrent_tasks_from_one_context_template_keep_distinct_receipts() { + let manager = AsyncTaskManager::new(); + let handle = manager.handle(); + let entries: Vec<_> = ["first", "second"] + .into_iter() + .map(|name| { + ( + name, + StdArc::new(AtomicBool::new(false)), + StdArc::new(std::sync::Mutex::new(None)), + ) + }) + .collect(); + let executor: Arc = Arc::new(TestExecutor::new(entries.iter().map( + |(name, entered, publisher)| { + ( + *name, + TestBehavior::ObserveBlock { + entered: entered.clone(), + release: StdArc::new(Notify::new()), + publisher: publisher.clone(), + outcome: ToolExecutionOutcome::Failed(native(false)), + }, + ) + }, + ))); + let context = make_context(executor, &TurnId::new("shared-turn"), None); + let mut ids = Vec::new(); + for (name, _, _) in &entries { + let request = make_request(name, "shared-turn", name); + let TaskStartOutcome::Pending { task_id, .. } = manager + .start_task(TaskLaunchRequest::plain(None, request), context.clone()) + .await + .unwrap() + else { + panic!() + }; + ids.push(task_id); + } + for (name, entered, publisher) in &entries { + wait_until_entered(entered).await; + publisher + .lock() + .unwrap() + .as_ref() + .unwrap() + .publish_receipt(agentkit_core::failure::HostFatalReceipt { + session_id: agentkit_core::failure::HostReceiptId::new("session").unwrap(), + event_id: agentkit_core::failure::HostReceiptId::new(name).unwrap(), + storage: agentkit_core::failure::FatalStorage::Unavailable, + }) + .unwrap(); + } + assert!(matches!(next_event(&handle).await, TaskEvent::Started(_))); + assert!(matches!(next_event(&handle).await, TaskEvent::Started(_))); + for id in ids { + handle.cancel(id).await.unwrap(); + } + for _ in 0..2 { + let TaskEvent::Cancelled(snapshot) = next_event(&handle).await else { + panic!() + }; + assert_eq!( + snapshot + .failure_observations + .unwrap() + .receipt() + .unwrap() + .event_id + .as_str(), + snapshot.call_id.0 + ); + } +} + +#[tokio::test] +async fn queued_winners_keep_origin_session_when_turn_and_call_ids_collide() { + let manager = AsyncTaskManager::new(); + let handle = manager.handle(); + for (session, cancelled) in [("a", false), ("b", true)] { + let executor: Arc = Arc::new(TestExecutor::new([( + "observed", + TestBehavior::ObserveBlock { + entered: StdArc::new(AtomicBool::new(false)), + release: { + let n = StdArc::new(Notify::new()); + n.notify_one(); + n + }, + publisher: StdArc::new(std::sync::Mutex::new(None)), + outcome: ToolExecutionOutcome::Failed(native(cancelled)), + }, + )])); + let mut request = make_request("observed", "same-turn", "same-call"); + request.session_id = session.into(); + manager + .start_task( + TaskLaunchRequest::plain(None, request.clone()), + make_context(executor, &request.turn_id, None), + ) + .await + .unwrap(); + assert!(matches!(next_event(&handle).await, TaskEvent::Started(_))); + let event = next_event(&handle).await; + assert!(matches!( + event, + TaskEvent::Failed(..) | TaskEvent::Cancelled(_) + )); + } + manager + .on_turn_interrupted(&"same-turn".into()) + .await + .unwrap(); + for (session, cancelled) in [("a", false), ("b", true)] { + let updates = manager.take_interrupted_task_updates(&session.into(), &["same-call".into()]); + assert_eq!(updates.len(), 1); + let TurnTaskUpdate::Resolution(resolution) = updates.into_iter().next().unwrap() else { + panic!() + }; + assert_eq!( + tool_failure_info(&result(*resolution)).unwrap(), + Some(native(cancelled).failure_info()) + ); + } +} + +#[tokio::test] +async fn unsurfaced_background_approval_interruption_preserves_delivery_policy() { + for manual in [false, true] { + let manager = + AsyncTaskManager::new().routing(|_: &ToolRequest| RoutingDecision::Background); + let handle = manager.handle(); + let request = make_request("observed", "turn", "call"); + let entered = StdArc::new(AtomicBool::new(false)); + let release = StdArc::new(Notify::new()); + let approval = ApprovalRequest { + task_id: None, + call_id: Some(request.call_id.clone()), + id: "approval:bg".into(), + request_kind: "test".into(), + reason: ApprovalReason::SensitivePath, + summary: "approve".into(), + metadata: MetadataMap::new(), + }; + let executor: Arc = Arc::new(TestExecutor::new([( + "observed", + TestBehavior::ObserveBlock { + entered: entered.clone(), + release: release.clone(), + publisher: StdArc::new(std::sync::Mutex::new(None)), + outcome: ToolExecutionOutcome::Interrupted(ToolInterruption::ApprovalRequired( + approval, + )), + }, + )])); + let TaskStartOutcome::Pending { task_id, .. } = manager + .start_task( + TaskLaunchRequest::plain(None, request.clone()), + make_context(executor, &request.turn_id, None), + ) + .await + .unwrap() + else { + panic!() + }; + if manual { + handle + .set_delivery_mode(task_id, DeliveryMode::Manual) + .await + .unwrap(); + } + wait_until_entered(&entered).await; + release.notify_one(); + timeout(Duration::from_secs(2), async { + while handle.list_suspended().await.is_empty() { + tokio::task::yield_now().await; + } + }) + .await + .unwrap(); + manager.on_turn_interrupted(&request.turn_id).await.unwrap(); + assert!(handle.list_suspended().await.is_empty()); + let results = if manual { + handle + .drain_ready_items() + .await + .into_iter() + .map(item_result) + .collect::>() + } else { + manager + .take_pending_loop_updates() + .await + .unwrap() + .resolutions + .into_iter() + .map(result) + .collect() + }; + assert_eq!(results.len(), 1); + assert_eq!( + tool_failure_info(&results[0]).unwrap().unwrap().kind, + ToolFailureKind::Cancelled + ); + assert!(task_failure_observations(&results[0]).unwrap().is_some()); + assert!( + manager + .take_interrupted_task_updates(&request.session_id, &[request.call_id]) + .is_empty() + ); + } +} diff --git a/crates/agentkit-tool-compose/Cargo.toml b/crates/agentkit-tool-compose/Cargo.toml index cd3361e..e79b84b 100644 --- a/crates/agentkit-tool-compose/Cargo.toml +++ b/crates/agentkit-tool-compose/Cargo.toml @@ -14,6 +14,7 @@ agentkit-core = { version = "0.10.5", path = "../agentkit-core" } agentkit-tools-core = { version = "0.10.5", path = "../agentkit-tools-core" } async-trait.workspace = true mlua = { workspace = true, optional = true } +getrandom = { version = "=0.4.3", default-features = false, optional = true } runlet = { version = "0.5.0", optional = true } serde = { workspace = true, features = ["derive"] } serde_json.workspace = true @@ -25,7 +26,7 @@ default = ["lua"] # Sandboxed Lua execution backend; enabled by default for backwards compatibility. lua = ["dep:mlua"] # Runlet execution backend (https://crates.io/crates/runlet); off by default. -runlet = ["dep:runlet", "tokio/rt"] +runlet = ["dep:runlet", "dep:getrandom", "tokio/rt"] # TOON encoding for compose results (ResultEncoding::Toon); off by default. toon = ["dep:serde_toon2"] diff --git a/crates/agentkit-tool-compose/src/lib.rs b/crates/agentkit-tool-compose/src/lib.rs index ee876d4..d85a298 100644 --- a/crates/agentkit-tool-compose/src/lib.rs +++ b/crates/agentkit-tool-compose/src/lib.rs @@ -331,6 +331,13 @@ impl ChildDispatcher { match outcome { ToolExecutionOutcome::Completed(result) => { + // Error-shaped legacy results are not successful compose values + // and must not enter the successful child replay cache. + if result.result.is_error { + return Err(DispatchError::Failed(ToolError::ExecutionFailed( + "nested tool returned an error result".into(), + ))); + } let output = tool_output_to_json(result.result.output).map_err(DispatchError::Failed)?; { diff --git a/crates/agentkit-tool-compose/src/runlet_backend.rs b/crates/agentkit-tool-compose/src/runlet_backend.rs index 6e822f4..5b8f6d8 100644 --- a/crates/agentkit-tool-compose/src/runlet_backend.rs +++ b/crates/agentkit-tool-compose/src/runlet_backend.rs @@ -95,6 +95,11 @@ const RULES_PRIMER: &str = "Run a Runlet program that composes available tools. `fail(\"NO_MATCH\", \"explanation\")` — e.g. `x = items[0] if items != [] else \ fail(\"EMPTY\", \"expected results\")` or a guard `g = fail(\"BAD\", \"...\") if \ invalid`.\n\ + - Native host failures expose sanitized `err.agentkit_failure` facts. Explicitly rethrow \ + with `fail(err.code, err.message, {agentkit_failure_token: err.agentkit_failure_token})`. \ + This preserves only that exact same-run failure; two-argument fail or changed code/message \ + creates a new failure without native facts. Never persist the ephemeral token. Catch-visible \ + facts must fit signed 64-bit integers; unrepresentable facts fail the bridge closed.\n\ - Validate an invariant eagerly with `assert(condition, \"message\")`; a false \ assertion fails the program. Assert inside Runlet instead of returning raw \ intermediate values for model-side checking.\n\ @@ -297,11 +302,7 @@ impl ComposeBackend for RunletBackend { eprintln!("[compose-runlet] executing program:\n{}\n---", run.script); } let handle = tokio::runtime::Handle::current(); - let bridge = Arc::new(HostBridge { - interrupt: StdMutex::new(None), - failures: StdMutex::new(HashMap::new()), - failure_counter: AtomicU64::new(0), - }); + let bridge = Arc::new(HostBridge::default()); let mut registry = RunletRegistry::new(); let mut names: Vec<(String, ToolName)> = Vec::new(); @@ -348,6 +349,12 @@ impl ComposeBackend for RunletBackend { let handle = handle.clone(); let tool_name = tool_name.clone(); builder = builder.tool(runlet_name.clone(), move |args, ctx| { + if bridge.transport_failed() { + return Err(RunletToolError::new( + "HOST_BRIDGE_FAILED", + "compose failure transport unavailable", + )); + } if bridge.interrupted() { return Err(RunletToolError::new( "HOST_INTERRUPTED", @@ -426,6 +433,15 @@ impl ComposeBackend for RunletBackend { return Err(ComposeOutcome::Interrupted(interruption)); } + if let Some(error) = bridge + .transport_failure + .lock() + .expect("transport failure lock") + .take() + { + return Err(ComposeOutcome::Failed(error)); + } + match result { Ok((execution, heal_notes)) => { let value = @@ -458,7 +474,7 @@ impl ComposeBackend for RunletBackend { } Err(RunletRunError::Run(error)) => Err(ComposeOutcome::Failed( bridge.recall_failure(&error).unwrap_or_else(|| { - ToolError::ExecutionFailed(render_runtime_error(&error, &run.script)) + ToolError::ExecutionFailed(bridge.redact_runtime_error(&error, &run.script)) }), )), } @@ -502,12 +518,23 @@ enum RunletRunError { /// Carries typed host state across the sync/async boundary: the pending /// approval interruption and the original [`ToolError`] behind each dispatch /// failure (runlet only transports its own string-based error type). +#[derive(Default)] struct HostBridge { interrupt: StdMutex>, - failures: StdMutex>, + failures: StdMutex>, + transport_failure: StdMutex>, failure_counter: AtomicU64, } +struct SavedFailure { + error: ToolError, + code: String, + message: String, +} + +const MAX_SAVED_FAILURES: usize = 4096; +const FAILURE_TOKEN_KEY: &str = "agentkit_failure_token"; +const FAILURE_FACTS_KEY: &str = "agentkit_failure"; const FAILURE_CODE_PREFIX: &str = "HOST_FAILURE_"; impl HostBridge { @@ -526,20 +553,119 @@ impl HostBridge { self.interrupt.lock().expect("interrupt lock").take() } + fn redact_runtime_error(&self, error: &RunletToolError, source: &str) -> String { + let mut rendered = render_runtime_error(error, source); + for token in self.failures.lock().expect("failures lock").keys() { + rendered = rendered.replace(token, ""); + } + if let Some(CanonicalValue::String(token)) = error.details.get(FAILURE_TOKEN_KEY) + && !token.is_empty() + { + rendered = rendered.replace(token, ""); + } + rendered + } + + fn transport_failed(&self) -> bool { + self.transport_failure + .lock() + .expect("transport failure lock") + .is_some() + } + + fn fail_transport(&self) -> RunletToolError { + self.fail_transport_with_message("compose failure transport unavailable") + } + + fn fail_transport_with_message(&self, message: &'static str) -> RunletToolError { + let mut failure = self + .transport_failure + .lock() + .expect("transport failure lock"); + if failure.is_none() { + *failure = Some(ToolError::Internal(message.into())); + } + RunletToolError::new("HOST_BRIDGE_FAILED", message) + } + fn record_failure(&self, error: ToolError) -> RunletToolError { + let mut bytes = [0u8; 16]; + if getrandom::fill(&mut bytes).is_err() { + return self.fail_transport(); + } + let token: String = bytes.iter().map(|byte| format!("{byte:02x}")).collect(); + self.record_failure_with_token(error, token) + } + + fn record_failure_with_token(&self, error: ToolError, token: String) -> RunletToolError { + let facts = match canonical_failure_facts( + &serde_json::to_value(error.failure_info()).expect("finite failure facts"), + ) { + Ok(facts) => facts, + Err(()) => { + return self.fail_transport_with_message( + "compose failure facts exceed Runlet integer range", + ); + } + }; + let mut failures = self.failures.lock().expect("failures lock"); + if failures.len() >= MAX_SAVED_FAILURES || failures.contains_key(&token) { + return self.fail_transport(); + } let id = self.failure_counter.fetch_add(1, Ordering::Relaxed); + let code = format!("{FAILURE_CODE_PREFIX}{id}"); + // Observational retry metadata never makes a child invocation retryable. let retryable = matches!(error, ToolError::Unavailable(_)); let message = error.to_string(); - self.failures - .lock() - .expect("failures lock") - .insert(id, error); - RunletToolError::new(format!("{FAILURE_CODE_PREFIX}{id}"), message).retryable(retryable) + let mut result = RunletToolError::new(code.clone(), message.clone()).retryable(retryable); + result.details.insert( + FAILURE_TOKEN_KEY.into(), + CanonicalValue::String(token.clone()), + ); + // Only an exact, representable view is issued. It is advisory and is + // never used as input to native restoration. + result.details.insert(FAILURE_FACTS_KEY.into(), facts); + failures.insert( + token, + SavedFailure { + error, + code, + message, + }, + ); + result } fn recall_failure(&self, error: &RunletToolError) -> Option { - let id: u64 = error.code.strip_prefix(FAILURE_CODE_PREFIX)?.parse().ok()?; - self.failures.lock().expect("failures lock").remove(&id) + let CanonicalValue::String(token) = error.details.get(FAILURE_TOKEN_KEY)? else { + return None; + }; + if token.len() != 32 || !token.bytes().all(|b| b.is_ascii_hexdigit()) { + return None; + } + let mut failures = self.failures.lock().expect("failures lock"); + let saved = failures.get(token)?; + if saved.code != error.code || saved.message != error.message { + return None; + } + failures.remove(token).map(|saved| saved.error) + } +} + +fn canonical_failure_facts(value: &Value) -> Result { + match value { + Value::Number(n) => n + .as_i64() + .filter(|n| *n >= 0) + .map(CanonicalValue::Integer) + .ok_or(()), + Value::Object(fields) => fields + .iter() + .map(|(key, value)| Ok((key.clone(), canonical_failure_facts(value)?))) + .collect::, ()>>() + .map(CanonicalValue::Object), + Value::Array(_) => Err(()), // The closed diagnostic schema has no arrays. + _ => Ok(canonical_from_json(value)), } } @@ -803,3 +929,124 @@ fn json_from_canonical(value: &CanonicalValue) -> Result { ), }) } + +#[cfg(test)] +mod failure_bridge_tests { + use super::*; + fn native() -> ToolError { + ToolError::diagnostic(agentkit_tools_core::DiagnosticToolFailure { + kind: agentkit_tools_core::DiagnosticFailureKind::Cancelled, + metadata: agentkit_core::failure::FailureMetadataV1::default(), + }) + } + #[test] + fn issued_capability_restores_once_and_only_in_issuing_run() { + let bridge = HostBridge::default(); + let error = bridge.record_failure(native()); + assert!(!error.retryable); + assert!(HostBridge::default().recall_failure(&error).is_none()); + assert_eq!(bridge.recall_failure(&error), Some(native())); + assert!(bridge.recall_failure(&error).is_none()); + } + #[test] + fn fake_codes_changed_messages_and_invented_tokens_do_not_restore() { + let bridge = HostBridge::default(); + let error = bridge.record_failure(native()); + assert!( + bridge + .recall_failure(&RunletToolError::new( + error.code.clone(), + error.message.clone() + )) + .is_none() + ); + let mut changed = error.clone(); + changed.code = "REPLACED".into(); + assert!(bridge.recall_failure(&changed).is_none()); + changed = error.clone(); + changed.message = "REPLACED".into(); + assert!(bridge.recall_failure(&changed).is_none()); + changed = error.clone(); + changed.details.insert( + FAILURE_TOKEN_KEY.into(), + CanonicalValue::String("0".repeat(32)), + ); + assert!(bridge.recall_failure(&changed).is_none()); + // Attacker facts are not used to construct native errors, even with a valid capability. + changed = error.clone(); + changed.details.insert( + FAILURE_FACTS_KEY.into(), + CanonicalValue::String("PRIVATE".into()), + ); + assert_eq!(bridge.recall_failure(&changed), Some(native())); + } + #[test] + fn concurrent_failures_never_exchange_identities() { + let bridge = Arc::new(HostBridge::default()); + std::thread::scope(|scope| { + let threads: Vec<_> = (0..16) + .map(|n| { + let bridge = bridge.clone(); + scope.spawn(move || { + let original = ToolError::ExecutionFailed(format!("failure {n}")); + let transported = bridge.record_failure(original.clone()); + assert_eq!(bridge.recall_failure(&transported), Some(original)); + }) + }) + .collect(); + for thread in threads { + thread.join().unwrap(); + } + }); + } + #[test] + fn exhaustion_and_collision_latch_a_nonretryable_host_failure() { + let bridge = HostBridge::default(); + for n in 0..MAX_SAVED_FAILURES { + bridge.record_failure_with_token(native(), format!("{n:032x}")); + } + let result = bridge.record_failure(native()); + assert!(!result.retryable); + assert!(bridge.transport_failed()); + assert_eq!(bridge.failures.lock().unwrap().len(), MAX_SAVED_FAILURES); + let bridge = HostBridge::default(); + let original = bridge.record_failure_with_token(native(), "0".repeat(32)); + bridge.record_failure_with_token(ToolError::Internal("different".into()), "0".repeat(32)); + assert!(bridge.transport_failed()); + assert_eq!(bridge.recall_failure(&original), Some(native())); + } + #[test] + fn control_capabilities_are_redacted_from_runtime_rendering() { + let bridge = HostBridge::default(); + let mut error = bridge.record_failure(native()); + let CanonicalValue::String(token) = error.details[FAILURE_TOKEN_KEY].clone() else { + panic!() + }; + error.message = token.clone(); + assert!(!bridge.redact_runtime_error(&error, &token).contains(&token)); + } +} + +#[cfg(test)] +mod failure_numeric_tests { + use super::*; + #[test] + fn checked_fact_projection_accepts_exact_boundary_and_rejects_other_numbers() { + assert_eq!( + canonical_failure_facts(&serde_json::json!(i64::MAX)), + Ok(CanonicalValue::Integer(i64::MAX)) + ); + for value in [ + serde_json::json!(i64::MAX as u64 + 1), + serde_json::json!(u64::MAX), + serde_json::json!(-1), + serde_json::json!(0.5), + ] { + assert!(canonical_failure_facts(&value).is_err()); + assert!( + canonical_failure_facts(&serde_json::json!({"duration":{"secs":value,"nanos":0}})) + .is_err() + ); + } + } +} diff --git a/crates/agentkit-tool-compose/src/tests.rs b/crates/agentkit-tool-compose/src/tests.rs index ec99749..e511487 100644 --- a/crates/agentkit-tool-compose/src/tests.rs +++ b/crates/agentkit-tool-compose/src/tests.rs @@ -154,6 +154,7 @@ pub(crate) fn owned_context( cancellation: None, }; agentkit_tools_core::OwnedToolContext { + failure_observer: None, session_id, turn_id, metadata, @@ -660,3 +661,53 @@ fn type_notation_renders_json_schema_compactly() { #[cfg(feature = "runlet")] mod runlet; + +#[cfg(feature = "lua")] +#[tokio::test] +async fn lua_native_diagnostic_rethrow_preserves_kind_and_metadata() { + struct NativeFailure(ToolSpec, ToolError); + #[async_trait] + impl Tool for NativeFailure { + fn spec(&self) -> &ToolSpec { + &self.0 + } + async fn invoke( + &self, + _: ToolRequest, + _: &mut ToolContext<'_>, + ) -> Result { + Err(self.1.clone()) + } + } + for (script, preserve) in [ + ("return tool('typed_failure', {})", true), + ( + "local ok, err = pcall(function() return tool('typed_failure', {}) end); error(err)", + true, + ), + ( + "local ok, err = pcall(function() return tool('typed_failure', {}) end); error(tostring(err))", + false, + ), + ] { + let error = ToolError::diagnostic(agentkit_tools_core::DiagnosticToolFailure { + kind: agentkit_tools_core::DiagnosticFailureKind::Cancelled, + metadata: agentkit_core::failure::FailureMetadataV1::default(), + }); + let tool = NativeFailure( + ToolSpec::new("typed_failure", "fail", json!({"type":"object"})), + error.clone(), + ); + let outcome = + execute_compose(ComposeConfig::default(), tool, request(script, json!(null))).await; + let ToolExecutionOutcome::Failed(actual) = outcome else { + panic!("{outcome:?}") + }; + if preserve { + assert_eq!(actual, error); + } else { + assert!(actual.failure_metadata().is_none()); + assert!(!actual.is_cancelled()); + } + } +} diff --git a/crates/agentkit-tool-compose/src/tests/runlet.rs b/crates/agentkit-tool-compose/src/tests/runlet.rs index ed6aace..476cf0d 100644 --- a/crates/agentkit-tool-compose/src/tests/runlet.rs +++ b/crates/agentkit-tool-compose/src/tests/runlet.rs @@ -465,3 +465,167 @@ async fn prelude_intrinsics_and_folds_run_locally_without_consuming_call_budget( other => panic!("unexpected outcome: {other:?}"), } } + +struct TypedFailureTool { + spec: ToolSpec, + error: ToolError, + error_result: bool, +} +impl TypedFailureTool { + fn new(cancelled: bool) -> Self { + let receipt = agentkit_core::failure::HostFatalReceipt { + session_id: agentkit_core::failure::HostReceiptId::new("session-1").unwrap(), + event_id: agentkit_core::failure::HostReceiptId::new("event-1").unwrap(), + storage: agentkit_core::failure::FatalStorage::Unavailable, + }; + Self { + spec: ToolSpec::new("typed_failure", "fail natively", json!({"type":"object"})), + error: ToolError::diagnostic(agentkit_tools_core::DiagnosticToolFailure { + kind: if cancelled { + agentkit_tools_core::DiagnosticFailureKind::Cancelled + } else { + agentkit_tools_core::DiagnosticFailureKind::ExecutionFailed + }, + metadata: agentkit_core::failure::FailureMetadataV1::default() + .with_receipt(receipt), + }), + error_result: false, + } + } +} +#[async_trait::async_trait] +impl Tool for TypedFailureTool { + fn spec(&self) -> &ToolSpec { + &self.spec + } + async fn invoke( + &self, + request: ToolRequest, + _ctx: &mut ToolContext<'_>, + ) -> Result { + if self.error_result { + let mut result = ToolResultPart::success( + request.call_id, + ToolOutput::Text("PRIVATE error output".into()), + ); + result.is_error = true; + Ok(ToolResult::new(result)) + } else { + Err(self.error.clone()) + } + } +} + +#[tokio::test(flavor = "multi_thread")] +async fn runlet_native_failure_and_explicit_rethrow_retain_exact_metadata() { + for cancelled in [false, true] { + for script in [ + "return typed_failure({})", + "return boundary { return typed_failure({}) } catch err { return fail(err.code, err.message, {agentkit_failure_token: err.agentkit_failure_token}) }", + "return boundary { return typed_failure({}) } catch err { return fail(err.code, err.message, err) }", + ] { + let tool = TypedFailureTool::new(cancelled); + let expected = tool.error.clone(); + let result = + execute_compose(ComposeConfig::default(), tool, request(script, json!(null))).await; + assert_eq!(result, ToolExecutionOutcome::Failed(expected), "{script}"); + } + } +} + +#[tokio::test(flavor = "multi_thread")] +async fn runlet_transformation_and_fake_host_codes_do_not_resurrect_native_metadata() { + for script in [ + "return boundary { return typed_failure({}) } catch err { return fail(err.code, err.message) }", + "return boundary { return typed_failure({}) } catch err { return fail(\"HOST_FAILURE_0\", err.message) }", + "return boundary { return typed_failure({}) } catch err { return fail(\"REPLACED\", err.message, err) }", + ] { + let result = execute_compose( + ComposeConfig::default(), + TypedFailureTool::new(true), + request(script, json!(null)), + ) + .await; + let ToolExecutionOutcome::Failed(error) = result else { + panic!("{result:?}") + }; + assert!( + matches!(error, ToolError::ExecutionFailed(_)), + "{script}: {error:?}" + ); + assert!(!error.is_cancelled()); + assert!(error.failure_metadata().is_none()); + } +} + +#[tokio::test(flavor = "multi_thread")] +async fn runlet_catch_sees_closed_facts_and_can_explicitly_recover() { + let tool = TypedFailureTool::new(false); + let expected = serde_json::to_value(tool.error.failure_info()).unwrap(); + let result = execute_compose(ComposeConfig::default(), tool, request("return boundary { return typed_failure({}) } catch err { return err.agentkit_failure }", json!(null))).await; + let ToolExecutionOutcome::Completed(result) = result else { + panic!("{result:?}") + }; + assert_eq!( + crate::tool_output_to_json(result.result.output).unwrap(), + expected + ); + assert!(!result.result.is_error); +} + +#[tokio::test(flavor = "multi_thread")] +async fn runlet_does_not_accept_error_shaped_completed_child_as_success() { + let mut tool = TypedFailureTool::new(false); + tool.error_result = true; + let result = execute_compose( + ComposeConfig::default(), + tool, + request("return typed_failure({})", json!(null)), + ) + .await; + let ToolExecutionOutcome::Failed(error) = result else { + panic!("{result:?}") + }; + assert!(!error.to_string().contains("PRIVATE")); +} + +#[tokio::test(flavor = "multi_thread")] +async fn unrepresentable_native_facts_fail_closed_even_when_caught() { + for seconds in [false, true] { + let mut tool = TypedFailureTool::new(false); + let ToolError::Diagnostic(failure) = &mut tool.error else { + panic!() + }; + failure.metadata = failure + .metadata + .clone() + .with_retry(agentkit_core::retry::ProviderFailure { + route: agentkit_core::retry::ProviderRoute::Unknown, + reason: agentkit_core::retry::ProviderFailureReason::Cancelled, + last_attempt_reason: None, + upstream: agentkit_core::retry::ProviderClassification::default(), + accounting: agentkit_core::retry::RetryAccounting { + attempts: if seconds { 1 } else { i64::MAX as u64 + 1 }, + elapsed: if seconds { + std::time::Duration::MAX + } else { + std::time::Duration::ZERO + }, + completed_backoff: std::time::Duration::ZERO, + }, + }) + .unwrap(); + let result = execute_compose( + ComposeConfig::default(), + tool, + request( + "return boundary { return typed_failure({}) } catch err { return true }", + json!(null), + ), + ) + .await; + assert!( + matches!(result, ToolExecutionOutcome::Failed(ToolError::Internal(message)) if message == "compose failure facts exceed Runlet integer range") + ); + } +} diff --git a/crates/agentkit-tool-fs/src/lib.rs b/crates/agentkit-tool-fs/src/lib.rs index c905fde..7b211b1 100644 --- a/crates/agentkit-tool-fs/src/lib.rs +++ b/crates/agentkit-tool-fs/src/lib.rs @@ -1495,6 +1495,7 @@ mod tests { resources: &'a dyn ToolResources, ) -> ToolContext<'a> { ToolContext { + failure_observer: None, capability: CapabilityContext { session_id: Some(session_id), turn_id: Some(turn_id), diff --git a/crates/agentkit-tool-shell/src/lib.rs b/crates/agentkit-tool-shell/src/lib.rs index 51efd0b..6dda5a0 100644 --- a/crates/agentkit-tool-shell/src/lib.rs +++ b/crates/agentkit-tool-shell/src/lib.rs @@ -342,6 +342,7 @@ mod tests { let executor = BasicToolExecutor::from_registry(registry()); let metadata = MetadataMap::new(); let mut ctx = ToolContext { + failure_observer: None, capability: CapabilityContext { session_id: Some(&SessionId::new("session-1")), turn_id: Some(&TurnId::new("turn-1")), @@ -389,6 +390,7 @@ mod tests { let executor = BasicToolExecutor::from_registry(registry()); let metadata = MetadataMap::new(); let mut ctx = ToolContext { + failure_observer: None, capability: CapabilityContext { session_id: Some(&SessionId::new("session-1")), turn_id: Some(&TurnId::new("turn-1")), diff --git a/crates/agentkit-tool-skills/src/lib.rs b/crates/agentkit-tool-skills/src/lib.rs index 2028cea..69326fe 100644 --- a/crates/agentkit-tool-skills/src/lib.rs +++ b/crates/agentkit-tool-skills/src/lib.rs @@ -954,6 +954,7 @@ mod tests { let noop_perms = NoopPermissions; let mut ctx = ToolContext { + failure_observer: None, capability: agentkit_capabilities::CapabilityContext { session_id: None, turn_id: None, @@ -1017,6 +1018,7 @@ mod tests { let noop_perms = NoopPermissions; let mut ctx = ToolContext { + failure_observer: None, capability: agentkit_capabilities::CapabilityContext { session_id: None, turn_id: None, @@ -1178,6 +1180,7 @@ mod tests { let tool = reg.build_tool(); let noop_perms = NoopPermissions; let mut ctx = ToolContext { + failure_observer: None, capability: agentkit_capabilities::CapabilityContext { session_id: None, turn_id: None, diff --git a/crates/agentkit-tools-core/src/failure_observation.rs b/crates/agentkit-tools-core/src/failure_observation.rs new file mode 100644 index 0000000..a7de419 --- /dev/null +++ b/crates/agentkit-tools-core/src/failure_observation.rs @@ -0,0 +1,135 @@ +//! Invocation-scoped latest-value observations. No queue, globals, or metadata trust. +use agentkit_core::failure::{FailureObservations, HostFatalReceipt, PossibleEffects}; +use agentkit_core::retry::ProviderFailure; +use std::sync::{Arc, Mutex, MutexGuard}; + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum ObservationUpdate { + Published, + Unchanged, +} +#[derive(Clone, Copy, Debug, PartialEq, Eq, thiserror::Error)] +pub enum ObservationPublishError { + #[error("failure observations are sealed")] + Sealed, + #[error("invalid failure observation")] + Invalid, + #[error("conflicting failure observation")] + Conflict, + #[error("regressive failure observation")] + Regression, +} +#[derive(Default)] +struct State { + sealed: bool, + value: FailureObservations, +} + +/// Host controller. Create one per invocation; sealing freezes all producer clones. +#[derive(Clone, Default)] +pub struct FailureObservationSlot(Arc>); +/// Trusted host producer handle, not serializable and never accepted from metadata. +#[derive(Clone)] +pub struct FailureObservationPublisher(Arc>); + +fn lock(state: &Mutex) -> MutexGuard<'_, State> { + // No user callbacks execute under this lock. If a host unwinds, keep the last + // accepted value rather than silently losing observations on cancellation. + state + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) +} +fn snapshot(state: &State) -> Option { + (!state.value.is_empty()).then(|| state.value.clone()) +} +impl FailureObservationSlot { + pub fn new() -> Self { + Self::default() + } + pub fn publisher(&self) -> FailureObservationPublisher { + FailureObservationPublisher(self.0.clone()) + } + pub fn snapshot(&self) -> Option { + snapshot(&lock(&self.0)) + } + pub fn seal(&self) -> Option { + let mut state = lock(&self.0); + state.sealed = true; + snapshot(&state) + } +} +impl FailureObservationPublisher { + pub fn publish_effects( + &self, + value: PossibleEffects, + ) -> Result { + if !value.observation_incomplete() { + return Err(ObservationPublishError::Invalid); + } + let mut state = lock(&self.0); + if state.sealed { + return Err(ObservationPublishError::Sealed); + } + if let Some(old) = state.value.effects() { + if old == &value { + return Ok(ObservationUpdate::Unchanged); + } + if old.source != value.source { + return Err(ObservationPublishError::Conflict); + } + if (old.assistant_output_observed && !value.assistant_output_observed) + || (old.tool_emission_observed && !value.tool_emission_observed) + || (old.tool_execution_start_reported && !value.tool_execution_start_reported) + || (old.tool_execution_completion_reported + && !value.tool_execution_completion_reported) + { + return Err(ObservationPublishError::Regression); + } + } + state.value = state.value.clone().with_effects(value); + Ok(ObservationUpdate::Published) + } + pub fn publish_receipt( + &self, + value: HostFatalReceipt, + ) -> Result { + let mut state = lock(&self.0); + if state.sealed { + return Err(ObservationPublishError::Sealed); + } + if let Some(old) = state.value.receipt() { + return if old == &value { + Ok(ObservationUpdate::Unchanged) + } else { + Err(ObservationPublishError::Conflict) + }; + } + state.value = state.value.clone().with_receipt(value); + Ok(ObservationUpdate::Published) + } + pub fn publish_retry( + &self, + value: ProviderFailure, + ) -> Result { + FailureObservations::default() + .with_retry(value) + .map_err(|_| ObservationPublishError::Invalid)?; + let mut state = lock(&self.0); + if state.sealed { + return Err(ObservationPublishError::Sealed); + } + if let Some(old) = state.value.retry() { + return if old == &value { + Ok(ObservationUpdate::Unchanged) + } else { + Err(ObservationPublishError::Conflict) + }; + } + state.value = state + .value + .clone() + .with_retry(value) + .map_err(|_| ObservationPublishError::Invalid)?; + Ok(ObservationUpdate::Published) + } +} diff --git a/crates/agentkit-tools-core/src/lib.rs b/crates/agentkit-tools-core/src/lib.rs index 501cec3..3f2f477 100644 --- a/crates/agentkit-tools-core/src/lib.rs +++ b/crates/agentkit-tools-core/src/lib.rs @@ -18,6 +18,11 @@ //! - **Bridge to the capability layer** with [`ToolCapabilityProvider`], //! which wraps every registered tool as an [`Invocable`]. +mod failure_observation; +pub use failure_observation::{ + FailureObservationPublisher, FailureObservationSlot, ObservationPublishError, ObservationUpdate, +}; + use std::any::Any; use std::collections::{BTreeMap, BTreeSet}; use std::fmt; @@ -501,6 +506,8 @@ impl ToolResources for () { /// permission checker, shared resources, and a cancellation signal so the /// tool can abort long-running work when a turn is cancelled. pub struct ToolContext<'a> { + /// Per-invocation host publisher; never inherited by nested execution scopes. + pub failure_observer: Option, /// Capability-layer context carrying session and turn identifiers. pub capability: CapabilityContext<'a>, /// The active permission checker for sub-operations the tool may perform. @@ -536,6 +543,7 @@ impl ToolExecutionScope { /// Creates an owned tool context for a nested tool call. pub fn nested_context(&self, metadata: MetadataMap) -> OwnedToolContext { OwnedToolContext { + failure_observer: None, session_id: self.session_id.clone(), turn_id: self.turn_id.clone(), metadata, @@ -574,6 +582,8 @@ impl ToolExecutionScope { /// [`ToolContext`] expected by existing tool implementations. #[derive(Clone)] pub struct OwnedToolContext { + /// Installed fresh by task managers; clones within this invocation share it. + pub failure_observer: Option, /// Session identifier for the invocation. pub session_id: SessionId, /// Turn identifier for the invocation. @@ -596,6 +606,7 @@ impl OwnedToolContext { /// Creates a borrowed [`ToolContext`] view over this owned context. pub fn borrowed(&self) -> ToolContext<'_> { ToolContext { + failure_observer: self.failure_observer.clone(), capability: CapabilityContext { session_id: Some(&self.session_id), turn_id: Some(&self.turn_id), @@ -3185,6 +3196,7 @@ impl Invocable for ToolInvocableAdapter { } let mut tool_ctx = ToolContext { + failure_observer: None, capability: CapabilityContext { session_id: ctx.session_id, turn_id: ctx.turn_id, @@ -3590,12 +3602,97 @@ impl ToolExecutor for BasicToolExecutor { } } +/// Finite failure classification, independent of provider retry policy. +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum ToolFailureKind { + NotFound, + InvalidInput, + PermissionDenied, + ExecutionFailed, + Unavailable, + Cancelled, + Internal, +} + +/// Sanitized diagnostics intentionally cannot represent a retryable Unavailable. +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum DiagnosticFailureKind { + ExecutionFailed, + Internal, + Cancelled, +} + +/// A nonrecursive failure carrying only validated facts, never a raw message. +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Error)] +#[error("tool diagnostic failure ({kind:?})")] +pub struct DiagnosticToolFailure { + pub kind: DiagnosticFailureKind, + pub metadata: agentkit_core::failure::FailureMetadataV1, +} + +/// Host-owned terminal projection, including cancellation without diagnostics. +#[derive(Clone, Debug, PartialEq, Eq, Serialize)] +pub struct ToolFailureInfo { + pub kind: ToolFailureKind, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub metadata: Option, +} + +impl<'de> Deserialize<'de> for DiagnosticToolFailure { + fn deserialize>(deserializer: D) -> Result { + #[derive(Deserialize)] + #[serde(deny_unknown_fields)] + struct Wire { + kind: DiagnosticFailureKind, + metadata: agentkit_core::failure::FailureMetadataV1, + } + let wire = Wire::deserialize(deserializer).map_err(|_| { + serde::de::Error::custom(agentkit_core::failure::FailureMetadataDecodeError) + })?; + Ok(Self { + kind: wire.kind, + metadata: wire.metadata, + }) + } +} +impl<'de> Deserialize<'de> for ToolFailureInfo { + fn deserialize>(deserializer: D) -> Result { + #[derive(Deserialize)] + #[serde(deny_unknown_fields)] + struct Wire { + kind: ToolFailureKind, + metadata: Option, + } + let wire = Wire::deserialize(deserializer).map_err(|_| { + serde::de::Error::custom(agentkit_core::failure::FailureMetadataDecodeError) + })?; + Ok(Self { + kind: wire.kind, + metadata: wire.metadata, + }) + } +} + +impl ToolFailureInfo { + /// Validate a parsed terminal projection without retaining unknown input. + pub fn from_value( + value: &Value, + ) -> Result { + Self::deserialize(value).map_err(|_| agentkit_core::failure::FailureMetadataDecodeError) + } +} + /// Errors that can occur during tool lookup, permission checking, or execution. /// /// Returned from [`Tool::invoke`] and also used internally by /// [`BasicToolExecutor`] to represent lookup and permission failures. #[derive(Debug, Error, Clone, PartialEq, Serialize, Deserialize)] pub enum ToolError { + /// Payload-free diagnostic failure; retry observations never authorize replay. + #[error("{0}")] + Diagnostic(Box), /// No tool with the given name exists in the registry. #[error("tool not found: {0}")] NotFound(ToolName), @@ -3620,6 +3717,43 @@ pub enum ToolError { } impl ToolError { + pub fn diagnostic(failure: DiagnosticToolFailure) -> Self { + Self::Diagnostic(Box::new(failure)) + } + pub fn is_cancelled(&self) -> bool { + self.failure_kind() == ToolFailureKind::Cancelled + } + pub fn is_permission_denied(&self) -> bool { + self.failure_kind() == ToolFailureKind::PermissionDenied + } + pub fn failure_kind(&self) -> ToolFailureKind { + match self { + Self::NotFound(_) => ToolFailureKind::NotFound, + Self::InvalidInput(_) => ToolFailureKind::InvalidInput, + Self::PermissionDenied(_) => ToolFailureKind::PermissionDenied, + Self::ExecutionFailed(_) => ToolFailureKind::ExecutionFailed, + Self::Unavailable(_) => ToolFailureKind::Unavailable, + Self::Cancelled => ToolFailureKind::Cancelled, + Self::Internal(_) => ToolFailureKind::Internal, + Self::Diagnostic(failure) => match failure.kind { + DiagnosticFailureKind::ExecutionFailed => ToolFailureKind::ExecutionFailed, + DiagnosticFailureKind::Internal => ToolFailureKind::Internal, + DiagnosticFailureKind::Cancelled => ToolFailureKind::Cancelled, + }, + } + } + pub fn failure_metadata(&self) -> Option<&agentkit_core::failure::FailureMetadataV1> { + match self { + Self::Diagnostic(failure) => Some(&failure.metadata), + _ => None, + } + } + pub fn failure_info(&self) -> ToolFailureInfo { + ToolFailureInfo { + kind: self.failure_kind(), + metadata: self.failure_metadata().cloned(), + } + } /// Convenience constructor for the [`PermissionDenied`](ToolError::PermissionDenied) variant. pub fn permission_denied(denial: PermissionDenial) -> Self { Self::PermissionDenied(denial) @@ -3632,6 +3766,13 @@ impl From for ToolError { } } +impl ToolContext<'_> { + /// Clone the invocation-scoped producer for host callbacks that outlive a borrow. + pub fn failure_observer(&self) -> Option { + self.failure_observer.clone() + } +} + #[cfg(test)] mod tests { use super::*; @@ -4100,6 +4241,7 @@ mod tests { // ...but the inner tool must see its own name in the request. let owned = OwnedToolContext { + failure_observer: None, session_id: SessionId::new("s"), turn_id: TurnId::new("t"), metadata: MetadataMap::new(), @@ -4289,6 +4431,7 @@ mod tests { fn test_context() -> OwnedToolContext { OwnedToolContext { + failure_observer: None, session_id: SessionId::new("s"), turn_id: TurnId::new("t"), metadata: MetadataMap::new(), @@ -4315,6 +4458,7 @@ mod tests { cancellation: None, }; OwnedToolContext { + failure_observer: None, session_id, turn_id, metadata, diff --git a/crates/agentkit-tools-core/tests/failure.rs b/crates/agentkit-tools-core/tests/failure.rs new file mode 100644 index 0000000..dd2b953 --- /dev/null +++ b/crates/agentkit-tools-core/tests/failure.rs @@ -0,0 +1,91 @@ +use agentkit_core::failure::{FailureCode, FailureMetadataV1}; +use agentkit_tools_core::{ + DiagnosticFailureKind, DiagnosticToolFailure, ToolError, ToolExecutionOutcome, ToolFailureKind, +}; +use serde_json::json; + +#[test] +fn historical_error_shapes_remain_readable_and_unchanged() { + for value in [ + json!("Cancelled"), + json!({"NotFound":"missing"}), + json!({"InvalidInput":"old"}), + json!({"ExecutionFailed":"old"}), + json!({"Unavailable":"old"}), + json!({"Internal":"old"}), + ] { + let error: ToolError = serde_json::from_value(value.clone()).unwrap(); + assert_eq!(serde_json::to_value(error).unwrap(), value); + } +} + +#[test] +fn diagnostic_failure_and_cancellation_are_native_and_payload_free() { + for kind in [ + DiagnosticFailureKind::ExecutionFailed, + DiagnosticFailureKind::Internal, + DiagnosticFailureKind::Cancelled, + ] { + let error = ToolError::diagnostic(DiagnosticToolFailure { + kind, + metadata: FailureMetadataV1::new(FailureCode::ChildFailed), + }); + assert_eq!( + error.is_cancelled(), + kind == DiagnosticFailureKind::Cancelled + ); + assert_eq!( + error.failure_metadata().unwrap().code(), + FailureCode::ChildFailed + ); + assert!(!error.is_permission_denied()); + let outcome = ToolExecutionOutcome::Failed(error.clone()); + let encoded = serde_json::to_value(&outcome).unwrap(); + assert_eq!( + serde_json::from_value::(encoded).unwrap(), + outcome + ); + assert_eq!( + error.failure_info().metadata, + error.failure_metadata().cloned() + ); + assert!(!error.to_string().contains("session")); + } + assert!(ToolError::Cancelled.is_cancelled()); + assert_eq!( + ToolError::ExecutionFailed("legacy".into()).failure_kind(), + ToolFailureKind::ExecutionFailed + ); + assert_eq!( + ToolError::Unavailable("legacy".into()) + .failure_info() + .metadata, + None + ); +} + +#[test] +fn diagnostic_schema_rejects_recursive_and_raw_errors() { + for value in [ + json!({"Diagnostic":{"kind":"execution_failed","metadata":{"version":1},"source":"PRIVATE"}}), + json!({"Diagnostic":{"kind":"unavailable","metadata":{"version":1}}}), + json!({"Diagnostic":{"kind":"cancelled","metadata":{"version":99}}}), + json!({"Diagnostic":{"kind":"internal","metadata":{"version":1,"cause":{"Internal":"PRIVATE"}}}}), + ] { + assert!(serde_json::from_value::(value).is_err()); + } +} + +#[test] +fn new_diagnostic_decode_errors_never_echo_unknown_fields_or_kinds() { + for value in [ + json!({"kind":"PRIVATE_KIND","metadata":{"version":1}}), + json!({"kind":"cancelled","metadata":{"version":1},"PRIVATE_FIELD":"PRIVATE_BODY"}), + ] { + let error = serde_json::from_value::(value.clone()).unwrap_err(); + assert!(!error.to_string().contains("PRIVATE")); + let error = + serde_json::from_value::(value).unwrap_err(); + assert!(!error.to_string().contains("PRIVATE")); + } +} diff --git a/crates/agentkit-tools-core/tests/failure_observation.rs b/crates/agentkit-tools-core/tests/failure_observation.rs new file mode 100644 index 0000000..3fd94f6 --- /dev/null +++ b/crates/agentkit-tools-core/tests/failure_observation.rs @@ -0,0 +1,117 @@ +use agentkit_core::{failure::*, retry::*}; +use agentkit_tools_core::{ + FailureObservationSlot, ObservationPublishError as Error, ObservationUpdate as Update, +}; + +fn receipt(id: &str) -> HostFatalReceipt { + HostFatalReceipt { + session_id: HostReceiptId::new("session").unwrap(), + event_id: HostReceiptId::new(id).unwrap(), + storage: FatalStorage::Unavailable, + } +} +fn retry() -> ProviderFailure { + ProviderFailure { + route: ProviderRoute::Unknown, + reason: ProviderFailureReason::Cancelled, + last_attempt_reason: None, + upstream: ProviderClassification::default(), + accounting: RetryAccounting::default(), + } +} +#[test] +fn independent_leaves_survive_updates_and_seal() { + let slot = FailureObservationSlot::new(); + let publisher = slot.publisher(); + assert!(slot.snapshot().is_none()); + let mut effects = PossibleEffects::default(); + effects.source = ObservationSource::LocalSession; + effects.tool_execution_start_reported = true; + assert_eq!(publisher.publish_effects(effects), Ok(Update::Published)); + assert_eq!( + publisher.publish_receipt(receipt("e-1")), + Ok(Update::Published) + ); + assert_eq!(publisher.publish_retry(retry()), Ok(Update::Published)); + effects.tool_execution_completion_reported = true; + assert_eq!(publisher.publish_effects(effects), Ok(Update::Published)); + let frozen = slot.seal().unwrap(); + assert_eq!(frozen.effects(), Some(&effects)); + assert_eq!(frozen.receipt(), Some(&receipt("e-1"))); + assert_eq!(frozen.retry(), Some(&retry())); + assert_eq!(slot.seal(), Some(frozen.clone())); + assert_eq!(publisher.publish_effects(effects), Err(Error::Sealed)); + assert_eq!(publisher.publish_retry(retry()), Err(Error::Sealed)); + assert_eq!( + publisher.publish_receipt(receipt("e-1")), + Err(Error::Sealed) + ); + let bytes = serde_json::to_vec(&frozen).unwrap(); + assert_eq!(FailureObservations::from_slice(&bytes).unwrap(), frozen); +} +#[test] +fn conflicts_regressions_and_invalid_values_do_not_mutate() { + let slot = FailureObservationSlot::new(); + let publisher = slot.publisher(); + let mut effects = PossibleEffects::default(); + effects.source = ObservationSource::LocalSession; + effects.tool_execution_start_reported = true; + publisher.publish_effects(effects).unwrap(); + assert_eq!(publisher.publish_effects(effects), Ok(Update::Unchanged)); + let mut regression = effects; + regression.tool_execution_start_reported = false; + assert_eq!( + publisher.publish_effects(regression), + Err(Error::Regression) + ); + regression = effects; + regression.source = ObservationSource::AcpNotifications; + assert_eq!(publisher.publish_effects(regression), Err(Error::Conflict)); + publisher.publish_receipt(receipt("e-1")).unwrap(); + assert_eq!( + publisher.publish_receipt(receipt("e-1")), + Ok(Update::Unchanged) + ); + assert_eq!( + publisher.publish_receipt(receipt("e-2")), + Err(Error::Conflict) + ); + publisher.publish_retry(retry()).unwrap(); + let mut other = retry(); + other.accounting.attempts = 1; + assert_eq!(publisher.publish_retry(other), Err(Error::Conflict)); + other.upstream.http_status = Some(900); + assert_eq!(publisher.publish_retry(other), Err(Error::Invalid)); + let frozen = slot.snapshot().unwrap(); + assert_eq!(frozen.effects(), Some(&effects)); + assert_eq!(frozen.receipt(), Some(&receipt("e-1"))); + assert_eq!(frozen.retry(), Some(&retry())); +} +#[test] +fn publication_and_sealing_are_linearized_without_cross_task_state() { + let first = FailureObservationSlot::new(); + let second = FailureObservationSlot::new(); + let publisher = first.publisher(); + let thread = std::thread::spawn(move || publisher.publish_receipt(receipt("e-1"))); + thread.join().unwrap().unwrap(); + assert!(first.seal().unwrap().receipt().is_some()); + assert!(second.snapshot().is_none()); + second.seal(); + let publisher = second.publisher(); + assert_eq!( + std::thread::spawn(move || publisher.publish_receipt(receipt("e-2"))) + .join() + .unwrap(), + Err(Error::Sealed) + ); +} +#[test] +fn observation_wire_is_closed_at_nested_boundaries() { + for value in [ + serde_json::json!({"effects":{"replay_safe":true}}), + serde_json::json!({"receipt":{"session_id":"s","event_id":"e","storage":"unavailable","path":"PRIVATE"}}), + serde_json::json!({"metadata":{"version":1}}), + ] { + assert!(FailureObservations::from_slice(&serde_json::to_vec(&value).unwrap()).is_err()); + } +} diff --git a/crates/agentkit-tools-derive/tests/macro_integration.rs b/crates/agentkit-tools-derive/tests/macro_integration.rs index d10862a..0d787cc 100644 --- a/crates/agentkit-tools-derive/tests/macro_integration.rs +++ b/crates/agentkit-tools-derive/tests/macro_integration.rs @@ -55,6 +55,7 @@ fn build_request(tool_name: &str, input: serde_json::Value) -> ToolRequest { fn ctx() -> OwnedToolContext { OwnedToolContext { + failure_observer: None, session_id: SessionId::new("s"), turn_id: TurnId::new("t"), metadata: MetadataMap::new(), diff --git a/docs/typed-failure-transport.md b/docs/typed-failure-transport.md new file mode 100644 index 0000000..663c65e --- /dev/null +++ b/docs/typed-failure-transport.md @@ -0,0 +1,86 @@ +# Typed failure transport + +AgentKit transports diagnostic facts as real tool failures, not successful values containing an error object. Retry observations describe what happened; they never authorize replay of a child invocation. + +## Values and compatibility + +The canonical retry values now live in `agentkit_core::retry`. The existing `agentkit_loop` root exports refer to those exact types; `RetryObserver` remains loop-owned. Existing enum spellings and `Duration` serde objects (`secs`, `nanos`) are unchanged. + +`agentkit_core::failure::FailureMetadataV1` is a closed version-1 value with a local `FailureCode` and optional retry, host fatal receipt, and `PossibleEffects` leaves. Missing leaves mean unknown, not zero attempts or no effects. Construction validates HTTP status; the new envelope's reader also closes every nested retry object and duration without changing standalone retry serde. + +Use `FailureMetadataV1::from_slice` before parsing untrusted transport bytes. It enforces the 4096-byte budget and returns a static validation error. Unsupported versions, unknown local enum variants or fields, malformed counts, invalid durations, and claims of complete effects observation are rejected. Unknown provider type/code spellings normalize to `UpstreamErrorKind::Unknown`; the original spelling is not retained. The fixed object layout bounds depth. The wire contains no provider message, URL, path, arbitrary map, recursive cause, or raw error source. + +`HostReceiptId` validates 1–128 ASCII letters, digits, underscores, or hyphens. This is grammar validation, **not authentication**. Hosts must issue receipt IDs and validate external envelopes against the expected child/session route. Allocate a fatal receipt once and retain that same identity across projections: + +- `Stored`: the store operation succeeded at emission time, not a promise of eternal availability. +- `MemoryOnly`: an actual retrievable in-memory record exists. +- `Unavailable`: no record is retained, including a failed storage operation. + +Never infer storage success from allocation, reconstruct a receipt from display text, or replace a child's receipt with a parent's local diagnostic. + +`ToolError::Diagnostic(Box)` has static display text and an `ExecutionFailed`, `Internal`, or `Cancelled` kind. The box bounds the enum size, not a recursive cause chain. It deliberately cannot represent retryable `Unavailable`. Use `ToolError::is_cancelled`, `is_permission_denied`, `failure_kind`, `failure_metadata`, and `failure_info` instead of matching only legacy variants. + +All legacy `ToolError` serde variants remain readable and retain their previous output shapes and messages. There is no artifact rewrite or migration of old strings into trusted facts. Older exhaustive enum consumers/readers must be updated before they receive the new Diagnostic variant. The new context/snapshot fields also require updating Rust struct literals; `TurnTaskUpdate::Detached` now boxes its snapshot to bound enum size. `LoopDriver::cancel_pending_approval_for` is now async so cancellation awaits the manager-owned terminal transition; callers must await it. This change does not publish crates or bump versions. + +## Compose catch and rethrow + +Uncaught native failures preserve the exact original `ToolError` in both backends. A child `Completed` result with `is_error = true` is rejected as a generic failure and is not cached as successful nested execution. Producers must return `Failed(error)` for typed transport. + +Runlet 0.5 flattens host details into the caught error: + +```text +return boundary { + return child({}) +} catch err { + return fail(err.code, err.message, { + agentkit_failure_token: err.agentkit_failure_token + }) +} +``` + +`err.agentkit_failure` is the closed advisory projection (`kind`, optional `metadata`). Runlet has signed integers. If a counter or duration cannot be represented exactly, the bridge reports a static, nonretryable internal range failure, issues no capability for that error, and cannot be recovered into success by a catch. Canonical durable values keep their full range; no rounded or saturated observation is published. The projection is never parsed back into an authoritative failure. + +The explicit third argument forwards an ephemeral, cryptographically random capability. The per-run bridge restores only the saved native error when its issued token and original code/message all match. Forwarding the full `err` object is also explicit capability forwarding; supplied facts cannot replace saved metadata. + +- `fail(err.code, err.message)` creates a **new generic execution failure**, dropping native classification and facts. +- A changed code/message, invented token, consumed token, or token from another run cannot restore metadata. +- A caught capability may intentionally rethrow its original error later in the same run. It does not identify a different concurrent failure. +- A catch that returns a value explicitly handles the failure; no native facts are implicitly attached to that successful result. +- Native diagnostic failures and cancellation are not implicitly retried, regardless of provider retry facts. Legacy `Unavailable` retry behavior is unchanged. + +Do not persist tokens. The bridge retains at most 4096 failures per run, does not overwrite on token collision, and releases the table at run end. Entropy failure, collision, capacity exhaustion, or an unrepresentable fact latches a nonretryable host internal failure rather than evicting live identities or allowing a catch to hide broken transport. Tokens are redacted from runtime error rendering. The optional, no-extra-feature `getrandom` dependency is enabled only with Runlet; Lua remains the default backend. + +Lua retains its native external error identity: `pcall` followed by `error(err)` preserves diagnostics; stringifying and recreating an error does not. + +## Observations that survive cancellation + +A task manager installs a fresh `FailureObservationSlot` publisher into its own `OwnedToolContext` copy. A host producer obtains it with `ToolContext::failure_observer()`. Borrowed contexts and callbacks within that invocation can clone the publisher. Nested execution scopes do **not** inherit it; nested tasks receive their own fresh slot. Request metadata is never a publisher or an observation source. + +The controller retains one bounded latest value, not an event queue. Publishers independently set final receipt, final retry summary, and effects snapshot: + +- Receipt and retry are write-once. Identical republishing is unchanged; a different value conflicts. Retry progress belongs to `RetryObserver`, not this slot. +- Effects may advance only monotonically with the same source. Clearing a true flag or changing source is rejected without mutation. There is no cross-source OR/merge. +- `LocalSession` covers the live owner's cumulative observations, not earlier persisted history or only the failing prompt. `AcpNotifications` covers reports during the selected child prompt. Completion is existential, not proof that every tool finished, committed, or rolled back. +- `seal()` freezes all producer clones. Publications accepted before sealing survive cancellation; later publications fail with `Sealed`. No publication means unknown. + +A task's native child error and its local observations can describe different scopes. They remain two separately labeled, bounded projections: + +| Surface | Native terminal facts | Task-owned frozen facts | +| --- | --- | --- | +| `TaskSnapshot` | `failure` | `failure_observations` | +| `ToolResultPart.metadata` | `agentkit.tool.failure` | `agentkit.tool.failure_observations` | +| Typed reader | `tool_failure_info` | `task_failure_observations` | + +Failed outcomes emit `TaskEvent::Failed`; cancellation emits `TaskEvent::Cancelled` with the typed snapshot. Foreground delivery, background loop updates, manual ready items, and detached notifications retain the same selected facts. Actual success remains Completed and gets no failure-only fields. Cancellation never fabricates a receipt, retry exhaustion, or completion observation. + +Cancellation and executor completion select a single terminal result under the task lock. The slot seals before a winning abort. Repeated cancellation cannot reclassify completed work. Launch identities prevent a stale approved generation's completion or detach timer from modifying a replacement task. Before executor admission, cancellation may truthfully mark `not_started`; task scheduling itself never publishes an execution-start observation. + +The loop drains `TaskManager::take_interrupted_task_updates(session_id, call_ids)` before synthesizing results for remaining unanswered calls, preserving observed foreground cancellation facts without a second result. Queued updates retain their originating session and task IDs instead of reconstructing ownership from potentially colliding turn/call IDs. Custom task-manager wrappers that retain tasks must delegate that scoped drain and the async `close_suspended_task` and `take_terminal_task_result` methods. Individual approval denial/cancellation terminalizes the suspended task and transfers its frozen result directly; an already-selected cancellation wins over a later denial or approval. If an approved continuation is rejected because the task already completed, the loop transfers its actual queued terminal result before considering a synthetic start error. Terminal-result transfer never closes a live or suspended task. Unsurfaced background approvals retain their normal loop/manual delivery destination during interruption; surfaced approvals close through the driver-owned interruption drain. Approved continuation verifies immutable session/turn/call/tool and approval identity while permitting host-approved input patches, preserves delivery policy, and seeds a fresh publisher from previously frozen facts; old publishers remain sealed. `list_suspended` distinguishes approval from terminal completion. Inline invocation owners also seal and retain a cancellation projection when their future is dropped. + +The host strips `agentkit.tool.failure`, `agentkit.tool.failure_observations`, `agentkit.tool.failure_kind`, and `agentkit.tool.not_started` from request/context-derived snapshots and successful result/item metadata before writing authoritative fields. Unrelated application metadata is preserved. Typed readers validate shape, not the provenance of arbitrary persisted JSON. + +## Downstream release boundary + +Compatible published AgentKit crates and downstream exhaustive-reader updates are still required. Kit must validate both ACP protocol envelopes and expected routing, wire its host producers to invocation-scoped publishers, allocate/store a fatal receipt once, and use cancellation helpers throughout child/subagent/wrapper handling. This upstream implementation does not install those Kit producers, release either project, or establish replay safety. No production Cargo patches are required or included. + +At untrusted boundaries, use the bounded metadata/observations entrypoints and the static-error terminal projectors. Do not deserialize raw leaf enums or arbitrary legacy `ToolError` blobs from provider/child data and log their serde errors: standalone leaf/legacy serde is not a redaction boundary. The new Diagnostic and ToolFailureInfo wrappers normalize their own direct serde errors. Host-approved input patching remains supported during a verified logical-call continuation; a routing policy may intentionally reroute that continuation while delivery and continue policy are retained.