diff --git a/crates/openspine-kernel/src/api/connector_breaker.rs b/crates/openspine-kernel/src/api/connector_breaker.rs index 92c2e5aa..32701ea8 100644 --- a/crates/openspine-kernel/src/api/connector_breaker.rs +++ b/crates/openspine-kernel/src/api/connector_breaker.rs @@ -298,20 +298,26 @@ fn map_admission_error( /// Map a resolved (non-timeout) write outcome to a dispatch error. /// -/// Candidate Gmail-write extension: a `gmail.create_draft` whose effect is not -/// confirmed by the returned value must retain durable pending evidence unless -/// the provider returns a *confirmed* response. A transport/no-response class -/// may mean the write landed before the response was lost, so it surfaces as -/// [`DispatchError::DeliveryUnknown`] rather than a confirmed failure. Only a -/// response the provider explicitly reports as failed (e.g. a definite `api` -/// error) resolves the pending row. +/// A `gmail.create_draft` whose effect is not confirmed by the returned value +/// must retain durable pending evidence unless the provider response proves +/// non-occurrence. Transport/no-response, malformed success responses, HTTP +/// 429, and HTTP 5xx may all mean the write landed before the response was +/// lost or substituted, so they surface as [`DispatchError::DeliveryUnknown`]. +/// A definite client rejection (HTTP 4xx other than 429) is a confirmed +/// failure and resolves the pending row. fn map_write_error(err: anyhow::Error) -> DispatchError { if let Some(gmail) = err.downcast_ref::() { - if matches!( - gmail.class, - crate::gmail::GmailFailureClass::Transport - | crate::gmail::GmailFailureClass::MalformedResponse - ) { + let ambiguous_api_status = matches!( + (gmail.class, gmail.status), + (crate::gmail::GmailFailureClass::Api, Some(429 | 500..=599)) + ); + if ambiguous_api_status + || matches!( + gmail.class, + crate::gmail::GmailFailureClass::Transport + | crate::gmail::GmailFailureClass::MalformedResponse + ) + { return DispatchError::DeliveryUnknown(anyhow!( "gmail write outcome is unconfirmed (delivery-unknown): {gmail}" )); @@ -343,11 +349,48 @@ mod tests { } #[test] - fn confirmed_gmail_api_write_failure_is_connector_error() { + fn gmail_5xx_write_is_delivery_unknown() { + // #173: a 5xx from drafts.create does not prove the write failed — the + // draft may have landed before the intermediary errored — so it is + // delivery-unknown, not a confirmed failure. + for status in [500u16, 502, 503, 504] { + let err = map_write_error(anyhow::Error::new(crate::gmail::GmailError { + status: Some(status), + class: crate::gmail::GmailFailureClass::Api, + })); + assert!( + matches!(err, DispatchError::DeliveryUnknown(_)), + "status {status} must be delivery-unknown, got {err:?}" + ); + } + } + + #[test] + fn gmail_429_write_is_delivery_unknown() { + // #173: a 429 (rate limited) likewise does not prove non-occurrence. let err = map_write_error(anyhow::Error::new(crate::gmail::GmailError { - status: Some(500), + status: Some(429), class: crate::gmail::GmailFailureClass::Api, })); - assert!(matches!(err, DispatchError::Connector(_))); + assert!( + matches!(err, DispatchError::DeliveryUnknown(_)), + "got {err:?}" + ); + } + + #[test] + fn confirmed_gmail_4xx_write_failure_is_connector_error() { + // A definite client rejection (4xx other than 429) proves the write + // did not take hold and resolves the pending row. + for status in [400u16, 403, 422] { + let err = map_write_error(anyhow::Error::new(crate::gmail::GmailError { + status: Some(status), + class: crate::gmail::GmailFailureClass::Api, + })); + assert!( + matches!(err, DispatchError::Connector(_)), + "status {status} must be a confirmed connector failure, got {err:?}" + ); + } } } diff --git a/crates/openspine-kernel/src/api/scoped_admission.rs b/crates/openspine-kernel/src/api/scoped_admission.rs index 96e48e1a..9b1d97e6 100644 --- a/crates/openspine-kernel/src/api/scoped_admission.rs +++ b/crates/openspine-kernel/src/api/scoped_admission.rs @@ -542,6 +542,9 @@ pub(crate) async fn dispatch_scoped_effect( #[path = "scoped_admission_drift_tests.rs"] mod drift_tests; #[cfg(test)] +#[path = "scoped_admission_effect_truth_tests.rs"] +mod effect_truth_tests; +#[cfg(test)] #[path = "scoped_admission_outcome_tests.rs"] mod outcome_tests; #[cfg(test)] diff --git a/crates/openspine-kernel/src/api/scoped_admission_effect_truth_tests.rs b/crates/openspine-kernel/src/api/scoped_admission_effect_truth_tests.rs new file mode 100644 index 00000000..8b3ea982 --- /dev/null +++ b/crates/openspine-kernel/src/api/scoped_admission_effect_truth_tests.rs @@ -0,0 +1,60 @@ +//! Effect Truth regressions for scope-matched standing-rule settlement. + +use jiff::Timestamp; +use openspine_schemas::action::ActionId; +use serde_json::json; + +use super::scoped_admission_support::*; +use crate::api::actions::{mediate_and_dispatch_action, FailureSurface}; + +/// #173 on the delegated path: a provider 5xx is delivery-unknown, so the +/// scoped reservation is RETAINED (not cancelled) and the fence stays open. +/// A 5xx was previously collapsed to a confirmed failure, cancelling the +/// reservation and releasing reviewed budget for a write that may have landed. +#[tokio::test] +async fn scoped_provider_5xx_retains_reservation_and_leaves_fence_open() { + let env = draft_env(&["thread-1"]).await; + mount_drafts(&env.api_server, 502, json!({"error": {"code": 502}})).await; + let grant = mint_draft_grant(&env.state, "thread-1"); + let context = resolved_context(&env.state, &grant).await; + env.state + .store + .activate_standing_rule( + &scoped_manifest("rule-5xx", &context), + None, + Timestamp::now(), + ) + .unwrap(); + + let result = mediate_and_dispatch_action( + &env.state, + &grant, + ActionId::new("email.create_draft"), + &crate::test_support::telegram_surface(CHAT_ID), + Some(&draft_payload()), + FailureSurface::DirectResponse, + None, + ) + .await; + + assert!( + result.is_err(), + "an unconfirmed 5xx write is never reported as a success" + ); + assert_eq!(usage_count(&env.state, "rule-5xx", "reserved"), 1); + assert_eq!(usage_count(&env.state, "rule-5xx", "committed"), 0); + assert_eq!( + env.state + .store + .standing_rule_remaining("rule-5xx", Timestamp::now()) + .unwrap(), + (4, 2), + "a retained reservation keeps consuming quota and rate" + ); + assert_eq!( + env.state.store.count_pending_draft_writes().unwrap(), + 1, + "the reconciliation fence stays open on a 5xx" + ); + assert!(audit_count(&env.state, "draft.delivery_unknown") >= 1); +} diff --git a/crates/openspine-kernel/src/api/scoped_admission_outcome_tests.rs b/crates/openspine-kernel/src/api/scoped_admission_outcome_tests.rs index 1d823923..d50dc782 100644 --- a/crates/openspine-kernel/src/api/scoped_admission_outcome_tests.rs +++ b/crates/openspine-kernel/src/api/scoped_admission_outcome_tests.rs @@ -152,7 +152,12 @@ async fn pending_delivery_unknown_fences_scoped_retry_before_reservation() { #[tokio::test] async fn failure_after_attempt_cancels_reservation() { let env = draft_env(&["thread-1"]).await; - mount_drafts(&env.api_server, 500, json!({"error": "boom"})).await; + mount_drafts( + &env.api_server, + 400, + json!({"error": {"code": 400, "message": "invalid draft"}}), + ) + .await; let grant = mint_draft_grant(&env.state, "thread-1"); let context = resolved_context(&env.state, &grant).await; env.state diff --git a/crates/openspine-kernel/src/pipeline/approval_draft.rs b/crates/openspine-kernel/src/pipeline/approval_draft.rs index db843499..90da4a67 100644 --- a/crates/openspine-kernel/src/pipeline/approval_draft.rs +++ b/crates/openspine-kernel/src/pipeline/approval_draft.rs @@ -16,11 +16,15 @@ use openspine_schemas::owner_surface::OwnerSurfaceRef; /// unexpired approval. Re-derives the recipient from a live Gmail fetch and /// re-checks it against the proposal-bound digest before calling /// `create_draft`, because a new thread message can change the recipient. -/// Candidate Gmail-write extension: a write timeout/no-response leaves -/// durable pending evidence for manual/operator reconciliation; no automatic -/// resend is safe without Gmail idempotency. -/// Returns a truthful outcome distinguishing pre-effect refusal, confirmed -/// execution, delivery uncertainty, and failure after an attempted write. +/// Provider-boundary contract: connector admission is taken before the pending +/// fence and before polling any provider future. A rejected admission is +/// [`EffectOutcome::RefusedPreEffect`]. Once the write future is polled, a +/// confirmed Gmail success is [`EffectOutcome::Executed`], a definite client +/// rejection is [`EffectOutcome::FailedAfterAttempt`], and an outcome that does +/// not prove non-occurrence (timeout, transport/malformed response, HTTP 429, +/// or HTTP 5xx) is [`EffectOutcome::DeliveryUnknown`] with durable pending +/// evidence left open for reconciliation. No automatic resend is safe without +/// Gmail idempotency. pub(crate) async fn create_approved_draft( state: &AppState, grant: &TaskGrant, @@ -92,7 +96,9 @@ pub(crate) async fn create_approved_draft( .await { Ok(thread) => thread, - Err(DispatchError::ConnectorUnavailable(err)) => return Err(err), + Err(DispatchError::ConnectorUnavailable(_)) => { + return Ok(EffectOutcome::RefusedPreEffect); + } Err(err) => { state.store.append_audit( "draft.creation_failed", @@ -199,7 +205,9 @@ pub(crate) async fn create_approved_draft( grant, ) { Ok(permit) => permit, - Err(DispatchError::ConnectorUnavailable(err)) => return Err(err), + Err(DispatchError::ConnectorUnavailable(_)) => { + return Ok(EffectOutcome::RefusedPreEffect); + } Err(err) => { state.store.append_audit( "draft.creation_failed", @@ -303,10 +311,6 @@ pub(crate) async fn create_approved_draft( notify_owner_best_effort(state, owner_surface, "Draft created in Gmail.").await; Ok(EffectOutcome::Executed) } - Err(DispatchError::ConnectorUnavailable(err)) => { - state.store.resolve_pending_draft_write(pending_id)?; - Err(err) - } Err(DispatchError::DeliveryUnknown(_)) => unreachable!("handled above"), Err(err) => { state.store.resolve_pending_draft_write(pending_id)?; diff --git a/crates/openspine-kernel/src/pipeline/tests/approval_draft_admission_tests.rs b/crates/openspine-kernel/src/pipeline/tests/approval_draft_admission_tests.rs index c99e2767..577668f7 100644 --- a/crates/openspine-kernel/src/pipeline/tests/approval_draft_admission_tests.rs +++ b/crates/openspine-kernel/src/pipeline/tests/approval_draft_admission_tests.rs @@ -151,3 +151,75 @@ async fn definite_write_failure_is_failed_after_attempt_and_resolves_the_fence() "the fence was recorded before the attempted write" ); } + +#[tokio::test] +async fn provider_5xx_write_is_delivery_unknown_and_leaves_the_fence_open() { + // #173: a 502 from drafts.create is NOT a confirmed failure — the draft + // may have been created before the intermediary answered — so the outcome + // is `DeliveryUnknown`, the pending-write fence stays `pending` (never + // resolved), and no retry is auto-sent. Previously a 5xx was collapsed to + // `FailedAfterAttempt`, resolving the fence and permitting a duplicate + // provider write on the delegated retry path. + let token_server = MockServer::start().await; + let api_server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/gmail/v1/users/me/threads/thread-1")) + .respond_with( + ResponseTemplate::new(200).set_body_json(thread_with_sender("alice@example.com")), + ) + .mount(&api_server) + .await; + Mock::given(method("POST")) + .and(path("/gmail/v1/users/me/drafts")) + .respond_with(ResponseTemplate::new(502).set_body_json(json!({ + "error": {"code": 502, "message": "bad gateway"} + }))) + .mount(&api_server) + .await; + let gmail = gmail_with_token_mock(&token_server, &api_server).await; + let state = test_state_with_gmail(gmail); + let grant = approval_fixture_grant(); + let request = approval_fixture_request( + &state, + grant.id, + "Re: invoice", + "sounds good", + "alice@example.com", + ); + + let outcome = crate::pipeline::approval::create_approved_draft( + &state, + &grant, + &request, + &crate::test_support::owner_surface(&state), + ) + .await + .unwrap(); + + assert_eq!(outcome, EffectOutcome::DeliveryUnknown); + assert_eq!( + state + .store + .count_audit_events_of_kind("draft.delivery_unknown") + .unwrap(), + 1 + ); + assert_eq!( + state + .store + .count_audit_events_of_kind("draft.creation_failed") + .unwrap(), + 0, + "a 5xx is never a confirmed creation failure" + ); + assert_eq!( + state.store.count_pending_draft_writes().unwrap(), + 1, + "the reconciliation fence stays open on an unconfirmed write" + ); + assert_eq!( + total_pending_draft_write_rows(&state), + 1, + "the fence was recorded before the attempted write and left pending" + ); +} diff --git a/crates/openspine-kernel/src/pipeline/tests/approval_draft_reconcile_tests.rs b/crates/openspine-kernel/src/pipeline/tests/approval_draft_reconcile_tests.rs index aadff791..a7083951 100644 --- a/crates/openspine-kernel/src/pipeline/tests/approval_draft_reconcile_tests.rs +++ b/crates/openspine-kernel/src/pipeline/tests/approval_draft_reconcile_tests.rs @@ -331,13 +331,13 @@ pub(crate) fn total_pending_draft_write_rows(state: &AppState) -> i64 { } #[tokio::test] -async fn unavailable_gmail_connector_refuses_before_any_fence_row() { - // An Open breaker blocks the executor at its FIRST connector call — the - // live thread fetch — so it exits before the write is ever admitted and no - // pending-write fence row is recorded. Deliberately scoped to that: it - // cannot pin write-admission ordering, because a breaker open enough to - // reject the write also rejects the preceding fetch. The write-admission - // ordering is pinned by the rate-limit test below. +async fn unavailable_gmail_connector_is_a_pre_effect_refusal_without_a_fence_row() { + // #174: an Open breaker blocks the executor at its FIRST connector call — + // the live thread fetch — before polling any provider future. The executor + // must preserve that known ordering as `RefusedPreEffect`; a bare error is + // collapsed to Resource by the scoped caller and permanently retains the + // reservation. This test cannot pin the later write-admission arm because + // an Open breaker necessarily rejects the preceding fetch first. let token_server = MockServer::start().await; let api_server = MockServer::start().await; // No mocks mounted: the Open breaker must block before any Gmail call. @@ -356,23 +356,29 @@ async fn unavailable_gmail_connector_refuses_before_any_fence_row() { state.connectors.record_connector_outcome("gmail", false); } - let result = crate::pipeline::approval::create_approved_draft( + let outcome = crate::pipeline::approval::create_approved_draft( &state, &grant, &request, &crate::test_support::owner_surface(&state), ) - .await; + .await + .expect("pre-effect connector rejection is a typed refusal, not an executor error"); - assert!( - result.is_err(), - "an unavailable gmail connector propagates as an error, not an outcome: {result:?}" - ); + assert_eq!(outcome, EffectOutcome::RefusedPreEffect); assert_eq!( total_pending_draft_write_rows(&state), 0, "refusing at the thread fetch must record no pending-write fence" ); + assert_eq!( + state + .store + .count_audit_events_of_kind("connector_unavailable") + .unwrap(), + 1, + "admission records exactly one connector_unavailable audit" + ); assert_eq!( state .store