Edits keep an item's other attributes; a failed keyring read stops the sync

Saving an edit replaced the item's whole attribute set with the form's
three fields (set_attributes replaces). On a 1Password-paired item that
dropped op-item and op-vault, so the next sync saw a new keyring item plus
an untouched remote one. It created a bare duplicate in 1Password and
archived the original with its one-time-code seed and custom fields. On
another app's item it dropped xdg:schema and the app's lookup attributes.
update_item now reads the current attributes first and lays the form's
fields over them; a field cleared in the form is removed, nothing else is.

The sync snapshot turned every failed keyring read into a default: an
unreadable lock state meant unlocked, a failed password or title read an
empty string, a failed modified time 0. A keyring that locked or restarted
mid-pass therefore read as every item blanked here, and the plan pushed
those blanks to 1Password as edits. An item whose attributes failed to
read was skipped, which read as deleted and archived its 1Password copy.
Every such read now ends the pass with an error before any plan is
applied, and the daemon retries next tick. adopt fails the same way rather
than pairing an item under a blank password.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

 src/bin/cce-keyring-sync/adopt.rs | 30 +++++++++++++---
 src/bin/cce-keyring-sync/sync.rs  | 30 +++++++++++++---
 src/main.rs                       | 74 +++++++++++++++++++++++++++++++++++++--
 3 files changed, 121 insertions(+), 13 deletions(-)

diff --git a/src/bin/cce-keyring-sync/adopt.rs b/src/bin/cce-keyring-sync/adopt.rs
index 173c228..d3ca018 100644
--- a/src/bin/cce-keyring-sync/adopt.rs
+++ b/src/bin/cce-keyring-sync/adopt.rs
@@ -181,7 +181,17 @@ pub async fn adopt(state_path: &std::path::Path, mut state: State, vault: &str,
             std::process::exit(1);
         }
     };
