From be880f50206479f2b562b0485c7d710f20b6446d Mon Sep 17 00:00:00 2001 From: Bhekani Khumalo Date: Wed, 10 Jun 2026 21:04:25 +0100 Subject: [PATCH] feat: tell users when a mutation's undo could not be recorded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When an undoable mutation (archive/trash/spam/...) committed but writing its undo entry failed, the response's `mutation_id` came back `None` — indistinguishable from a mutation that simply isn't undoable. The undo affordance vanished silently. Add an `undo_unavailable` flag to `MutationResultData` (serialized only when true, so existing consumers are unaffected). The handler sets it on the undo-write failure path; the CLI prints a note that undo is unavailable, and the JSON contract carries the flag so agents can tell "undo failed" apart from "not undoable by design". --- crates/daemon/src/chimes.rs | 1 + .../daemon/src/commands/mutations/helpers.rs | 5 ++++ crates/daemon/src/handler/mutations.rs | 18 +++++++---- crates/protocol/src/lib.rs | 30 +++++++++++++++++++ crates/protocol/src/types.rs | 6 ++++ crates/tui/src/runner/tests/mailbox_views.rs | 1 + crates/web/src/tests.rs | 1 + .../docs/guides/automation-contract.md | 1 + 8 files changed, 57 insertions(+), 6 deletions(-) diff --git a/crates/daemon/src/chimes.rs b/crates/daemon/src/chimes.rs index d88f50ff..d50d8827 100644 --- a/crates/daemon/src/chimes.rs +++ b/crates/daemon/src/chimes.rs @@ -272,6 +272,7 @@ mod tests { error: None, }], mutation_id: None, + undo_unavailable: false, } } } diff --git a/crates/daemon/src/commands/mutations/helpers.rs b/crates/daemon/src/commands/mutations/helpers.rs index 48b20bce..4cd9859e 100644 --- a/crates/daemon/src/commands/mutations/helpers.rs +++ b/crates/daemon/src/commands/mutations/helpers.rs @@ -640,6 +640,11 @@ fn print_mutation_result_output( println!("{}", counts.changed_line(result.succeeded as usize)); if let Some(mutation_id) = result.mutation_id.as_deref() { println!("Undo with: mxr undo {mutation_id}"); + } else if result.undo_unavailable { + eprintln!( + "Note: undo is unavailable for this mutation — recording the undo entry failed, \ + so it can't be reversed with `mxr undo`." + ); } } OutputFormat::Json => println!( diff --git a/crates/daemon/src/handler/mutations.rs b/crates/daemon/src/handler/mutations.rs index 36d7f60d..d3fbba45 100644 --- a/crates/daemon/src/handler/mutations.rs +++ b/crates/daemon/src/handler/mutations.rs @@ -546,6 +546,7 @@ pub(super) async fn mutation( failed: 0, accounts, mutation_id: None, + undo_unavailable: false, }, }); } @@ -637,7 +638,7 @@ pub(super) async fn mutation( // Persist an undo entry only for undoable kinds and only if at // least one envelope succeeded. The mutation_id is the same one // used for dedup above; undo + dedup share a key by design. - let mutation_id = match (undoable_kind, succeeded_snapshots.is_empty()) { + let (mutation_id, undo_unavailable) = match (undoable_kind, succeeded_snapshots.is_empty()) { (Some(kind), false) => { let now = chrono::Utc::now().timestamp(); let entry = UndoEntry { @@ -648,15 +649,16 @@ pub(super) async fn mutation( expires_at: now + UNDO_WINDOW_SECS, }; if let Err(error) = state.store.write_undo_entry(&entry).await { - // Non-fatal: the mutation already succeeded; the user - // just loses the undo affordance. + // Non-fatal: the mutation already succeeded. The user + // loses the undo affordance, but flag it so clients can + // say so rather than silently dropping undo. tracing::warn!(%error, "failed to write undo entry"); - None + (None, true) } else { - Some(mutation_id) + (Some(mutation_id), false) } } - _ => None, + _ => (None, false), }; let result = MutationResultData { @@ -666,6 +668,7 @@ pub(super) async fn mutation( failed, accounts, mutation_id, + undo_unavailable, }; emit_mutation_reconciliation_failed_if_needed(state, client_correlation_id, &result); @@ -888,6 +891,7 @@ fn empty_mutation_result(requested: u32) -> MutationResultData { failed: 0, accounts: Vec::new(), mutation_id: None, + undo_unavailable: false, } } @@ -2710,6 +2714,7 @@ mod reconciliation_failed_emit_tests { failed: 0, accounts: vec![], mutation_id: None, + undo_unavailable: false, } } @@ -2750,6 +2755,7 @@ mod reconciliation_failed_emit_tests { failed: 0, accounts: vec![], mutation_id: None, + undo_unavailable: false, }; emit_mutation_reconciliation_failed_if_needed(&state, Some("1"), &result); assert!( diff --git a/crates/protocol/src/lib.rs b/crates/protocol/src/lib.rs index a49b663d..df5aefae 100644 --- a/crates/protocol/src/lib.rs +++ b/crates/protocol/src/lib.rs @@ -16,6 +16,35 @@ mod tests { use super::*; use bytes::BytesMut; + + #[test] + fn mutation_result_undo_unavailable_serializes_only_when_set() { + let mut result = MutationResultData { + requested: 1, + succeeded: 1, + skipped: 0, + failed: 0, + accounts: vec![], + mutation_id: None, + undo_unavailable: false, + }; + // Default (false) is omitted from the wire, so existing + // consumers see no new field. + let json = serde_json::to_value(&result).unwrap(); + assert!(json.get("undo_unavailable").is_none()); + + // Set to true, it appears so clients can warn the user. + result.undo_unavailable = true; + let json = serde_json::to_value(&result).unwrap(); + assert_eq!(json.get("undo_unavailable"), Some(&serde_json::json!(true))); + + // And it round-trips back through a missing field as false. + let parsed: MutationResultData = + serde_json::from_value(serde_json::json!({"requested":1,"succeeded":1,"skipped":0,"failed":0,"accounts":[]})) + .unwrap(); + assert!(!parsed.undo_unavailable); + } + use mxr_core::id::*; use mxr_core::{ Address, Draft, DraftIntent, ExportFormat, SavedSearch, SearchMode, SemanticProfile, @@ -774,6 +803,7 @@ mod tests { error: None, }], mutation_id: None, + undo_unavailable: false, }, }, IpcCategory::CoreMail, diff --git a/crates/protocol/src/types.rs b/crates/protocol/src/types.rs index ecf4ccdd..4826fb13 100644 --- a/crates/protocol/src/types.rs +++ b/crates/protocol/src/types.rs @@ -1700,6 +1700,12 @@ pub struct MutationResultData { /// `None` for non-undoable mutations (Star, ModifyLabels, Move). #[serde(default, skip_serializing_if = "Option::is_none")] pub mutation_id: Option, + /// True when an undoable mutation succeeded but persisting its undo + /// entry failed, so `mutation_id` is `None` even though undo was + /// expected. Lets clients distinguish "no undo by design" from "undo + /// was lost" and warn the user instead of silently dropping it. + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + pub undo_unavailable: bool, } #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] diff --git a/crates/tui/src/runner/tests/mailbox_views.rs b/crates/tui/src/runner/tests/mailbox_views.rs index 58372b35..bda8f33e 100644 --- a/crates/tui/src/runner/tests/mailbox_views.rs +++ b/crates/tui/src/runner/tests/mailbox_views.rs @@ -858,6 +858,7 @@ fn format_mutation_failure_joins_per_account_errors() { error: None, }], mutation_id: None, + undo_unavailable: false, }; assert_eq!( format_mutation_failure(&bare), diff --git a/crates/web/src/tests.rs b/crates/web/src/tests.rs index 7f20a6af..85196056 100644 --- a/crates/web/src/tests.rs +++ b/crates/web/src/tests.rs @@ -2721,6 +2721,7 @@ async fn read_mutation_endpoint_reports_skipped_result_as_not_ok() { error: Some("account unavailable".into()), }], mutation_id: None, + undo_unavailable: false, }, }, }) diff --git a/site/src/content/docs/guides/automation-contract.md b/site/src/content/docs/guides/automation-contract.md index bfc353ec..0fc36244 100644 --- a/site/src/content/docs/guides/automation-contract.md +++ b/site/src/content/docs/guides/automation-contract.md @@ -128,6 +128,7 @@ mxr archive --search 'from:noreply older_than:30d' --account work --yes - `mxr send DRAFT_ID` is **not idempotent** — calling twice will send twice (the daemon schedules the second send). Always check `mxr drafts list` first. - `mxr unsubscribe` may hit a provider URL once; re-running on an already-unsubscribed message is harmless but emits no useful new state. - `mxr undo MUTATION_ID` works within a 60-second window; after that it returns an error. +- An undoable mutation normally returns a `mutation_id`. If that field is absent **and** the result has `"undo_unavailable": true`, the mutation succeeded but its undo entry could not be recorded — it can't be reversed with `mxr undo`. A plain absent `mutation_id` (no `undo_unavailable`) just means the mutation isn't undoable by design (e.g. star, label, move). ## See also