mirror of
https://github.com/block/buzz.git
synced 2026-10-29 13:31:44 +00:00
fix(personas): address Carl/Wes review — publication recovery, refresh lock, linked identity effects
Three correctness fixes from review 5070974257: P1-1 (combined saves bypass publication recovery): settle publication independently at the definition boundary before advancing to I/L writes. When the publish command throws after persona persist and I/L writes remain, attempt the publish-only retry immediately — not at final settlement where the !observedRemainder gate suppresses it. On retry success, clear publishFailed and continue the same save. On retry failure (or no seam), set firstError to block I/L advancement with a clear publication-failure reason; the partial-failure toast names both D (saved) and the I/L remainder (not saved). Definition-only saves retain the existing final !observedRemainder && publishFailed path unchanged. New family-12 TS tests cover D+I and D+L combined save with retry success, retry failure, and no-seam cases; mutation (restoring the original !observedRemainder gate) turns all four RED. P1-2 (unlocked retry refresh can undo explicit unshare): serialize refresh_for_persona_at inside a managed_agents_store_lock scope in publish_and_refresh_teams_at, acquired after the network await and held for the synchronous refresh. Without the lock a concurrent set_team_shared(false) can retain an unshared head that the unlocked refresh overwrites at the same monotonic timestamp. Adds REFRESH_LOCK_OBSERVER (cfg(test)) matching the PRE_GUARD_OBSERVER pattern; new test asserts try_lock() fails while the refresh runs — removing the lock scope causes try_lock() to succeed, turning the test RED. P2 (successful publish-only retry hides skipped linked identity effects): capture retain_result without ? in update_persona_with, complete linked managed-agent name/avatar propagation and save_managed_agents, then propagate the retain error. Before the fix, a strict publication failure after save_personas skipped propagate_persona_name_rename and save_managed_agents entirely; a subsequent publish-only retry published the persona but linked instances retained stale names/avatars. New Rust regression seeds a persona + linked instance, induces a retain failure, and asserts the linked agent's name was updated before the error propagated; restoring retain()? before the propagation block turns the test RED. Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz> Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This commit is contained in:
Duncan
@@ -12,6 +12,16 @@ use crate::{
|
||||
|
||||
use super::pending::{prepare_persona_publication, PreparedPersonaPublication};
|
||||
|
||||
/// Test-only observer called immediately after the `managed_agents_store_lock`
|
||||
/// is acquired in `publish_and_refresh_teams_at`'s refresh section. Tests use
|
||||
/// this to assert `try_lock()` fails — proving the lock is held during the
|
||||
/// synchronous refresh. Moving the lock acquisition to AFTER the refresh call
|
||||
/// (recreating the TOCTOU race) causes `try_lock()` to succeed, turning the
|
||||
/// probe test RED.
|
||||
#[cfg(test)]
|
||||
pub(crate) static REFRESH_LOCK_OBSERVER: std::sync::Mutex<Option<Box<dyn Fn(&AppState) + Send>>> =
|
||||
std::sync::Mutex::new(None);
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Serialize)]
|
||||
#[serde(rename_all = "kebab-case")]
|
||||
pub enum PersonaSharePublicationStatus {
|
||||
@@ -116,9 +126,34 @@ pub(crate) async fn publish_and_refresh_teams_at(
|
||||
persona_id: &str,
|
||||
) -> Result<SetPersonaSharedResult, String> {
|
||||
let result = publish_prepared_persona(state, prepared).await?;
|
||||
// F2: refresh any shared 30178 heads that include this persona, matching the
|
||||
// contract of `update_persona_and_publish`. Best-effort — failures are logged.
|
||||
let _ = crate::commands::teams::refresh_for_persona_at(base_dir, keys, db_path, persona_id);
|
||||
// F2: refresh any shared 30178 heads that include this persona. The refresh
|
||||
// reads the current team/persona definitions and may retain a new head — it
|
||||
// must be serialized with team edit/unshare/delete operations that also hold
|
||||
// `managed_agents_store_lock` and may retain/retract the same head.
|
||||
//
|
||||
// Without the lock a concurrent `set_team_shared(false)` can:
|
||||
// 1. acquire the lock, retain unshared head T+1, release the lock;
|
||||
// 2. refresh (unlocked) reads old shared head T, rebuilds, retains T+1;
|
||||
// `retain_event` accepts equal timestamps — the refresh wins, undoing
|
||||
// the explicit unshare.
|
||||
//
|
||||
// Acquire AFTER the network await so the lock is never held across I/O;
|
||||
// the synchronous refresh completes entirely inside the critical section.
|
||||
{
|
||||
let _guard = state
|
||||
.managed_agents_store_lock
|
||||
.lock()
|
||||
.map_err(|e| format!("managed_agents_store_lock poisoned: {e}"))?;
|
||||
#[cfg(test)]
|
||||
{
|
||||
if let Ok(obs) = REFRESH_LOCK_OBSERVER.lock() {
|
||||
if let Some(ref f) = *obs {
|
||||
f(state);
|
||||
}
|
||||
}
|
||||
}
|
||||
let _ = crate::commands::teams::refresh_for_persona_at(base_dir, keys, db_path, persona_id);
|
||||
}
|
||||
Ok(result)
|
||||
}
|
||||
|
||||
@@ -421,4 +456,68 @@ mod tests {
|
||||
|
||||
assert!(error.contains("failed to open retention db"));
|
||||
}
|
||||
|
||||
/// Lock-scope regression for P1-2: the synchronous refresh inside
|
||||
/// `publish_and_refresh_teams_at` must hold `managed_agents_store_lock`
|
||||
/// for its entire read+retain sequence so it is serialized with concurrent
|
||||
/// team edits, unshare, and delete operations.
|
||||
///
|
||||
/// The observer fires after the lock is acquired and asserts `try_lock()`
|
||||
/// fails — proving the lock is held. Moving the lock acquisition to AFTER
|
||||
/// the refresh call (removing the lock scope) causes `try_lock()` to
|
||||
/// succeed, turning this test RED.
|
||||
#[tokio::test]
|
||||
async fn test_retry_refresh_holds_store_lock_during_refresh() {
|
||||
use std::sync::atomic::{AtomicBool, Ordering};
|
||||
use std::sync::Arc;
|
||||
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let db_path = dir.path().join("retention.db");
|
||||
let keys = nostr::Keys::generate();
|
||||
let relay_url = spawn_relay(true).await;
|
||||
let prepared = prepared(&db_path, relay_url, keys.clone(), Some(true));
|
||||
let state = build_app_state();
|
||||
|
||||
// Write an empty teams.json so refresh_for_persona_at reads zero teams
|
||||
// (no 30178 to retract). This keeps the test focused on the lock scope.
|
||||
std::fs::write(dir.path().join("teams.json"), b"[]").unwrap();
|
||||
|
||||
let observer_fired = Arc::new(AtomicBool::new(false));
|
||||
let observer_fired_clone = observer_fired.clone();
|
||||
{
|
||||
let mut slot = REFRESH_LOCK_OBSERVER.lock().expect("observer slot");
|
||||
*slot = Some(Box::new(move |state: &AppState| {
|
||||
assert!(
|
||||
state.managed_agents_store_lock.try_lock().is_err(),
|
||||
"managed_agents_store_lock must be held during the refresh — \
|
||||
a successful try_lock means the refresh runs outside the lock, \
|
||||
recreating the concurrent-unshare race condition"
|
||||
);
|
||||
observer_fired_clone.store(true, Ordering::SeqCst);
|
||||
}));
|
||||
}
|
||||
|
||||
let persona_id = prepared.persona.id.clone();
|
||||
let result = publish_and_refresh_teams_at(
|
||||
&state,
|
||||
prepared,
|
||||
dir.path(),
|
||||
&keys,
|
||||
&db_path,
|
||||
&persona_id,
|
||||
)
|
||||
.await;
|
||||
|
||||
// Clear observer before any assertion that could panic.
|
||||
{
|
||||
let mut slot = REFRESH_LOCK_OBSERVER.lock().expect("observer slot");
|
||||
*slot = None;
|
||||
}
|
||||
|
||||
result.expect("publish_and_refresh_teams_at must succeed");
|
||||
assert!(
|
||||
observer_fired.load(Ordering::SeqCst),
|
||||
"the refresh lock observer must have fired — if not, the hook is not wired"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -209,7 +209,15 @@ pub(super) async fn update_persona_with<Rt: tauri::Runtime + 'static, R: Send +
|
||||
let result = persona.clone();
|
||||
save_personas(&app, &personas)?;
|
||||
|
||||
let retained = retain(&app, &state, &result)?;
|
||||
// Capture the retain result WITHOUT propagating the error yet.
|
||||
// Linked managed-agent persistence (name/avatar propagation) and
|
||||
// relay profile sync are independent of strict publication — a
|
||||
// transient publication failure must not skip those local effects.
|
||||
// Return the retain error after completing all linked work so the
|
||||
// coordinator sees the accurate "definition saved but publication
|
||||
// failed" error and can attempt the publish-only retry seam, which
|
||||
// will now find the linked identities already updated.
|
||||
let retain_result = retain(&app, &state, &result);
|
||||
try_regenerate_nest(&app);
|
||||
|
||||
// If the avatar or display_name changed, propagate to linked agent
|
||||
@@ -295,7 +303,7 @@ pub(super) async fn update_persona_with<Rt: tauri::Runtime + 'static, R: Send +
|
||||
Vec::new()
|
||||
};
|
||||
|
||||
Ok((result, retained, sync_params))
|
||||
Ok((result, retain_result?, sync_params))
|
||||
}
|
||||
})
|
||||
.await
|
||||
|
||||
@@ -382,3 +382,156 @@ async fn retain_failure_after_persist_is_returned_not_swallowed() {
|
||||
None => std::env::remove_var("XDG_DATA_HOME"),
|
||||
}
|
||||
}
|
||||
|
||||
/// P2 regression: linked managed-agent name and avatar propagation must
|
||||
/// complete even when the retain/publish callback returns an error.
|
||||
///
|
||||
/// Before the fix: `retain(&app, &state, &result)?` propagated the error
|
||||
/// immediately, skipping the avatar/name propagation block and `save_managed_agents`.
|
||||
/// A publish-only retry (`set_persona_shared`) then published the persona but
|
||||
/// the linked instance still carried the old name/avatar.
|
||||
///
|
||||
/// After the fix: `retain_result` is captured without `?`, all linked
|
||||
/// persistence runs to completion, and THEN the retain error is propagated.
|
||||
///
|
||||
/// Mutation acceptance: restoring `retain(&app, &state, &result)?` before
|
||||
/// the avatar/name propagation block causes `save_managed_agents` to never run
|
||||
/// and the assertion on the stored agent name turns RED.
|
||||
#[tokio::test]
|
||||
async fn linked_instance_rename_completes_before_retain_error_propagates() {
|
||||
let temp = tempfile::tempdir().unwrap();
|
||||
let home = temp.path().join("home2");
|
||||
std::fs::create_dir_all(&home).unwrap();
|
||||
|
||||
let old_home = std::env::var_os("HOME");
|
||||
let old_xdg = std::env::var_os("XDG_DATA_HOME");
|
||||
{
|
||||
let _path_guard = crate::managed_agents::lock_path_mutex();
|
||||
std::env::set_var("HOME", &home);
|
||||
std::env::set_var("XDG_DATA_HOME", &home);
|
||||
}
|
||||
|
||||
let app = mock_app();
|
||||
|
||||
// Seed persona "Alice" at R1.
|
||||
save_personas(app.handle(), &[persona("p1", "Alice", R1)]).expect("seed must succeed");
|
||||
|
||||
// Seed a linked agent whose name matches the persona's display_name.
|
||||
let agent_record = crate::managed_agents::ManagedAgentRecord {
|
||||
pubkey: "pk-alice-p2".to_string(),
|
||||
name: "Alice".to_string(),
|
||||
persona_id: Some("p1".to_string()),
|
||||
private_key_nsec: String::new(),
|
||||
auth_tag: None,
|
||||
relay_url: String::new(),
|
||||
avatar_url: None,
|
||||
acp_command: String::new(),
|
||||
agent_command: String::new(),
|
||||
agent_command_override: None,
|
||||
agent_args: vec![],
|
||||
mcp_command: String::new(),
|
||||
turn_timeout_seconds: 0,
|
||||
idle_timeout_seconds: None,
|
||||
max_turn_duration_seconds: None,
|
||||
parallelism: 1,
|
||||
system_prompt: None,
|
||||
model: None,
|
||||
provider: None,
|
||||
persona_source_version: None,
|
||||
env_vars: std::collections::BTreeMap::new(),
|
||||
start_on_app_launch: false,
|
||||
auto_restart_on_config_change: false,
|
||||
runtime_pid: None,
|
||||
backend: Default::default(),
|
||||
backend_agent_id: None,
|
||||
provider_policy_pending: false,
|
||||
provider_binary_path: None,
|
||||
team_id: None,
|
||||
persona_team_dir: None,
|
||||
persona_name_in_team: None,
|
||||
created_at: String::new(),
|
||||
updated_at: String::new(),
|
||||
last_started_at: None,
|
||||
last_stopped_at: None,
|
||||
last_exit_code: None,
|
||||
last_error: None,
|
||||
last_error_code: None,
|
||||
respond_to: Default::default(),
|
||||
respond_to_allowlist: vec![],
|
||||
display_name: Some("Alice".to_string()),
|
||||
slug: None,
|
||||
runtime: None,
|
||||
name_pool: vec![],
|
||||
is_builtin: false,
|
||||
is_active: true,
|
||||
shared: false,
|
||||
source_team: None,
|
||||
source_team_persona_slug: None,
|
||||
catalog_source: None,
|
||||
team_catalog_source: None,
|
||||
definition_respond_to: None,
|
||||
definition_respond_to_allowlist: vec![],
|
||||
definition_parallelism: None,
|
||||
relay_mesh: None,
|
||||
effort_level: None,
|
||||
};
|
||||
crate::managed_agents::save_managed_agents(app.handle(), &[agent_record])
|
||||
.expect("agent seed must succeed");
|
||||
|
||||
// Submit a rename with a retain callback that always fails — simulates a
|
||||
// strict publication/enqueue failure AFTER save_personas ran.
|
||||
let result = update_persona_with(
|
||||
UpdatePersonaRequest {
|
||||
id: "p1".to_string(),
|
||||
display_name: "Alice Renamed".to_string(),
|
||||
avatar_url: None,
|
||||
system_prompt: "Do the work.".to_string(),
|
||||
runtime: None,
|
||||
model: None,
|
||||
provider: None,
|
||||
name_pool: Vec::new(),
|
||||
env_vars: None,
|
||||
behavior: None,
|
||||
expected_updated_at: Some(R1.to_string()),
|
||||
},
|
||||
app.handle().clone(),
|
||||
|_app, _state, _persona| -> Result<(), String> {
|
||||
Err("simulated publish failure after persona persisted".to_string())
|
||||
},
|
||||
)
|
||||
.await;
|
||||
|
||||
// The retain error must propagate so the coordinator sees publishFailed.
|
||||
assert!(
|
||||
result.is_err(),
|
||||
"update_persona_with must return Err when retain fails; \
|
||||
if this is Ok the retain error was swallowed"
|
||||
);
|
||||
|
||||
// The linked agent must have been renamed BEFORE the error propagated.
|
||||
// If save_managed_agents was skipped (the pre-fix path), the name is still "Alice".
|
||||
let agents = crate::managed_agents::load_managed_agents(app.handle())
|
||||
.expect("agent reload must succeed");
|
||||
let stored_agent = agents
|
||||
.iter()
|
||||
.find(|a| a.pubkey == "pk-alice-p2")
|
||||
.expect("linked agent must still exist");
|
||||
|
||||
assert_eq!(
|
||||
stored_agent.name, "Alice Renamed",
|
||||
"linked instance name must be propagated before the retain error is returned; \
|
||||
restoring `retain()?` before the propagation block turns this RED"
|
||||
);
|
||||
|
||||
// Cleanup
|
||||
std::env::remove_var("HOME");
|
||||
std::env::remove_var("XDG_DATA_HOME");
|
||||
match old_home {
|
||||
Some(v) => std::env::set_var("HOME", v),
|
||||
None => std::env::remove_var("HOME"),
|
||||
}
|
||||
match old_xdg {
|
||||
Some(v) => std::env::set_var("XDG_DATA_HOME", v),
|
||||
None => std::env::remove_var("XDG_DATA_HOME"),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1864,3 +1864,256 @@ test("test_no_publish_retry_seam_shows_terminal_warning_not_reopen", async () =>
|
||||
cap.restore();
|
||||
}
|
||||
});
|
||||
|
||||
// ── Test family 12: P1 — combined saves (D+I, D+L) bypass recovery ─────────────
|
||||
//
|
||||
// When `publishCatalogUpdates` is true and the command throws after persist,
|
||||
// `publishFailed` is set and `firstError` suppresses I/L writes. A combined
|
||||
// D+I or D+L save must settle publication independently — before advancing —
|
||||
// so the full D+I or D+L path can complete when the retry succeeds, and must
|
||||
// name the unpublished catalog alongside the unsaved I/L remainder when it
|
||||
// fails.
|
||||
//
|
||||
// Mutation acceptance: restoring the original `!observedRemainder` gate must
|
||||
// turn these tests RED because the retry is never called for combined saves.
|
||||
|
||||
test("test_combined_di_save_publish_retry_succeeds_continues_instance_write", async () => {
|
||||
// D+I combined save: publish throws after persist, retry succeeds — I write
|
||||
// must proceed and the whole save must close as full success.
|
||||
const cap = captureToasts();
|
||||
try {
|
||||
const persistedDef = makeDefinition({ displayName: "Alice" });
|
||||
const persistedInst = makeInstance({ name: "Alice" });
|
||||
let retryCalls = 0;
|
||||
let agentCalls = 0;
|
||||
const opts = makeOpts({
|
||||
ctx: {
|
||||
kind: "instance-with-definition",
|
||||
definition: makeDefinition({ displayName: "Alice" }),
|
||||
instance: makeInstance(),
|
||||
},
|
||||
personaInput: makePersonaInput({ displayName: "Alice" }),
|
||||
agentInput: makeAgentInput({ name: "Alice" }),
|
||||
publishCatalogUpdates: true,
|
||||
updatePersonaAndPublish: async () => {
|
||||
throw new Error("failed to open retention db");
|
||||
},
|
||||
updateManagedAgent: async () => {
|
||||
agentCalls++;
|
||||
return { agent: persistedInst, profileSyncError: null };
|
||||
},
|
||||
refetchStores: async () => ({
|
||||
persona: persistedDef,
|
||||
agent: persistedInst,
|
||||
}),
|
||||
publishRetry: async () => {
|
||||
retryCalls++;
|
||||
return { persona: persistedDef, publicationStatus: "published" };
|
||||
},
|
||||
});
|
||||
|
||||
const result = await runAgentSaveCoordinator(opts);
|
||||
|
||||
assert.equal(
|
||||
result,
|
||||
true,
|
||||
"full success when retry + I write both succeed",
|
||||
);
|
||||
assert.equal(
|
||||
opts._calls.onDone,
|
||||
1,
|
||||
"onDone must be called on full success",
|
||||
);
|
||||
assert.equal(
|
||||
retryCalls,
|
||||
1,
|
||||
"publishRetry must be called once for the early retry",
|
||||
);
|
||||
assert.equal(
|
||||
agentCalls,
|
||||
1,
|
||||
"instance write must proceed after retry success",
|
||||
);
|
||||
const warnings = cap.captured.filter((c) => c.kind === "warning");
|
||||
assert.equal(warnings.length, 0, "no warning when everything succeeds");
|
||||
const successes = cap.captured.filter((c) => c.kind === "success");
|
||||
assert.equal(successes.length, 1, "success toast must fire");
|
||||
} finally {
|
||||
cap.restore();
|
||||
}
|
||||
});
|
||||
|
||||
test("test_combined_dl_save_publish_retry_succeeds_continues_policy_write", async () => {
|
||||
// D+L combined save: publish throws after persist, retry succeeds — policy
|
||||
// write must proceed and the whole save must close as full success.
|
||||
const cap = captureToasts();
|
||||
try {
|
||||
const persistedDef = makeDefinition({ displayName: "Alice" });
|
||||
const persistedInst = makeInstance({ autoRestartOnConfigChange: true });
|
||||
let retryCalls = 0;
|
||||
let policyCalls = 0;
|
||||
const opts = makeOpts({
|
||||
ctx: {
|
||||
kind: "instance-with-definition",
|
||||
definition: makeDefinition({ displayName: "Alice" }),
|
||||
instance: makeInstance(),
|
||||
},
|
||||
personaInput: makePersonaInput({ displayName: "Alice" }),
|
||||
agentInput: null,
|
||||
policySets: [{ type: "autoRestart", pubkey: "pk-abc", value: true }],
|
||||
publishCatalogUpdates: true,
|
||||
updatePersonaAndPublish: async () => {
|
||||
throw new Error("failed to open retention db");
|
||||
},
|
||||
setAutoRestart: async () => {
|
||||
policyCalls++;
|
||||
},
|
||||
refetchStores: async () => ({
|
||||
persona: persistedDef,
|
||||
agent: persistedInst,
|
||||
}),
|
||||
publishRetry: async () => {
|
||||
retryCalls++;
|
||||
return { persona: persistedDef, publicationStatus: "published" };
|
||||
},
|
||||
});
|
||||
|
||||
const result = await runAgentSaveCoordinator(opts);
|
||||
|
||||
assert.equal(
|
||||
result,
|
||||
true,
|
||||
"full success when retry + policy write both succeed",
|
||||
);
|
||||
assert.equal(
|
||||
opts._calls.onDone,
|
||||
1,
|
||||
"onDone must be called on full success",
|
||||
);
|
||||
assert.equal(retryCalls, 1, "publishRetry must be called once");
|
||||
assert.equal(
|
||||
policyCalls,
|
||||
1,
|
||||
"policy write must proceed after retry success",
|
||||
);
|
||||
const warnings = cap.captured.filter((c) => c.kind === "warning");
|
||||
assert.equal(warnings.length, 0, "no warning when everything succeeds");
|
||||
} finally {
|
||||
cap.restore();
|
||||
}
|
||||
});
|
||||
|
||||
test("test_combined_di_save_publish_retry_failure_names_both_catalog_and_remainder", async () => {
|
||||
// D+I combined save: publish throws, retry also fails — toast must name
|
||||
// the profile as saved, instance settings as not saved, and catalog failure
|
||||
// as the reason. Never claim reopen can republish the catalog.
|
||||
const cap = captureToasts();
|
||||
try {
|
||||
const persistedDef = makeDefinition({ displayName: "Alice" });
|
||||
const opts = makeOpts({
|
||||
ctx: {
|
||||
kind: "instance-with-definition",
|
||||
definition: makeDefinition({ displayName: "Alice" }),
|
||||
instance: makeInstance(),
|
||||
},
|
||||
personaInput: makePersonaInput({ displayName: "Alice" }),
|
||||
agentInput: makeAgentInput({ name: "Alice-renamed" }),
|
||||
publishCatalogUpdates: true,
|
||||
updatePersonaAndPublish: async () => {
|
||||
throw new Error("relay unreachable");
|
||||
},
|
||||
updateManagedAgent: async () => {
|
||||
throw new Error("should not be called");
|
||||
},
|
||||
refetchStores: async () => ({ persona: persistedDef, agent: null }),
|
||||
publishRetry: async () => {
|
||||
throw new Error("still unreachable");
|
||||
},
|
||||
});
|
||||
|
||||
const result = await runAgentSaveCoordinator(opts);
|
||||
|
||||
assert.equal(result, false, "combined failure returns false");
|
||||
assert.equal(opts._calls.onDone, 0, "onDone must not be called");
|
||||
// Instance write must NOT have been attempted.
|
||||
assert.equal(
|
||||
opts._calls.updateManagedAgent,
|
||||
0,
|
||||
"instance write must be skipped",
|
||||
);
|
||||
const warnings = cap.captured.filter((c) => c.kind === "warning");
|
||||
assert.equal(warnings.length, 1, "exactly one warning");
|
||||
// Must name D as saved and publication failure.
|
||||
assert.match(
|
||||
warnings[0].message,
|
||||
/profile saved/i,
|
||||
"warning must name profile as saved",
|
||||
);
|
||||
assert.match(
|
||||
warnings[0].message,
|
||||
/catalog publication failed|could not be published/i,
|
||||
"warning must name the publication failure",
|
||||
);
|
||||
// Must name the I/L remainder as not saved.
|
||||
assert.match(
|
||||
warnings[0].message,
|
||||
/instance settings/i,
|
||||
"warning must name instance settings as the unsaved remainder",
|
||||
);
|
||||
} finally {
|
||||
cap.restore();
|
||||
}
|
||||
});
|
||||
|
||||
test("test_combined_di_save_no_retry_seam_names_both_catalog_and_remainder", async () => {
|
||||
// D+I combined save: publish throws, no retry seam provided — toast must
|
||||
// still name catalog failure + unsaved I/L remainder without a publishRetry.
|
||||
const cap = captureToasts();
|
||||
try {
|
||||
const persistedDef = makeDefinition({ displayName: "Alice" });
|
||||
const opts = makeOpts({
|
||||
ctx: {
|
||||
kind: "instance-with-definition",
|
||||
definition: makeDefinition({ displayName: "Alice" }),
|
||||
instance: makeInstance(),
|
||||
},
|
||||
personaInput: makePersonaInput({ displayName: "Alice" }),
|
||||
agentInput: makeAgentInput({ name: "Alice-renamed" }),
|
||||
publishCatalogUpdates: true,
|
||||
updatePersonaAndPublish: async () => {
|
||||
throw new Error("relay unreachable");
|
||||
},
|
||||
refetchStores: async () => ({ persona: persistedDef, agent: null }),
|
||||
// publishRetry intentionally omitted.
|
||||
});
|
||||
|
||||
const result = await runAgentSaveCoordinator(opts);
|
||||
|
||||
assert.equal(result, false, "combined failure without seam returns false");
|
||||
assert.equal(opts._calls.onDone, 0, "onDone must not be called");
|
||||
assert.equal(
|
||||
opts._calls.updateManagedAgent,
|
||||
0,
|
||||
"instance write must be skipped",
|
||||
);
|
||||
const warnings = cap.captured.filter((c) => c.kind === "warning");
|
||||
assert.equal(warnings.length, 1, "exactly one warning");
|
||||
assert.match(
|
||||
warnings[0].message,
|
||||
/profile saved/i,
|
||||
"warning must name profile as saved",
|
||||
);
|
||||
assert.match(
|
||||
warnings[0].message,
|
||||
/catalog publication failed|could not be published/i,
|
||||
"warning must name the catalog failure",
|
||||
);
|
||||
assert.match(
|
||||
warnings[0].message,
|
||||
/instance settings/i,
|
||||
"warning must name instance settings as the unsaved remainder",
|
||||
);
|
||||
} finally {
|
||||
cap.restore();
|
||||
}
|
||||
});
|
||||
|
||||
@@ -291,7 +291,47 @@ export async function runAgentSaveCoordinator(
|
||||
// reached. Mark this so final settlement does not close as full success:
|
||||
// "save and publish" promised a relay outcome that was not delivered.
|
||||
publishFailed = true;
|
||||
firstError = caughtError;
|
||||
// For combined D+I or D+L saves, settle publication independently now —
|
||||
// before advancing to I/L writes — so a transient preparation failure
|
||||
// does not permanently bypass recovery. If retry succeeds, clear
|
||||
// publishFailed/firstError and proceed normally. If retry fails (or no
|
||||
// seam), set firstError to block I/L advancement: the partial-failure
|
||||
// toast will name both the unsaved I/L remainder and the unpublished
|
||||
// catalog, since a fresh reopen seeds persisted values with no
|
||||
// personaInput, meaning publication is never re-attempted.
|
||||
const hasPendingIL = agentInput !== null || policySets.length > 0;
|
||||
if (hasPendingIL) {
|
||||
// Combined D+I or D+L save: settle publication before advancing to I/L.
|
||||
if (publishRetry && def) {
|
||||
let earlyRetryError: string | null = null;
|
||||
try {
|
||||
const earlyRetryResult = await publishRetry(def.id);
|
||||
publicationStatus = earlyRetryResult.publicationStatus;
|
||||
// Retry succeeded — publication settled. Clear both flags so I/L
|
||||
// writes can continue and the final success branch fires.
|
||||
publishFailed = false;
|
||||
firstError = null;
|
||||
} catch (err) {
|
||||
earlyRetryError =
|
||||
err instanceof Error ? err.message : "unknown error";
|
||||
}
|
||||
if (publishFailed) {
|
||||
// Retry also failed — block I/L advancement and carry the reason
|
||||
// as firstError. The partial-failure toast will name the profile as
|
||||
// saved and the I/L remainder as not saved, with the catalog error
|
||||
// as the failure reason — reopen/retry is accurate for the I/L
|
||||
// remainder; the D change IS kept, catalog publication is not.
|
||||
firstError = `catalog publication failed: ${earlyRetryError ?? caughtError ?? "unknown error"}`;
|
||||
}
|
||||
} else {
|
||||
// No retry seam — block I/L advancement for the same reason.
|
||||
firstError = `catalog publication failed: ${caughtError ?? "unknown error"}`;
|
||||
}
|
||||
} else {
|
||||
// Definition-only save: the final `!observedRemainder && publishFailed`
|
||||
// block handles publication retry after all settlement.
|
||||
firstError = caughtError;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user