From e45f8dc9a2527eb9354344811b8975240fc6c48c Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Sat, 1 Aug 2026 15:54:31 -0400 Subject: [PATCH] fix: harden three mutation-escape test paths from Paul's bounce MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three test-correctness fixes, no production logic changes: 1. test_retain_agent_record_user_intent_clears_gate_but_reconcile_does_not: The interactive-save call previously used the same record (unchanged content), so the early-return branch fired and set_publish_blocked(false) cleared the gate before the retain_user_intent_event path was reached. Reverting the changed-content branch to retain_event left the test green. Added Case B: interactive save with a different system_prompt, which exercises the retain_user_intent_event path directly. Also added Case D: reconcile with changed content, confirming retain_event does not clear the gate. Now all four combinations (same/changed × user/reconcile) are verified. 2. test_non_persona_tombstone_user_intent_clears_gate: Previously called retain_user_intent_event directly — reverting tombstone_managed_agent_pending to retain_event left zero test callers on the production path, so the suite stayed green. Extracted tombstone_agent_pending_inner from commands/agents.rs into managed_agents/reconcile.rs (pub(crate), symmetric with retain_agent_record) and rewired tombstone_managed_agent_pending to call it. Test now drives the seam directly. Pre-seeds the tombstone coordinate with publish_blocked=true so retain_event (which leaves the existing value) and retain_user_intent_event (which calls set_publish_blocked(false)) produce distinguishable outcomes. Mutation at the production call site now fails the suite. 3. test_mark_synced_and_stamp_baseline_atomic_both_written_on_success: Previously tested the success path only. Added Case B: drops the persona_baselines table before the call, forces the baseline INSERT to fail, and asserts the call errors AND pending_sync is still 1 (transaction rolled back). If the function is split back into two autocommit statements, the UPDATE commits first and pending_sync becomes 0 before the INSERT fails — the rollback assertion catches the regression. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/src-tauri/src/commands/agents.rs | 26 +-- .../config_sync/regression_tests.rs | 210 +++++++++++++----- .../src-tauri/src/managed_agents/reconcile.rs | 39 ++++ 3 files changed, 197 insertions(+), 78 deletions(-) diff --git a/desktop/src-tauri/src/commands/agents.rs b/desktop/src-tauri/src/commands/agents.rs index 0be90aa1e3..11ba0946a7 100644 --- a/desktop/src-tauri/src/commands/agents.rs +++ b/desktop/src-tauri/src/commands/agents.rs @@ -78,33 +78,15 @@ pub(super) fn tombstone_managed_agent_pending( state: &AppState, agent_pubkey: &str, ) { - use crate::managed_agents::{ - agent_events::build_agent_delete, - retention::{ - delete_retained_event, open_retention_db, retain_user_intent_event, - tombstone_retention_d_tag, RetainedEvent, - }, - }; - use buzz_core_pkg::kind::KIND_MANAGED_AGENT; - - const KIND_DELETE: u32 = 5; + use crate::managed_agents::retention::open_retention_db; let result = (|| -> Result<(), String> { let scope = crate::managed_agents::retention::active_retention_scope(app, state)?; - let owner_pubkey = scope.owner_keys.public_key().to_hex(); - let event = build_agent_delete(agent_pubkey, &owner_pubkey)? - .sign_with_keys(&scope.owner_keys) - .map_err(|e| format!("failed to sign managed-agent tombstone: {e}"))?; let conn = open_retention_db(&scope.db_path)?; - delete_retained_event(&conn, KIND_MANAGED_AGENT, &owner_pubkey, agent_pubkey)?; - retain_user_intent_event( + crate::managed_agents::reconcile::tombstone_agent_pending_inner( &conn, - &RetainedEvent::pending( - KIND_DELETE, - owner_pubkey, - tombstone_retention_d_tag(KIND_MANAGED_AGENT, agent_pubkey), - &event, - ), + &scope.owner_keys, + agent_pubkey, ) })(); if let Err(e) = result { diff --git a/desktop/src-tauri/src/managed_agents/config_sync/regression_tests.rs b/desktop/src-tauri/src/managed_agents/config_sync/regression_tests.rs index 66014ced9f..6bae901c68 100644 --- a/desktop/src-tauri/src/managed_agents/config_sync/regression_tests.rs +++ b/desktop/src-tauri/src/managed_agents/config_sync/regression_tests.rs @@ -394,13 +394,19 @@ fn test_user_edit_reopens_gated_row_but_reconcile_retain_does_not() { // ── (c-ext) Thufir pass-1 finding #2: retain_agent_record user-intent clears // the gate; boot-reconcile (user_intent=false) does NOT. // -// Mutation-sensitive: passing `false` (reconcile mode) to both calls would -// leave the gate set after the first assert succeeds (reconcile doesn't clear -// it) and would cause the second assert to pass trivially — but the assert on -// the gate being CLEAR after user_intent=true would fail because nothing -// cleared it, catching the regression. Conversely, passing `true` to both -// would make the reconcile call incorrectly clear the gate, failing the final -// "reconcile must NOT clear" assertion. +// Two cases exercised: +// +// Case A — unchanged content (same record re-saved). The early-return branch +// in retain_agent_record fires; user-intent clears the gate via +// `set_publish_blocked(false)`. Mutation: removing the early-return +// `set_publish_blocked` call leaves the gate set → assertion fails. +// +// Case B — changed content (record with different system_prompt). The +// changed-content branch fires; user-intent calls `retain_user_intent_event` +// which clears publish_blocked. Mutation: reverting that branch to +// `retain_event` leaves the gate set → assertion fails. This is the critical +// path Paul's mutation revealed: a user who actually EDITS a parked agent must +// also see the gate cleared. #[test] fn test_retain_agent_record_user_intent_clears_gate_but_reconcile_does_not() { let conn = test_db(); @@ -408,53 +414,97 @@ fn test_retain_agent_record_user_intent_clears_gate_but_reconcile_does_not() { let owner = keys.public_key().to_hex(); let record = make_sample_agent_record(); + // ── Case A: unchanged content ────────────────────────────────────────── // Seed the row as blocked (simulates what the boot barrier would have set). retain_agent_record(&conn, &keys, &record, false).unwrap(); set_publish_blocked(&conn, KIND_MANAGED_AGENT, &owner, &record.pubkey, true).unwrap(); assert!( is_publish_blocked(&conn, KIND_MANAGED_AGENT, &owner, &record.pubkey).unwrap(), - "precondition: gate must be set" + "precondition: gate must be set (case A)" ); - // Interactive save (user_intent = true) must reopen the gate. + // Interactive save with SAME content — early-return branch, still clears gate. retain_agent_record(&conn, &keys, &record, true).unwrap(); assert!( !is_publish_blocked(&conn, KIND_MANAGED_AGENT, &owner, &record.pubkey).unwrap(), - "user-intent retain_agent_record must clear publish_blocked" + "user-intent retain_agent_record must clear publish_blocked even for unchanged content (case A)" ); - // Re-gate to simulate a fresh boot decision. + // Re-gate for case B. set_publish_blocked(&conn, KIND_MANAGED_AGENT, &owner, &record.pubkey, true).unwrap(); - // Boot reconcile (user_intent = false) must NOT clear it. + // ── Case B: changed content ──────────────────────────────────────────── + // Mutate the record's system prompt so the changed-content branch fires, + // exercising the retain_user_intent_event path (not just set_publish_blocked). + let mut edited_record = make_sample_agent_record(); + edited_record.system_prompt = + Some("EDITED PROMPT — triggers the changed-content branch".to_string()); + + retain_agent_record(&conn, &keys, &edited_record, true).unwrap(); + assert!( + !is_publish_blocked(&conn, KIND_MANAGED_AGENT, &owner, &edited_record.pubkey).unwrap(), + "user-intent retain_agent_record must clear publish_blocked for changed content (case B)" + ); + + // Re-gate to test the reconcile path (changed content, user_intent=false). + set_publish_blocked(&conn, KIND_MANAGED_AGENT, &owner, &record.pubkey, true).unwrap(); + + // Boot reconcile (user_intent = false) must NOT clear it — either branch. retain_agent_record(&conn, &keys, &record, false).unwrap(); assert!( is_publish_blocked(&conn, KIND_MANAGED_AGENT, &owner, &record.pubkey).unwrap(), - "reconcile retain_agent_record must NOT clear publish_blocked" + "reconcile retain_agent_record must NOT clear publish_blocked (same-content, case C)" + ); + + set_publish_blocked(&conn, KIND_MANAGED_AGENT, &owner, &record.pubkey, true).unwrap(); + let mut reconcile_edit = make_sample_agent_record(); + reconcile_edit.system_prompt = + Some("RECONCILE EDIT — changed-content branch, reconcile mode".to_string()); + retain_agent_record(&conn, &keys, &reconcile_edit, false).unwrap(); + assert!( + is_publish_blocked(&conn, KIND_MANAGED_AGENT, &owner, &record.pubkey).unwrap(), + "reconcile retain_agent_record must NOT clear publish_blocked (changed-content, case D)" ); } -// ── (c-ext2) Thufir pass-1 finding #2: non-persona tombstone (kind:5 for -// a managed-agent coordinate) via retain_user_intent_event clears the gate; -// bare retain_event does NOT. +// ── (c-ext2) Thufir pass-1 finding #2: agent tombstone via the production seam +// (`tombstone_agent_pending_inner`) clears the gate; the pre-existing row gate +// confirms that a plain `retain_event` call would NOT clear it. // -// Mutation-sensitive: the production paths in `tombstone_managed_agent_pending` -// and `tombstone_team_pending` now call `retain_user_intent_event`; if they -// were accidentally reverted to `retain_event`, the first assertion would fail. +// Mutation-sensitive: `tombstone_agent_pending_inner` calls +// `retain_user_intent_event`. `retain_event` alone does NOT update +// `publish_blocked` in its ON CONFLICT SET clause — it leaves the existing +// value unchanged, so a pre-seeded gate survives. `retain_user_intent_event` +// always calls `set_publish_blocked(false)` after the upsert. If +// `tombstone_agent_pending_inner` is reverted to `retain_event`, the pre-seeded +// gate stays set and the post-tombstone assertion fails. +// +// The test drives the production seam directly, so a missing or changed +// production caller is caught without depending on AppHandle. #[test] fn test_non_persona_tombstone_user_intent_clears_gate() { - use nostr::JsonUtil; + use crate::managed_agents::reconcile::tombstone_agent_pending_inner; let conn = test_db(); - const KIND_DELETE: u32 = 5; - let tombstone_d = tombstone_retention_d_tag(KIND_MANAGED_AGENT, "agent-abc123"); let keys = nostr::Keys::generate(); let owner = keys.public_key().to_hex(); - let event = nostr::EventBuilder::new(nostr::Kind::Custom(KIND_DELETE as u16), "{}".to_string()) - .sign_with_keys(&keys) - .unwrap(); + // The agent coordinate to be tombstoned. + let agent_pubkey = "abababababababababababababababababababababababababababababababababab"; - // Gate a placeholder row at the tombstone coordinate. + // The tombstone coordinate is (KIND_DELETE=5, owner, tombstone_d_tag). + use crate::managed_agents::retention::tombstone_retention_d_tag; + const KIND_DELETE: u32 = 5; + let tombstone_d = tombstone_retention_d_tag(KIND_MANAGED_AGENT, agent_pubkey); + + // Pre-seed the tombstone coordinate as blocked. This simulates the case + // where the coordinate was previously parked by the boot barrier. + // retain_event does NOT clear publish_blocked on conflict — only + // retain_user_intent_event does. Seeding here forces the test to distinguish + // the two paths on the pre-existing row, making it mutation-sensitive. + let placeholder_event = + nostr::EventBuilder::new(nostr::Kind::Custom(KIND_DELETE as u16), "{}".to_string()) + .sign_with_keys(&keys) + .unwrap(); retain_event( &conn, &RetainedEvent { @@ -462,9 +512,12 @@ fn test_non_persona_tombstone_user_intent_clears_gate() { pubkey: owner.clone(), d_tag: tombstone_d.clone(), content: "{}".to_string(), - created_at: event.created_at.as_secs() as i64, - raw_event: event.as_json(), - event_id: Some(event.id.to_hex()), + created_at: placeholder_event.created_at.as_secs() as i64, + raw_event: { + use nostr::JsonUtil; + placeholder_event.as_json() + }, + event_id: Some(placeholder_event.id.to_hex()), pending_sync: false, publish_blocked: false, }, @@ -473,43 +526,47 @@ fn test_non_persona_tombstone_user_intent_clears_gate() { set_publish_blocked(&conn, KIND_DELETE, &owner, &tombstone_d, true).unwrap(); assert!( is_publish_blocked(&conn, KIND_DELETE, &owner, &tombstone_d).unwrap(), - "precondition: gate must be set" + "precondition: tombstone coordinate must be gated before the seam call" ); - // User-intent tombstone retain must clear the gate. - let event2 = - nostr::EventBuilder::new(nostr::Kind::Custom(KIND_DELETE as u16), "{}".to_string()) - .sign_with_keys(&keys) - .unwrap(); - retain_user_intent_event( - &conn, - &RetainedEvent { - kind: KIND_DELETE, - pubkey: owner.clone(), - d_tag: tombstone_d.clone(), - content: "{}".to_string(), - created_at: event2.created_at.as_secs() as i64 + 1, - raw_event: event2.as_json(), - event_id: Some(event2.id.to_hex()), - pending_sync: true, - publish_blocked: false, - }, - ) - .unwrap(); + // Also seed the agent row (tombstone_agent_pending_inner deletes it). + retain_agent_record(&conn, &keys, &make_sample_agent_record(), false).unwrap(); + + // Run the production tombstone seam through the conn+keys boundary. + // This must: (1) delete the agent's pending row, and (2) retain a user-intent + // kind:5 tombstone whose post-upsert set_publish_blocked call clears the gate. + tombstone_agent_pending_inner(&conn, &keys, agent_pubkey).unwrap(); + + // Gate must NOT be set on the tombstone row — user intent cleared it. assert!( !is_publish_blocked(&conn, KIND_DELETE, &owner, &tombstone_d).unwrap(), - "user-intent tombstone retain must clear publish_blocked" + "tombstone_agent_pending_inner must clear publish_blocked on the tombstone coordinate" + ); + + // The tombstone row must be pending for relay sync. + let tombstone_row = get_retained_event(&conn, KIND_DELETE, &owner, &tombstone_d) + .unwrap() + .expect("tombstone row must exist after tombstone_agent_pending_inner"); + assert!( + tombstone_row.pending_sync, + "tombstone row must have pending_sync = 1 (queued for relay)" ); } // ── (f) Thufir pass-1 finding: mark_synced_and_stamp_baseline atomically // clears pending AND stamps the baseline in a single transaction. // -// Mutation-sensitive: if the function is split back into two independent -// autocommit statements, the test still passes on the happy path — but the -// invariant that "pending_sync = 0 iff baseline is stamped" becomes testable -// by verifying both columns together. If only one statement ran, one of the -// two final assertions would fail, catching the regression. +// Two cases: +// +// Case A (success): both effects land together. Mutation: splitting back into +// two autocommit statements still passes the happy path — but see case B. +// +// Case B (failure): the persona_baselines table is dropped before the call, +// forcing the baseline INSERT to fail. The transaction must roll back — meaning +// pending_sync must remain 1. Mutation: if the helper is split back into two +// autocommit statements (mark_synced first, then set_baseline_full), the first +// statement clears pending_sync BEFORE the baseline INSERT fails; pending_sync +// then reads 0, failing the rollback assertion and catching the regression. #[test] fn test_mark_synced_and_stamp_baseline_atomic_both_written_on_success() { let conn = test_db(); @@ -547,6 +604,7 @@ fn test_mark_synced_and_stamp_baseline_atomic_both_written_on_success() { "no baseline before mark_synced_and_stamp_baseline" ); + // ── Case A: success path ────────────────────────────────────────────── mark_synced_and_stamp_baseline( &conn, KIND_PERSONA, @@ -579,6 +637,46 @@ fn test_mark_synced_and_stamp_baseline_atomic_both_written_on_success() { baseline.1, content, "baseline content must match the published content" ); + + // ── Case B: failure path — transaction must roll back ───────────────── + // Re-set pending_sync so there is something to clear. + conn.execute( + "UPDATE persona_events SET pending_sync = 1 WHERE kind = ?1 AND pubkey = ?2 AND d_tag = ?3", + rusqlite::params![KIND_PERSONA, OWNER, D_TAG], + ) + .unwrap(); + assert!( + get_retained_event(&conn, KIND_PERSONA, OWNER, D_TAG) + .unwrap() + .unwrap() + .pending_sync, + "precondition: pending_sync must be 1 before failure test" + ); + + // Drop persona_baselines so the baseline INSERT inside the transaction fails. + conn.execute_batch("DROP TABLE persona_baselines").unwrap(); + + // The call must return an error. + let result = mark_synced_and_stamp_baseline( + &conn, + KIND_PERSONA, + OWNER, + D_TAG, + created_at, + &content, + &event_id, + false, + ); + assert!(result.is_err(), "must error when baseline table is absent"); + + // The transaction must have rolled back — pending_sync must still be 1. + assert!( + get_retained_event(&conn, KIND_PERSONA, OWNER, D_TAG) + .unwrap() + .unwrap() + .pending_sync, + "pending_sync must remain 1 when the transaction rolled back (atomicity contract)" + ); } /// Helper shared by the agent-record tests in this module. diff --git a/desktop/src-tauri/src/managed_agents/reconcile.rs b/desktop/src-tauri/src/managed_agents/reconcile.rs index 9404fb4ecf..77cb74f4c6 100644 --- a/desktop/src-tauri/src/managed_agents/reconcile.rs +++ b/desktop/src-tauri/src/managed_agents/reconcile.rs @@ -166,5 +166,44 @@ pub(crate) fn reconcile_agents_in_dir(base_dir: &Path, keys: &nostr::Keys) -> Re reconcile_agents_in_dir_at(base_dir, keys, &base_dir.join("retention.db")) } +/// AppHandle-free core of the agent tombstone flow. Extracted as a seam so the +/// retention logic is independently testable without a running Tauri app. +/// +/// Deletes the pending agent row at `(30177, owner, agent_pubkey)` and retains +/// a user-intent kind:5 tombstone at `(5, owner, tombstone_d_tag)`. The +/// user-intent write unconditionally clears `publish_blocked` so a tombstone +/// always propagates in-session, even if the coordinate was previously parked. +pub(crate) fn tombstone_agent_pending_inner( + conn: &rusqlite::Connection, + keys: &nostr::Keys, + agent_pubkey: &str, +) -> Result<(), String> { + use super::{ + agent_events::build_agent_delete, + retention::{ + delete_retained_event, retain_user_intent_event, tombstone_retention_d_tag, + RetainedEvent, + }, + }; + use buzz_core_pkg::kind::KIND_MANAGED_AGENT; + + const KIND_DELETE: u32 = 5; + + let owner_pubkey = keys.public_key().to_hex(); + let event = build_agent_delete(agent_pubkey, &owner_pubkey)? + .sign_with_keys(keys) + .map_err(|e| format!("failed to sign managed-agent tombstone: {e}"))?; + delete_retained_event(conn, KIND_MANAGED_AGENT, &owner_pubkey, agent_pubkey)?; + retain_user_intent_event( + conn, + &RetainedEvent::pending( + KIND_DELETE, + owner_pubkey, + tombstone_retention_d_tag(KIND_MANAGED_AGENT, agent_pubkey), + &event, + ), + ) +} + #[cfg(test)] mod tests;