-    if col.is_locked().await.unwrap_or(false) && col.unlock().await.is_err() {
+    // As in `sync_remote`: a read the keyring cannot answer stops the run
+    // rather than standing in as an empty value, which would pair an item
+    // under a blank password.
+    let locked = match col.is_locked().await {
+        Ok(l) => l,
+        Err(e) => {
+            eprintln!("cannot tell whether the keyring is locked: {e}");
+            std::process::exit(1);
+        }
+    };
+    if locked && col.unlock().await.is_err() {
         eprintln!("collection locked");
         std::process::exit(1);
     }
@@ -191,21 +201,31 @@ pub async fn adopt(state_path: &std::path::Path, mut state: State, vault: &str,
     match col.get_all_items().await {
         Ok(items) => {
             for item in items {
-                let Ok(attrs) = item.get_attributes().await else { continue };
+                let attrs = item.get_attributes().await.unwrap_or_else(|e| {
+                    eprintln!("reading an item's attributes failed: {e}; nothing adopted");
+                    std::process::exit(1);
+                });
                 if attrs.get("application").map(String::as_str) == Some(APP) {
                     continue;
                 }
                 if !attrs.contains_key("UserName") && !attrs.contains_key("kdbx-uuid") {
                     continue;
                 }
+                let unreadable = |what: &str, e: &dyn std::fmt::Display| -> ! {
+                    eprintln!("reading the {what} of a keyring login failed: {e}; nothing adopted");
+                    std::process::exit(1);
+                };
+                let title = item.get_label().await.unwrap_or_else(|e| unreadable("title", &e));
+                let secret = item.get_secret().await.unwrap_or_else(|e| unreadable("password", &e));
+                let modified = item.get_modified().await.unwrap_or_else(|e| unreadable("modified time", &e));
                 let entry = KrEntry {
-                    title: item.get_label().await.unwrap_or_default(),
+                    title,
                     username: attrs.get("UserName").cloned().unwrap_or_default(),
-                    password: String::from_utf8_lossy(&item.get_secret().await.unwrap_or_default()).into_owned(),
+                    password: String::from_utf8_lossy(&secret).into_owned(),
                     url: attrs.get("URL").cloned().unwrap_or_default(),
                     notes: attrs.get("Notes").cloned().unwrap_or_default(),
                     group: String::new(), // becomes the vault name once paired
-                    modified: item.get_modified().await.unwrap_or(0),
+                    modified,
                 };
                 locals.push(Local { item, attrs, entry });
             }
diff --git a/src/bin/cce-keyring-sync/sync.rs b/src/bin/cce-keyring-sync/sync.rs
index 4dfeb64..945bd0c 100644
--- a/src/bin/cce-keyring-sync/sync.rs
+++ b/src/bin/cce-keyring-sync/sync.rs
@@ -169,7 +169,17 @@ pub async fn sync_remote<I: Interchange>(
         _ => return Err("no state hash key — run `cce-keyring-sync adopt` first".into()),
     };
     let col = ss.get_default_collection().await.map_err(|e| format!("no default collection: {e}"))?;
-    if col.is_locked().await.unwrap_or(false) && col.unlock().await.is_err() {
+    // A read the keyring could not answer must stop the pass, never stand
+    // in as an empty value. Until 2026-10-02 every read below fell back to a
+    // default — a lock state of "unlocked", an empty password and title, a
+    // modified time of 0 — so a keyring that locked or restarted mid-pass
+    // read as every item having been blanked here, which the plan then
+    // pushed to 1Password as edits (and other machines pulled back). An item
+    // whose attributes failed to read was skipped, which read as deleted and
+    // archived its 1Password copy. A pass that errs writes nothing: the
+    // snapshot is taken before any plan is applied, and the daemon retries.
+    let locked = col.is_locked().await.map_err(|e| format!("cannot tell whether the keyring is locked: {e}"))?;
+    if locked && col.unlock().await.is_err() {
         return Err("collection locked".into());
     }
 
@@ -177,7 +187,10 @@ pub async fn sync_remote<I: Interchange>(
     let mut kr: HashMap<String, Local<'_>> = HashMap::new();
     let mut born: Vec<Local<'_>> = Vec::new();
     for item in col.get_all_items().await.map_err(|e| format!("listing collection failed: {e}"))? {
-        let Ok(attrs) = item.get_attributes().await else { continue };
+        let attrs = item
+            .get_attributes()
+            .await
+            .map_err(|e| format!("reading an item's attributes failed: {e}; nothing synced this pass"))?;
         if attrs.get("application").map(String::as_str) == Some(APP) {
             continue;
         }
@@ -185,14 +198,21 @@ pub async fn sync_remote<I: Interchange>(
         if id.is_none() && !attrs.contains_key("UserName") && !attrs.contains_key("kdbx-uuid") {
             continue; // some other app's item — never ours to sync
         }
+        let unreadable = |what: &str, e: &dyn std::fmt::Display| {
+            let which = attrs.get(OP_ITEM_ATTR).map(String::as_str).unwrap_or("an unpaired item");
+            format!("reading the {what} of {which} failed: {e}; nothing synced this pass")
+        };
+        let title = item.get_label().await.map_err(|e| unreadable("title", &e))?;
+        let secret = item.get_secret().await.map_err(|e| unreadable("password", &e))?;
+        let modified = item.get_modified().await.map_err(|e| unreadable("modified time", &e))?;
         let entry = KrEntry {
-            title: item.get_label().await.unwrap_or_default(),
+            title,
             username: attrs.get("UserName").cloned().unwrap_or_default(),
-            password: String::from_utf8_lossy(&item.get_secret().await.unwrap_or_default()).into_owned(),
+            password: String::from_utf8_lossy(&secret).into_owned(),
             url: attrs.get("URL").cloned().unwrap_or_default(),
             notes: attrs.get("Notes").cloned().unwrap_or_default(),
             group: attrs.get(OP_VAULT_ATTR).cloned().unwrap_or_else(|| vault.clone()),
-            modified: item.get_modified().await.unwrap_or(0),
+            modified,
         };
         let local = Local { item, attrs, entry };
         match id {
diff --git a/src/main.rs b/src/main.rs
index a1350e5..f88cf7a 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -502,6 +502,31 @@ async fn create_item(
     Ok(())
 }
 
+/// What an edit leaves on an item: its current attributes with the form's
+/// fields laid over them. A field the form left empty is removed; every
+/// attribute the form does not show is kept as it is.
+///
+/// Until 2026-10-02 a save wrote the form's fields ALONE, and the Secret
+/// Service's `set_attributes` replaces the whole set. On a 1Password-paired
+/// item that dropped `op-item` / `op-vault`, so the next sync saw a new
+/// keyring item and an untouched remote one: it created a bare duplicate in
+/// 1Password and archived the original with its one-time-code seed and
+/// custom fields. On another app's item ("Chrome Safe Storage") it dropped
+/// `xdg:schema` and the lookup attributes the app finds its secret by.
+fn merge_edited_attributes(
+    mut current: std::collections::HashMap<String, String>,
+    edits: &[(String, String)],
+) -> std::collections::HashMap<String, String> {
+    for (key, value) in edits {
+        if value.is_empty() {
+            current.remove(key);
+        } else {
+            current.insert(key.clone(), value.clone());
+        }
+    }
+    current
+}
+
 async fn update_item(
     ss: &SecretService<'_>,
     tx: &calloop::channel::Sender<AppMessage>,
@@ -512,10 +537,17 @@ async fn update_item(
 ) -> Result<(), String> {
     let item = resolve_item(ss, &path).await?;
     let _ = item.ensure_unlocked().await;
+    // Read before anything is written: `set_attributes` replaces the whole
+    // set, so without the current attributes there is nothing safe to save.
+    let current = item
+        .get_attributes()
+        .await
+        .map_err(|e| format!("Reading the item's attributes failed: {e}"))?;
     item.set_label(&label)
         .await
         .map_err(|e| format!("Saving label failed: {e}"))?;
-    let attr_map = attrs.iter().map(|(k, v)| (k.as_str(), v.as_str())).collect();
+    let merged = merge_edited_attributes(current, &attrs);
+    let attr_map = merged.iter().map(|(k, v)| (k.as_str(), v.as_str())).collect();
     item.set_attributes(attr_map)
         .await
         .map_err(|e| format!("Saving attributes failed: {e}"))?;
@@ -775,10 +807,11 @@ impl SecretsApp {
         }
         let values = [&self.user_box, &self.url_box, &self.notes_box]
             .map(|b| live_text(b).trim().to_string());
+        // Every edited field, empty ones included: an update removes what was
+        // cleared (`merge_edited_attributes`); a new item just skips them.
         let attrs: Vec<(String, String)> = EDIT_ATTRS
             .iter()
             .zip(values)
-            .filter(|(_, v)| !v.is_empty())
             .map(|(k, v)| (k.to_string(), v))
             .collect();
         let password = live_text(&self.pass_box).to_string();
@@ -790,7 +823,11 @@ impl SecretsApp {
                 attrs,
                 secret: (!password.is_empty()).then_some(password),
             },
-            None => Cmd::CreateItem { label: title, attrs, secret: password },
+            None => Cmd::CreateItem {
+                label: title,
+                attrs: attrs.into_iter().filter(|(_, v)| !v.is_empty()).collect(),
+                secret: password,
+            },
         })
     }
 
@@ -1553,6 +1590,37 @@ fn main() {
 mod tests {
     use super::*;
 
+    #[test]
+    fn an_edit_keeps_every_attribute_the_form_does_not_show() {
+        let current: std::collections::HashMap<String, String> = [
+            ("op-item", "abc123"),
+            ("op-vault", "Personal"),
+            ("xdg:schema", "org.freedesktop.Secret.Generic"),
+            ("UserName", "old@example.org"),
+            ("URL", "https://old.example.org"),
+            ("Notes", "keep me"),
+        ]
+        .into_iter()
+        .map(|(k, v)| (k.to_string(), v.to_string()))
+        .collect();
+        let edits = vec![
+            ("UserName".to_string(), "new@example.org".to_string()),
+            ("URL".to_string(), String::new()),
+            ("Notes".to_string(), "keep me".to_string()),
+        ];
+        let merged = merge_edited_attributes(current, &edits);
+        // The pairing and the schema survive: losing them is what duplicated
+        // and archived 1Password items.
+        assert_eq!(merged.get("op-item").map(String::as_str), Some("abc123"));
+        assert_eq!(merged.get("op-vault").map(String::as_str), Some("Personal"));
+        assert_eq!(merged.get("xdg:schema").map(String::as_str), Some("org.freedesktop.Secret.Generic"));
+        // The form's fields are what it says: changed, cleared, unchanged.
+        assert_eq!(merged.get("UserName").map(String::as_str), Some("new@example.org"));
+        assert!(!merged.contains_key("URL"), "a cleared field is removed");
+        assert_eq!(merged.get("Notes").map(String::as_str), Some("keep me"));
+        assert_eq!(merged.len(), 5);
+    }
+
     #[test]
     fn daemon_replies_parse() {
         assert_eq!(parse_otp_reply("otp 123456 17"), OtpReply::Code("123456".into(), 17));