fix(desktop): stop boot migration and instance saves from clearing keyring secret refs

The boot migration ran already-projected records (empty inline, live ref)
through the strip-on-save seam, which reads an empty inline field as a user
clear and drops the ref — silently wiping every committed secret ref on the
second launch, then GC deletes the orphaned generations. And save_managed_agents
re-read the definition half through the hydrating loader, so every instance-side
save re-inlined definition secrets back into plaintext JSON and froze GC on the
inline+ref conflict.

Give the migration its own field-granular transition (migrate_inline_field ->
FieldMigration) that only projects a non-empty inline value and never clears a
ref it did not write; expose it as a pub(crate) seam so the custom-harness
migration shares one W1-safe semantic. Re-read the definition half RAW under the
txn lock and propagate a parse error with ? instead of unwrap_or_default(), so a
malformed store fails the save rather than deleting every definition. Broaden the
backup recognizer structurally to catch this repo's own pre-backfill.bak and
pre-team-suffix-strip.bak producers.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>

desktop/src-tauri/src/managed_agents/secret_seam.rs

@@ -290,6 +290,175 @@ pub(crate) fn strip_and_persist_all_for_records<S: ProjectionStore>(
     }
 }
 
+// ── Boot-migration transition (W1-safe) ───────────────────────────────────
+//
+// The ordinary save path (`strip_and_persist_*`) treats an empty inline field
+// on an AVAILABLE record as a deliberate user-clear and drops the ref. The
+// boot migration must NOT: it re-reads ALREADY-PROJECTED records straight off
+// disk (empty inline + live ref, and `secrets_unavailable` is `#[serde(skip)]`
+// so always `false`), which the save path would read as a clear and wipe every
+// committed ref on the second launch. This transition is the migration's own
+// contract: project only a non-empty inline value; preserve an existing ref
+// when inline is absent; never infer a clear from a raw projected record.
+
+/// Outcome of one field's boot-migration transition.
+///
+/// The caller applies the record mutation implied by each arm — the transition
+/// itself is coordinate-generic (it does not know which record field it serves)
+/// so it can back the agent tiers here AND the custom-harness surface, which
+/// carries a single `env` map under its own keyring namespace.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub(crate) enum FieldMigration {
+    /// Inline value written and read-back verified under a new generation.
+    /// Caller: clear the inline field and set its `*_ref` to `gen`.
+    Projected { gen: String },
+    /// Inline absent, an existing ref is present. Caller: leave the ref
+    /// untouched — the committed generation stays live.
+    Preserved,
+    /// No projection: inline absent with no existing ref, OR the keyring write
+    /// failed (value kept inline for retry). Caller: leave the field as-is; the
+    /// migration never sets a ref it did not write.
+    Cleared,
+}
+
+/// Field-granular boot-migration transition. Projects `inline` into the keyring
+/// under a fresh generation when present; otherwise preserves `existing_ref`
+/// without ever clearing it.
+///
+/// Distinct from the strip-on-save seam: an absent inline value here means
+/// "already projected on a prior launch," never "the user cleared it," so a
+/// present ref is preserved rather than dropped. This is the seam Hayt's
+/// custom-harness boot migration consumes so the two surfaces share one
+/// W1-safe semantic instead of forking it.
+pub(crate) fn migrate_inline_field<S: ProjectionStore>(
+    store: &S,
+    coord_key_fn: impl Fn(&str) -> String,
+    inline: Option<&str>,
+    existing_ref: Option<&str>,
+    context: &str,
+) -> FieldMigration {
+    match write_secret(store, &coord_key_fn, inline, context) {
+        WriteOutcome::Persisted { gen } => {
+            cancel_gc_candidacy(store, &coord_key_fn(&gen));
+            FieldMigration::Projected { gen }
+        }
+        // Inline absent: `write_secret` attempted nothing. Preserve an existing
+        // ref (already-projected record) instead of treating empty as a clear.
+        WriteOutcome::Nothing if existing_ref.is_some() => FieldMigration::Preserved,
+        WriteOutcome::Nothing | WriteOutcome::KeptInline { .. } => FieldMigration::Cleared,
+    }
+}
+
+/// Boot-migrate one instance record's secret fields at field granularity.
+/// W1-safe: an already-projected record (empty inline, live refs) is left with
+/// its refs intact rather than having them cleared.
+fn migrate_agent_secrets_with<S: ProjectionStore>(store: &S, record: &mut ManagedAgentRecord) {
+    let pubkey = record.pubkey.clone();
+    let inline_env = if !record.env_vars.is_empty() {
+        serialize_env_map(&record.env_vars).ok()
+    } else {
+        None
+    };
+    if let FieldMigration::Projected { gen } = migrate_inline_field(
+        store,
+        |gen| agent_env_key(&pubkey, gen),
+        inline_env.as_deref(),
+        record.env_vars_ref.as_deref(),
+        &format!("agent:{pubkey} env_vars"),
+    ) {
+        record.env_vars.clear();
+        record.env_vars_ref = Some(gen);
+    }
+
+    let auth_val = record.auth_tag.clone();
+    if let FieldMigration::Projected { gen } = migrate_inline_field(
+        store,
+        |gen| agent_auth_tag_key(&pubkey, gen),
+        auth_val.as_deref(),
+        record.auth_tag_ref.as_deref(),
+        &format!("agent:{pubkey} auth_tag"),
+    ) {
+        record.auth_tag = None;
+        record.auth_tag_ref = Some(gen);
+    }
+
+    if let BackendKind::Provider {
+        ref id,
+        ref mut config,
+    } = record.backend
+    {
+        let serialized = if !config.is_null() {
+            serialize_provider_config(config).ok()
+        } else {
+            None
+        };
+        if let FieldMigration::Projected { gen } = migrate_inline_field(
+            store,
+            |gen| agent_provider_config_key(&pubkey, gen),
+            serialized.as_deref(),
+            record.provider_config_ref.as_deref(),
+            &format!("agent:{pubkey} provider_config ({id})"),
+        ) {
+            *config = serde_json::Value::Null;
+            record.provider_config_ref = Some(gen);
+        }
+    }
+}
+
+/// Boot-migrate one definition record's `env_vars` at field granularity.
+/// The definition-tier mirror of [`migrate_agent_secrets_with`].
+fn migrate_definition_secrets_with<S: ProjectionStore>(store: &S, record: &mut ManagedAgentRecord) {
+    let Some(slug) = record.slug.as_deref().map(str::to_string) else {
+        return;
+    };
+    let inline_env = if !record.env_vars.is_empty() {
+        serialize_env_map(&record.env_vars).ok()
+    } else {
+        None
+    };
+    if let FieldMigration::Projected { gen } = migrate_inline_field(
+        store,
+        |gen| definition_env_key(&slug, gen),
+        inline_env.as_deref(),
+        record.env_vars_ref.as_deref(),
+        &format!("definition:{slug} env_vars"),
+    ) {
+        record.env_vars.clear();
+        record.env_vars_ref = Some(gen);
+    }
+}
+
+/// Boot-migrate all secret fields in `records` at field granularity. Returns
+/// `true` when any record's ref set changed (the caller then rewrites JSON).
+pub(crate) fn migrate_all_secrets_for_records<S: ProjectionStore>(
+    store: &S,
+    records: &mut [ManagedAgentRecord],
+) -> bool {
+    let mut changed = false;
+    for record in records.iter_mut() {
+        let before = (
+            record.env_vars_ref.clone(),
+            record.auth_tag_ref.clone(),
+            record.provider_config_ref.clone(),
+        );
+        if record.pubkey.is_empty() {
+            migrate_definition_secrets_with(store, record);
+        } else {
+            migrate_agent_secrets_with(store, record);
+        }
+        if before
+            != (
+                record.env_vars_ref.clone(),
+                record.auth_tag_ref.clone(),
+                record.provider_config_ref.clone(),
+            )
+        {
+            changed = true;
+        }
+    }
+    changed
+}
+
 #[cfg(test)]
 #[path = "secret_seam_tests.rs"]
 mod tests;

desktop/src-tauri/src/managed_agents/secret_seam_tests.rs

@@ -558,3 +558,292 @@ fn test_cleared_conflict_marker_restores_availability_and_allows_spawn() {
         "spawn must be allowed once the conflict is resolved"
     );
 }
+
+// ── W1: boot-migration transition never clears a committed ref ──────────────
+//
+// The strip-on-save seam reads an empty inline field on an available record as
+// a deliberate user-clear and drops the ref. The boot migration re-reads
+// ALREADY-PROJECTED records off disk (empty inline + live ref) on EVERY launch;
+// routing those through the strip seam wiped every committed ref on the second
+// launch (W1). These tests pin the migration seam's distinct contract: project
+// a non-empty inline value, preserve an existing ref when inline is absent, and
+// never clear a ref it did not write.
+
+#[test]
+fn test_migrate_inline_field_projects_nonempty_inline() {
+    let store = FakeProjectionStore::new();
+    let outcome = migrate_inline_field(
+        &store,
+        |gen| agent_env_key("abc", gen),
+        Some(r#"{"K":"v"}"#),
+        None,
+        "agent:abc env_vars",
+    );
+    let gen = match outcome {
+        FieldMigration::Projected { gen } => gen,
+        other => panic!("expected Projected, got {other:?}"),
+    };
+    assert!(
+        store.contains(&agent_env_key("abc", &gen)),
+        "a projected value must be written under its new generation"
+    );
+}
+
+#[test]
+fn test_migrate_inline_field_preserves_ref_when_inline_absent() {
+    // The W1 shape: an already-projected record (no inline, live ref). The
+    // migration must PRESERVE the ref, never treat empty as a clear.
+    let store =
+        FakeProjectionStore::new().with_entry(&agent_env_key("abc", "gen_live"), r#"{"K":"v"}"#);
+    let outcome = migrate_inline_field(
+        &store,
+        |gen| agent_env_key("abc", gen),
+        None,
+        Some("gen_live"),
+        "agent:abc env_vars",
+    );
+    assert_eq!(
+        outcome,
+        FieldMigration::Preserved,
+        "an absent inline with a live ref must preserve, not clear"
+    );
+    assert!(
+        store.contains(&agent_env_key("abc", "gen_live")),
+        "the preserved generation must remain in the keyring"
+    );
+}
+
+#[test]
+fn test_migrate_inline_field_cleared_when_no_inline_and_no_ref() {
+    // Genuinely empty field: nothing inline, no ref. The migration does not
+    // fabricate a ref — Cleared means "leave the field exactly as-is."
+    let store = FakeProjectionStore::new();
+    let outcome = migrate_inline_field(
+        &store,
+        |gen| agent_env_key("abc", gen),
+        None,
+        None,
+        "agent:abc env_vars",
+    );
+    assert_eq!(outcome, FieldMigration::Cleared);
+}
+
+#[test]
+fn test_migrate_two_launches_preserves_every_ref_for_instance_and_definition() {
+    // The end-to-end W1 regression at the seam level: a first launch projects
+    // inline env/auth/provider (instance) + env (definition) into fresh
+    // generations; a second launch over the now-projected records (empty
+    // inline, live refs) is a no-op that leaves EVERY ref intact. The old
+    // strip-seam path cleared them all on launch two.
+    let store = FakeProjectionStore::new();
+
+    let mut instance = instance_record("abc");
+    instance.env_vars = env_map(&[("ANTHROPIC_API_KEY", "sk-secret")]);
+    instance.auth_tag = Some("auth-secret".to_string());
+    instance.backend = BackendKind::Provider {
+        id: "anthropic".to_string(),
+        config: serde_json::json!({"api_key": "provider-secret"}),
+    };
+    let mut definition = definition_record("my-def");
+    definition.env_vars = env_map(&[("DEF_KEY", "def-secret")]);
+    let mut records = vec![instance, definition];
+
+    // Launch 1: inline present → all fields projected.
+    let changed1 = migrate_all_secrets_for_records(&store, &mut records);
+    assert!(changed1, "first launch projects inline secrets");
+    let env_ref = records[0].env_vars_ref.clone().expect("instance env ref");
+    let auth_ref = records[0].auth_tag_ref.clone().expect("instance auth ref");
+    let pc_ref = records[0]
+        .provider_config_ref
+        .clone()
+        .expect("instance provider ref");
+    let def_ref = records[1].env_vars_ref.clone().expect("definition env ref");
+    assert!(
+        records[0].env_vars.is_empty(),
+        "inline env cleared on launch 1"
+    );
+    assert!(
+        records[0].auth_tag.is_none(),
+        "inline auth cleared on launch 1"
+    );
+
+    // Launch 2: records are already projected (empty inline, live refs).
+    let changed2 = migrate_all_secrets_for_records(&store, &mut records);
+    assert!(
+        !changed2,
+        "second launch is a no-op — no ref changes on already-projected records"
+    );
+    assert_eq!(
+        records[0].env_vars_ref,
+        Some(env_ref.clone()),
+        "env ref survives launch 2"
+    );
+    assert_eq!(
+        records[0].auth_tag_ref,
+        Some(auth_ref.clone()),
+        "auth ref survives launch 2"
+    );
+    assert_eq!(
+        records[0].provider_config_ref,
+        Some(pc_ref.clone()),
+        "provider ref survives launch 2"
+    );
+    assert_eq!(
+        records[1].env_vars_ref,
+        Some(def_ref.clone()),
+        "definition ref survives launch 2"
+    );
+
+    // The generations themselves must still be present and hydratable.
+    assert!(store.contains(&agent_env_key("abc", &env_ref)));
+    assert!(store.contains(&agent_auth_tag_key("abc", &auth_ref)));
+    assert!(store.contains(&agent_provider_config_key("abc", &pc_ref)));
+    assert!(store.contains(&definition_env_key("my-def", &def_ref)));
+    let errors = hydrate_all_secrets_for_records(&store, &mut records);
+    assert!(
+        errors.is_empty(),
+        "every ref must still hydrate after two launches: {errors:?}"
+    );
+    assert_eq!(
+        records[0]
+            .env_vars
+            .get("ANTHROPIC_API_KEY")
+            .map(String::as_str),
+        Some("sk-secret")
+    );
+    assert_eq!(
+        records[1].env_vars.get("DEF_KEY").map(String::as_str),
+        Some("def-secret")
+    );
+}
+
+#[test]
+fn test_migrate_does_not_clear_ref_on_keyring_write_failure() {
+    // Distinct from Preserved: an inline value present but the keyring write
+    // fails (KeptInline). The migration must return Cleared (caller leaves the
+    // field as-is, value stays inline for retry) and must NOT set a ref it did
+    // not write. The record keeps its inline value; no ref is fabricated.
+    struct FailingWriteStore;
+    impl ProjectionStore for FailingWriteStore {
+        fn write_and_verify(&self, _key: &str, _value: &str) -> Result<(), String> {
+            Err("simulated keyring write failure".to_string())
+        }
+        fn load_key(&self, _key: &str) -> Result<Option<String>, String> {
+            Ok(None)
+        }
+        fn load_all(&self) -> Result<Option<HashMap<String, String>>, String> {
+            Ok(Some(HashMap::new()))
+        }
+        fn store_batch(&self, _entries: &HashMap<String, String>) -> Result<(), String> {
+            Ok(())
+        }
+        fn remove_batch(&self, _keys: &[&str]) -> Result<(), String> {
+            Ok(())
+        }
+    }
+    let store = FailingWriteStore;
+    let mut record = instance_record("abc");
+    record.env_vars = env_map(&[("K", "v")]);
+    // No pre-existing ref.
+    migrate_agent_secrets_with(&store, &mut record);
+    assert_eq!(
+        record.env_vars,
+        env_map(&[("K", "v")]),
+        "a failed keyring write must keep the value inline for retry"
+    );
+    assert_eq!(
+        record.env_vars_ref, None,
+        "the migration must not fabricate a ref it did not write"
+    );
+}
+
+#[test]
+fn test_migrate_two_launches_survive_both_gc_cycles() {
+    // The full W1 regression through the real GC: after a first launch projects
+    // inline secrets into fresh generations and commits the refs to JSON, a
+    // complete two-cycle GC sweep (delete→mark, twice, the boot order) must NOT
+    // reclaim any of those generations — they are all referenced by live JSON —
+    // and a second launch's migration must leave every ref intact. Before the
+    // W1 fix the second launch cleared the refs, orphaning the generations, and
+    // this GC would then delete them.
+    use crate::managed_agents::secret_projection::{
+        collect_live_refs, delete_gc_candidates, mark_gc_candidates,
+    };
+
+    let store = FakeProjectionStore::new();
+
+    let mut instance = instance_record("abc");
+    instance.env_vars = env_map(&[("ANTHROPIC_API_KEY", "sk-secret")]);
+    instance.auth_tag = Some("auth-secret".to_string());
+    instance.backend = BackendKind::Provider {
+        id: "anthropic".to_string(),
+        config: serde_json::json!({"api_key": "provider-secret"}),
+    };
+    let mut definition = definition_record("my-def");
+    definition.env_vars = env_map(&[("DEF_KEY", "def-secret")]);
+    let mut records = vec![instance, definition];
+
+    // Launch 1: project inline → refs.
+    migrate_all_secrets_for_records(&store, &mut records);
+    let env_ref = records[0].env_vars_ref.clone().expect("env ref");
+    let auth_ref = records[0].auth_tag_ref.clone().expect("auth ref");
+    let pc_ref = records[0]
+        .provider_config_ref
+        .clone()
+        .expect("provider ref");
+    let def_ref = records[1].env_vars_ref.clone().expect("def ref");
+
+    // Commit the projected records to disk exactly as the migration does, so GC
+    // reads the same JSON production would. Global config is empty.
+    let dir = tempfile::tempdir().expect("tempdir");
+    let agents_path = dir.path().join("managed-agents.json");
+    let global_path = dir.path().join("global-agent-config.json");
+    std::fs::write(
+        &agents_path,
+        serde_json::to_string(&records).expect("serialize records"),
+    )
+    .expect("write agents json");
+    std::fs::write(&global_path, "{}").expect("write global json");
+
+    // Sanity: the committed JSON is a clean projection — every ref resolves to
+    // a live coordinate present in the blob, no inline+ref conflict.
+    let agents_json = std::fs::read_to_string(&agents_path).unwrap();
+    let live = collect_live_refs(&agents_json, "{}").expect("clean live refs");
+    for gen in [&env_ref, &auth_ref, &pc_ref, &def_ref] {
+        assert!(live.gen_ids.contains(gen), "{gen} must be a live ref");
+    }
+
+    // Two full boot-order GC cycles (delete before mark) over the committed
+    // JSON. A referenced generation is never a deletion candidate.
+    for _ in 0..2 {
+        delete_gc_candidates(&store, &agents_path, &global_path);
+        mark_gc_candidates(&store, &agents_path, &global_path);
+    }
+
+    // Launch 2: migrate again over the already-projected records (empty inline,
+    // live refs). Must be a no-op that preserves every ref.
+    let changed = migrate_all_secrets_for_records(&store, &mut records);
+    assert!(!changed, "second launch must not change any ref");
+
+    // Every generation must have survived both GC cycles and still hydrate.
+    assert!(store.contains(&agent_env_key("abc", &env_ref)));
+    assert!(store.contains(&agent_auth_tag_key("abc", &auth_ref)));
+    assert!(store.contains(&agent_provider_config_key("abc", &pc_ref)));
+    assert!(store.contains(&definition_env_key("my-def", &def_ref)));
+    let errors = hydrate_all_secrets_for_records(&store, &mut records);
+    assert!(
+        errors.is_empty(),
+        "every ref must still hydrate after two launches + two GC cycles: {errors:?}"
+    );
+    assert_eq!(
+        records[0]
+            .env_vars
+            .get("ANTHROPIC_API_KEY")
+            .map(String::as_str),
+        Some("sk-secret")
+    );
+    assert_eq!(
+        records[1].env_vars.get("DEF_KEY").map(String::as_str),
+        Some("def-secret")
+    );
+}

desktop/src-tauri/src/managed_agents/storage.rs

@@ -192,7 +192,7 @@ enum KeyMigration {
 ///
 /// The single source of truth for the migrate-vs-keep decision, shared by the
 /// load-time opportunistic re-migrate ([`hydrate_keys`]) and the save-time
-/// chokepoint ([`persist_agent_keys`]). An empty key returns
+/// chokepoint ([`persist_agent_keys_with`]). An empty key returns
 /// [`KeyMigration::Nothing`] — never [`KeyMigration::Persisted`], so a record
 /// left empty by a keyring outage is not mistaken for one verified present.
 fn migrate_inline_key(store: &impl KeyStore, record: &ManagedAgentRecord) -> KeyMigration {
@@ -252,13 +252,19 @@ pub(crate) fn spawn_key_refusal(record: &ManagedAgentRecord) -> Option<String> {
 /// Read the raw unified store — keyed instances AND key-less definitions —
 /// with fail-loud parse handling. Internal seam; public readers filter.
 fn load_agent_store(app: &AppHandle) -> Result<Vec<ManagedAgentRecord>, String> {
-    let path = managed_agents_store_path(app)?;
+    load_agent_store_at(&managed_agents_store_path(app)?)
+}
+
+/// Path-based core of [`load_agent_store`]: reads and parses the raw store at
+/// `path`. Split out so the save path and tests can drive the load/split/write
+/// merge against a tempdir without an `AppHandle`.
+fn load_agent_store_at(path: &Path) -> Result<Vec<ManagedAgentRecord>, String> {
     if !path.exists() {
         return Ok(Vec::new());
     }
 
-    let content = fs::read_to_string(&path)
-        .map_err(|error| format!("failed to read agent store: {error}"))?;
+    let content =
+        fs::read_to_string(path).map_err(|error| format!("failed to read agent store: {error}"))?;
     serde_json::from_str(&content).map_err(|error| {
         // Fail loudly and preserve the evidence: a later in-app save rewrites
         // this file wholesale, which would silently destroy a malformed hand
@@ -266,7 +272,7 @@ fn load_agent_store(app: &AppHandle) -> Result<Vec<ManagedAgentRecord>, String>
         // reconcile): the broken content survives as `.invalid` for the user
         // to recover, and the parse error propagates instead of being
         // swallowed into an empty store.
-        backup_invalid_store(&path);
+        backup_invalid_store(path);
         format!("failed to parse agent store (preserved as .invalid): {error}")
     })
 }
@@ -391,7 +397,38 @@ pub fn save_managed_agents(app: &AppHandle, records: &[ManagedAgentRecord]) -> R
     // Lock FIRST — the definition half re-read below must be under the lock
     // (Race 1); see `acquire_secret_txn_lock` for span + residual.
     let _txn = acquire_secret_txn_lock(app)?;
-    let definitions = load_agent_definitions(app).unwrap_or_default();
+    save_managed_agents_at(
+        &managed_agents_store_path(app)?,
+        agent_secret_store(),
+        records,
+    )
+}
+
+/// Path-based core of [`save_managed_agents`]: merges the caller's instance
+/// records with the definition half re-read from disk and commits the unified
+/// store. Split out so the definition-preservation contract can be exercised
+/// over a tempdir without an `AppHandle`.
+///
+/// The definition half is re-read RAW ([`load_agent_store_at`] + retain), NOT
+/// through the hydrating `load_agent_definitions`: a hydrated definition holds
+/// its `env_vars` inline, so writing it back would re-inline the definition's
+/// provider secrets into plaintext JSON on every instance-side save — the exact
+/// regression the projection protocol exists to prevent — and would create the
+/// inline+ref conflict that freezes GC. A raw definition is already stripped on
+/// disk, so it needs no strip pass. The parse error propagates with `?` instead
+/// of collapsing into an empty definition half: the wholesale rewrite below
+/// would otherwise delete every definition from the live store.
+fn save_managed_agents_at<S>(
+    store_path: &Path,
+    store: Option<&S>,
+    records: &[ManagedAgentRecord],
+) -> Result<(), String>
+where
+    S: KeyStore + crate::managed_agents::secret_projection::ProjectionStore,
+{
+    let mut definitions = load_agent_store_at(store_path)?;
+    definitions.retain(|record| record.pubkey.is_empty());
+
     let mut sorted = records.to_vec();
     // A caller-supplied key-less record would collide with the definition
     // half re-read above; instances always carry a pubkey.
@@ -403,16 +440,15 @@ pub fn save_managed_agents(app: &AppHandle, records: &[ManagedAgentRecord]) -> R
             .then_with(|| left.pubkey.cmp(&right.pubkey))
     });
 
-    // Persist each nsec to the keyring; on success blank the inline copy.
-    persist_agent_keys(&mut sorted);
-
-    // Persist env/auth_tag/provider_config to the keyring; on success blank
-    // inline values and set *_ref. On failure keep inline and clear *_ref.
-    if let Some(store) = agent_secret_store() {
+    if let Some(store) = store {
+        // Persist each nsec to the keyring; on success blank the inline copy.
+        persist_agent_keys_with(store, &mut sorted);
+        // Persist env/auth_tag/provider_config to the keyring; on success blank
+        // inline values and set *_ref. On failure keep inline and clear *_ref.
         strip_and_persist_all_for_records(store, &mut sorted);
     }
 
-    write_agent_store(app, definitions, sorted)
+    write_agent_store_at(store_path, definitions, sorted)
 }
 
 /// Save the key-less agent *definitions*, preserving the keyed instances —
@@ -442,6 +478,15 @@ pub(crate) fn save_agent_definitions(
 /// name/pubkey order their save path established.
 fn write_agent_store(
     app: &AppHandle,
+    definitions: Vec<ManagedAgentRecord>,
+    instances: Vec<ManagedAgentRecord>,
+) -> Result<(), String> {
+    write_agent_store_at(&managed_agents_store_path(app)?, definitions, instances)
+}
+
+/// Path-based core of [`write_agent_store`].
+fn write_agent_store_at(
+    path: &Path,
     mut definitions: Vec<ManagedAgentRecord>,
     instances: Vec<ManagedAgentRecord>,
 ) -> Result<(), String> {
@@ -449,7 +494,6 @@ fn write_agent_store(
     let mut all = definitions;
     all.extend(instances);
 
-    let path = managed_agents_store_path(app)?;
     let payload = serde_json::to_vec_pretty(&all)
         .map_err(|error| format!("failed to serialize agent store: {error}"))?;
 
@@ -457,22 +501,13 @@ fn write_agent_store(
     // fallback. Write it owner-only (`0o600`) unconditionally — harmless for the
     // keyring-backed case (it is the user's own agent store) and closes the
     // umask window a post-write `chmod` would leave open.
-    atomic_write_json_restricted(&path, &payload)
+    atomic_write_json_restricted(path, &payload)
 }
 
 /// Write each record's in-memory key to the keyring and blank the inline copy
 /// on success. Keys that cannot be persisted (keyring unreachable) stay inline
 /// in the JSON. Mutates `records` (a save-local clone) — the caller's in-memory
 /// records keep their keys.
-fn persist_agent_keys(records: &mut [ManagedAgentRecord]) {
-    let Some(store) = agent_secret_store() else {
-        // No keyring backend: keys stay inline.
-        return;
-    };
-    persist_agent_keys_with(store, records);
-}
-
-/// Testable core of [`persist_agent_keys`], generic over the [`KeyStore`] seam.
 fn persist_agent_keys_with(store: &impl KeyStore, records: &mut [ManagedAgentRecord]) {
     for record in records.iter_mut() {
         // Only a verified keyring entry lets us drop the inline copy. Both

desktop/src-tauri/src/managed_agents/storage_tests.rs

@@ -973,3 +973,196 @@ fn serialized_store_is_empty_of_secret_values_after_strip() {
         "provider_config_ref must be set after successful strip"
     );
 }
+
+// ── W2: instance-side save preserves the definition half raw ───────────────
+//
+// `save_managed_agents` re-reads the definition half RAW under the txn lock
+// (never through the hydrating loader) before committing the unified store, so
+// an instance-only save can never re-inline a definition's projected secrets
+// into plaintext JSON, and a store parse error propagates instead of silently
+// collapsing the definition half to empty.
+
+/// In-memory store implementing BOTH seams `save_managed_agents_at` requires:
+/// [`KeyStore`] (nsec persistence) and `ProjectionStore` (env/auth/provider
+/// projection). A single backing map so a written secret reads back verified.
+struct FakeCombinedStore {
+    data: RefCell<HashMap<String, String>>,
+}
+
+impl FakeCombinedStore {
+    fn new() -> Self {
+        Self {
+            data: RefCell::new(HashMap::new()),
+        }
+    }
+}
+
+impl KeyStore for FakeCombinedStore {
+    fn probe(&self, _name: &str) -> KeyringProbe {
+        KeyringProbe::ReachableButEmpty
+    }
+    fn load(&self, name: &str) -> Result<Option<String>, String> {
+        Ok(self.data.borrow().get(name).cloned())
+    }
+    fn load_all_readonly(&self) -> Result<Option<HashMap<String, String>>, String> {
+        Ok(Some(self.data.borrow().clone()))
+    }
+    fn write_and_verify(&self, name: &str, value: &str) -> Result<(), String> {
+        self.data
+            .borrow_mut()
+            .insert(name.to_string(), value.to_string());
+        Ok(())
+    }
+    fn store_all(&self, entries: &HashMap<String, String>) -> Result<(), String> {
+        for (k, v) in entries {
+            self.data.borrow_mut().insert(k.clone(), v.clone());
+        }
+        Ok(())
+    }
+}
+
+impl crate::managed_agents::secret_projection::ProjectionStore for FakeCombinedStore {
+    fn write_and_verify(&self, key: &str, value: &str) -> Result<(), String> {
+        self.data
+            .borrow_mut()
+            .insert(key.to_string(), value.to_string());
+        Ok(())
+    }
+    fn load_key(&self, key: &str) -> Result<Option<String>, String> {
+        Ok(self.data.borrow().get(key).cloned())
+    }
+    fn load_all(&self) -> Result<Option<HashMap<String, String>>, String> {
+        Ok(Some(self.data.borrow().clone()))
+    }
+    fn store_batch(&self, entries: &HashMap<String, String>) -> Result<(), String> {
+        for (k, v) in entries {
+            self.data.borrow_mut().insert(k.clone(), v.clone());
+        }
+        Ok(())
+    }
+    fn remove_batch(&self, keys: &[&str]) -> Result<(), String> {
+        let mut d = self.data.borrow_mut();
+        for k in keys {
+            d.remove(*k);
+        }
+        Ok(())
+    }
+}
+
+/// A key-less definition record carrying an already-projected `env_vars_ref`
+/// (no inline env) — the on-disk shape after the definition's secrets were
+/// stripped into the keyring on a prior save.
+fn projected_definition(slug: &str, env_ref: &str) -> ManagedAgentRecord {
+    let mut record: ManagedAgentRecord = serde_json::from_str(&format!(
+        r#"{{
+            "pubkey": "",
+            "name": "def-{slug}",
+            "slug": "{slug}",
+            "relay_url": "",
+            "acp_command": "buzz-acp",
+            "agent_command": "goose",
+            "agent_args": [],
+            "mcp_command": "",
+            "turn_timeout_seconds": 320,
+            "created_at": "2026-01-01T00:00:00Z",
+            "updated_at": "2026-01-01T00:00:00Z"
+        }}"#
+    ))
+    .expect("definition record");
+    record.env_vars_ref = Some(env_ref.to_string());
+    record
+}
+
+#[test]
+fn save_managed_agents_preserves_projected_definition_ref_without_reinlining() {
+    // Seed a store whose definition half is already projected: `env_vars_ref`
+    // set, NO inline env bytes. Save an UNRELATED instance. The committed store
+    // must keep the definition's ref verbatim and must not resurrect its inline
+    // env — the W2 regression was that the hydrating re-read re-inlined it.
+    use crate::managed_agents::secret_projection::definition_env_key;
+
+    let dir = tempfile::tempdir().expect("tempdir");
+    let store_path = dir.path().join("managed-agents.json");
+
+    // The keyring already holds the definition's projected env under its ref.
+    let store = FakeCombinedStore::new();
+    let def_env_gen = "gendef123";
+    store.data.borrow_mut().insert(
+        definition_env_key("shared-def", def_env_gen),
+        r#"{"DEF_SECRET":"def-secret-value"}"#.to_string(),
+    );
+
+    // On-disk starting store: one projected definition, no instances.
+    let definition = projected_definition("shared-def", def_env_gen);
+    std::fs::write(
+        &store_path,
+        serde_json::to_string(&[&definition]).expect("serialize seed"),
+    )
+    .expect("write seed store");
+
+    // Save an unrelated instance (carries its own inline env to project).
+    let mut instance = record_with_pubkey_and_key("instance-pubkey", "nsec1instkey");
+    instance.env_vars = [("INSTANCE_KEY".to_string(), "inst-secret".to_string())]
+        .into_iter()
+        .collect();
+
+    super::save_managed_agents_at(&store_path, Some(&store), std::slice::from_ref(&instance))
+        .expect("save must succeed");
+
+    // Re-read the committed store RAW (no hydration).
+    let committed = std::fs::read_to_string(&store_path).expect("read committed");
+
+    // The definition's plaintext secret must NOT be in the JSON.
+    assert!(
+        !committed.contains("def-secret-value"),
+        "an instance-side save must not re-inline the definition's projected secret"
+    );
+
+    // The definition's ref must survive verbatim.
+    let records: Vec<ManagedAgentRecord> =
+        serde_json::from_str(&committed).expect("parse committed");
+    let def = records
+        .iter()
+        .find(|r| r.pubkey.is_empty() && r.slug.as_deref() == Some("shared-def"))
+        .expect("definition must survive the instance save");
+    assert_eq!(
+        def.env_vars_ref.as_deref(),
+        Some(def_env_gen),
+        "the definition's projected ref must be preserved unchanged"
+    );
+    assert!(
+        def.env_vars.is_empty(),
+        "the definition must carry no inline env after the save"
+    );
+    // The instance was persisted alongside it.
+    assert!(
+        records.iter().any(|r| r.pubkey == "instance-pubkey"),
+        "the saved instance must be present in the committed store"
+    );
+}
+
+#[test]
+fn save_managed_agents_propagates_store_parse_error_instead_of_dropping_definitions() {
+    // A malformed on-disk store must fail the save with an error — NEVER be
+    // read as an empty definition half, because the wholesale rewrite would
+    // then delete every definition from the live store. W2/F2: the re-read uses
+    // `?`, not `unwrap_or_default()`.
+    let dir = tempfile::tempdir().expect("tempdir");
+    let store_path = dir.path().join("managed-agents.json");
+    std::fs::write(&store_path, b"{ this is not valid json ]").expect("write malformed");
+
+    let store = FakeCombinedStore::new();
+    let instance = record_with_pubkey_and_key("instance-pubkey", "nsec1instkey");
+
+    let result =
+        super::save_managed_agents_at(&store_path, Some(&store), std::slice::from_ref(&instance));
+
+    assert!(
+        result.is_err(),
+        "a malformed store must fail the save, not silently drop definitions"
+    );
+    assert!(
+        result.unwrap_err().contains("parse"),
+        "the error must surface the parse failure"
+    );
+}

desktop/src-tauri/src/migration/migration_secrets.rs

@@ -4,7 +4,15 @@
 //!
 //! Runs ONCE at the END of `run_boot_migrations_inner` (after
 //! `materialize_agent_runtimes`) so raw-JSON migrations see inline values.
-//! Idempotent: records that already have refs skip the write path.
+//!
+//! Idempotent across launches, but NOT by "skipping records that already have
+//! refs": it re-runs the field-granular migration seam over every record on
+//! every launch. That seam projects only a non-empty inline value and
+//! otherwise preserves an existing ref — it never re-reads an already-projected
+//! record's empty inline field as a user-clear, so a second launch is a no-op
+//! that leaves every committed ref intact (the strip-on-save seam would have
+//! cleared them). See
+//! [`crate::managed_agents::secret_seam::migrate_all_secrets_for_records`].
 
 use tauri::Manager;
 
@@ -318,9 +326,12 @@ pub(crate) fn cleanup_secret_artifacts(agents_dir: &std::path::Path) {
         };
 
         // ── 0o600 sweep ──────────────────────────────────────────────────
+        // Tighten permissions on every store-shaped file up front, so even a
+        // backup whose scrub write later fails is not left group/world-readable
+        // with plaintext. Uses the same backup recognizer as the scrub below.
         #[cfg(unix)]
         if fname.ends_with(".json")
-            || fname.contains(".json.bak")
+            || is_backup_filename(&fname)
             || fname.ends_with(".json.invalid")
         {
             let _ = std::fs::set_permissions(&real_path, std::fs::Permissions::from_mode(0o600));
@@ -364,15 +375,30 @@ fn is_atomic_write_temp(fname: &str) -> bool {
 }
 
 /// Returns true when `fname` is a backup file we should scrub.
+///
+/// Recognizes every managed-store backup naming family this repo produces —
+/// each is `<store>.json` with a backup suffix appended, and each can carry
+/// pre-projection plaintext (env vars, auth tags, nsecs, provider config):
+///
+/// - `managed-agents.json.bak`, `.bak-<timestamp>`, `.bak.<timestamp>`
+/// - `managed-agents.json.pre-backfill.bak` ([`crate::migration::backfill`])
+/// - `managed-agents.json.pre-team-suffix-strip.bak`
+///   ([`crate::migration::team_suffix`])
+/// - `personas.json.bak` ([`crate::migration::fold`]) / `teams.json.bak-*`
+///
+/// The match is structural rather than an enumerated suffix list: a name
+/// belonging to one of our stores whose `.json` base is followed by a `.bak`
+/// marker anywhere. This catches a new `<store>.json.<phase>.bak` producer
+/// without another edit here. Deliberately excludes the live `<store>.json`
+/// (no `.bak` after `.json`) and `.invalid` artifacts (handled separately).
 fn is_backup_filename(fname: &str) -> bool {
-    // managed-agents.json.bak-<timestamp>
-    // managed-agents.json.bak
-    // personas.json.bak-<timestamp> / teams.json.bak-<timestamp>
-    let is_json_bak = fname.contains(".json.bak");
-    is_json_bak
-        && (fname.starts_with("managed-agents")
-            || fname.starts_with("personas")
-            || fname.starts_with("teams"))
+    let is_ours = fname.starts_with("managed-agents")
+        || fname.starts_with("personas")
+        || fname.starts_with("teams");
+    let Some((_, after_json)) = fname.split_once(".json") else {
+        return false;
+    };
+    is_ours && after_json.contains(".bak")
 }
 
 /// Strip secret fields from a parseable backup and overwrite it at 0o600.
@@ -454,6 +480,18 @@ fn strip_secrets_from_json(content: &str) -> Option<String> {
     serde_json::to_string_pretty(&v).ok()
 }
 
+/// Boot-migrate inline secrets to the keyring using the field-granular
+/// migration seam — NOT the strip-on-save seam.
+///
+/// This function runs on EVERY launch over records read straight off disk.
+/// After the first launch those records are already projected: their inline
+/// fields are empty and their `*_ref`s point at live generations. The
+/// strip-on-save seam would read an empty inline field as a deliberate
+/// user-clear and drop the ref (W1: silent credential loss on the second
+/// launch, then GC deletes the orphaned generation). The migration seam does
+/// not: it only ever projects a non-empty inline value or preserves an
+/// existing ref, and never clears one it did not write. See
+/// [`crate::managed_agents::secret_seam::migrate_all_secrets_for_records`].
 fn migrate_inline_secrets_in_records(
     _app: &tauri::AppHandle,
     records: &mut [crate::managed_agents::ManagedAgentRecord],
@@ -461,26 +499,7 @@ fn migrate_inline_secrets_in_records(
     let Some(store) = crate::managed_agents::storage::agent_secret_store_pub() else {
         return false;
     };
-    let mut changed = false;
-    for record in records.iter_mut() {
-        let before_env_ref = record.env_vars_ref.clone();
-        let before_auth_ref = record.auth_tag_ref.clone();
-        let before_pc_ref = record.provider_config_ref.clone();
-        if record.pubkey.is_empty() {
-            crate::managed_agents::secret_seam::strip_and_persist_definition_secrets_with(
-                store, record,
-            );
-        } else {
-            crate::managed_agents::secret_seam::strip_and_persist_agent_secrets_with(store, record);
-        }
-        if record.env_vars_ref != before_env_ref
-            || record.auth_tag_ref != before_auth_ref
-            || record.provider_config_ref != before_pc_ref
-        {
-            changed = true;
-        }
-    }
-    changed
+    crate::managed_agents::secret_seam::migrate_all_secrets_for_records(store, records)
 }
 
 // ── Tests ──────────────────────────────────────────────────────────────────
@@ -511,6 +530,41 @@ mod tests {
         assert!(!is_backup_filename("global-agent-config.json.bak-20260608")); // not in our set
     }
 
+    #[test]
+    fn test_is_backup_filename_recognizes_phase_suffixed_producers() {
+        // W4: the exact backup filenames this repo's own migrations produce.
+        // Each is `<store>.json` with a phase/marker suffix appended — NOT the
+        // literal `.json.bak` the old recognizer required — so each survived
+        // the scrub with pre-projection plaintext (env vars, auth tags, nsecs,
+        // provider config) intact.
+        assert!(
+            is_backup_filename("managed-agents.json.pre-backfill.bak"),
+            "backfill.rs pre-migration backup must be recognized"
+        );
+        assert!(
+            is_backup_filename("managed-agents.json.pre-team-suffix-strip.bak"),
+            "team_suffix.rs pre-migration backup must be recognized"
+        );
+        assert!(
+            is_backup_filename("personas.json.bak"),
+            "the persona-fold backup must be recognized"
+        );
+        // `.bak.<stamp>` ordering (marker then stamp) is ours too.
+        assert!(is_backup_filename(
+            "managed-agents.json.bak.20260608-175938"
+        ));
+        // A future `<store>.json.<phase>.bak` producer is caught structurally,
+        // without another edit here — this is why the match is not an
+        // enumerated suffix list.
+        assert!(is_backup_filename(
+            "managed-agents.json.some-future-phase.bak"
+        ));
+
+        // A phase-suffixed backup of a store we do NOT own is not ours to
+        // scrub, even though it is shaped identically.
+        assert!(!is_backup_filename("credentials.json.pre-backfill.bak"));
+    }
+
     #[test]
     fn test_strip_secrets_from_json_strips_agent_records() {
         let input = r#"[
@@ -637,6 +691,37 @@ mod tests {
         );
     }
 
+    /// W4 end-to-end: the phase-suffixed producer backups (`pre-backfill.bak`,
+    /// `pre-team-suffix-strip.bak`) carry pre-projection plaintext and must be
+    /// scrubbed by the full cleanup sweep — not just matched by the recognizer.
+    /// Before the structural recognizer these names lacked the literal
+    /// `.json.bak` substring and survived cleanup with secrets intact.
+    #[test]
+    fn test_cleanup_scrubs_phase_suffixed_producer_backups() {
+        let dir = tempfile::tempdir().expect("tempdir");
+        let content = r#"[{"pubkey":"abc","name":"test","env_vars":{"ANTHROPIC_API_KEY":"sk-ant-secret"},"auth_tag":"tag-secret","created_at":"2026","updated_at":"2026"}]"#;
+        for name in [
+            "managed-agents.json.pre-backfill.bak",
+            "managed-agents.json.pre-team-suffix-strip.bak",
+        ] {
+            let path = dir.path().join(name);
+            std::fs::write(&path, content).expect("write producer backup");
+
+            cleanup_secret_artifacts(dir.path());
+
+            assert!(path.exists(), "{name} must survive as a scrubbed backup");
+            let result = std::fs::read_to_string(&path).expect("read");
+            assert!(
+                !result.contains("sk-ant-secret"),
+                "{name} must have its env_vars secret stripped"
+            );
+            assert!(
+                !result.contains("tag-secret"),
+                "{name} must have its auth_tag secret stripped"
+            );
+        }
+    }
+
     /// Cleanup must not follow symlinks that escape the agents dir.
     #[cfg(unix)]
     #[test]