mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
fix(desktop): close cross-workspace library review findings F1-F5
Interim review of Phases 0+1 found one CRITICAL and three IMPORTANT defects plus one MINOR; all are resolved here so Phase 2 can resume. F1 (CRITICAL): cross-owner binding aliasing. The document identity index now maps each bound agent_pubkey to its owner and group-quarantines every entry when one pubkey is bound under two owners; same-owner reuse across entries stays healthy (§2.5). Per-entry validation could not see this document-wide alias of a process-global keyring identity. F2 (IMPORTANT): deploy-intent routing is now validated by validate_provider_config on read (fail -> Unreadable) and at the save_deploy_intents writer boundary, so a malformed or secret-bearing row is never exposed as authoritative routing (§2.1). F3 (IMPORTANT): the Phase-0 routing seam is present. A raw-record-by-slug lookup plus a typed MutationRoute decision route delete/inbound/import; merge_preserving_definitions fails closed when a plain save would delete or edit the shared slots of a library-projected record, while an unchanged projected re-pass rides through intact (§2.7). F4 (IMPORTANT): the P14-I2 provenance regressions are added — legacy byte-compat round-trip, a concurrent deploy-success pair-churn rollback, and a non-None deploy stamp surviving apply_definition_view/into_agent_record. F5 (MINOR): deferred_archives gains encapsulated upsert_deferred_archive (SET semantics on (scope_id, agent_pubkey)) and a deferred_archive_obligations read view that collapses legacy duplicate rows to one obligation (§2.3). Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This commit is contained in:
@@ -201,4 +201,43 @@ mod tests {
|
||||
assert_eq!(records[0].last_error.as_deref(), Some("harness exited"));
|
||||
assert_eq!(records[0].updated_at, "runtime-change");
|
||||
}
|
||||
|
||||
/// P14-I2: a provider deploy success lands the inseparable pair
|
||||
/// `{backend_agent_id, last_completed_deploy_attempt_id}` on the live record
|
||||
/// WHILE a rename's profile sync is still awaiting; the profile step then
|
||||
/// fails and rollback runs. `copy_runtime_state` carries the pair as one
|
||||
/// unit, so `same_configuration` still matches (no equality-mismatch
|
||||
/// refusal), the restored record keeps the exact pair (never an
|
||||
/// ID-without-stamp record), and the pre-rename configuration is restored.
|
||||
#[test]
|
||||
fn failed_profile_sync_carries_deploy_provenance_pair_through_rollback() {
|
||||
let previous = record("Old name", "before");
|
||||
let mut attempted = previous.clone();
|
||||
attempted.name = "New name".to_string();
|
||||
attempted.updated_at = "attempt".to_string();
|
||||
let rollback = AgentUpdateRollback::new(previous, &attempted);
|
||||
|
||||
// Concurrent provider success: the live record gains BOTH provenance
|
||||
// fields in the same write, plus normal runtime churn.
|
||||
let mut deployed = attempted;
|
||||
deployed.backend_agent_id = Some("backend-42".to_string());
|
||||
deployed.last_completed_deploy_attempt_id = Some("attempt-42".to_string());
|
||||
deployed.last_started_at = Some("started".to_string());
|
||||
deployed.updated_at = "deploy-landed".to_string();
|
||||
let mut records = vec![deployed];
|
||||
|
||||
restore_agent_update(&mut records, "abcd1234", rollback)
|
||||
.expect("a concurrent deploy success must not block the rename rollback");
|
||||
|
||||
// Configuration rolled back; the provenance PAIR survives intact.
|
||||
assert_eq!(records[0].name, "Old name");
|
||||
assert_eq!(records[0].backend_agent_id.as_deref(), Some("backend-42"));
|
||||
assert_eq!(
|
||||
records[0].last_completed_deploy_attempt_id.as_deref(),
|
||||
Some("attempt-42"),
|
||||
"the deploy-attempt stamp must ride the rollback with its backend id",
|
||||
);
|
||||
assert_eq!(records[0].last_started_at.as_deref(), Some("started"));
|
||||
assert_eq!(records[0].updated_at, "deploy-landed");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -155,9 +155,12 @@ pub(crate) struct LibraryEntry {
|
||||
/// Archive obligations deferred by a protected removal (§3.6 step 3).
|
||||
/// PERMANENT journaled retirement markers in v1 (P17-C1): no v1 path
|
||||
/// discharges them; their presence keeps `key_archive_protected(pubkey)`
|
||||
/// true. SET semantics on `(library_id, scope_id, agent_pubkey)` — every
|
||||
/// append is an upsert (P15-MINOR); reads treat legacy duplicate rows as
|
||||
/// one obligation.
|
||||
/// true. SET semantics on `(library_id, scope_id, agent_pubkey)`
|
||||
/// (`library_id` is fixed per entry). Insert ONLY through
|
||||
/// [`upsert_deferred_archive`](LibraryEntry::upsert_deferred_archive) — every
|
||||
/// append is an upsert (P15-MINOR) — and read through
|
||||
/// [`deferred_archive_obligations`](LibraryEntry::deferred_archive_obligations),
|
||||
/// which collapses legacy duplicate rows to one obligation.
|
||||
#[serde(default)]
|
||||
pub deferred_archives: Vec<DeferredArchive>,
|
||||
/// Authoritative per-scope membership — sole source for delete-confirm
|
||||
@@ -165,6 +168,42 @@ pub(crate) struct LibraryEntry {
|
||||
pub projections: BTreeMap<String, ProjectionEntry>,
|
||||
}
|
||||
|
||||
impl LibraryEntry {
|
||||
/// Upsert a deferred-archive obligation (§2.3, P15-MINOR). SET semantics on
|
||||
/// `(scope_id, agent_pubkey)` — `library_id` is fixed per entry — so §3.6's
|
||||
/// crash-then-re-consent retry can re-run the obligation write idempotently:
|
||||
/// a marker already present is a no-op, never a duplicate row. This is the
|
||||
/// ONLY insertion path; callers never push onto `deferred_archives`
|
||||
/// directly. Returns `true` iff a new marker was added.
|
||||
pub fn upsert_deferred_archive(&mut self, scope_id: String, agent_pubkey: String) -> bool {
|
||||
if self
|
||||
.deferred_archives
|
||||
.iter()
|
||||
.any(|d| d.scope_id == scope_id && d.agent_pubkey == agent_pubkey)
|
||||
{
|
||||
return false;
|
||||
}
|
||||
self.deferred_archives.push(DeferredArchive {
|
||||
scope_id,
|
||||
agent_pubkey,
|
||||
});
|
||||
true
|
||||
}
|
||||
|
||||
/// The deferred-archive obligations as a SET (§2.3): a legacy `Vec` with
|
||||
/// duplicate rows is collapsed to one obligation per `(scope_id,
|
||||
/// agent_pubkey)`. Phase-3 callers read obligations through this view, never
|
||||
/// the raw field, so multiplicity in old on-disk data never becomes
|
||||
/// multiplicity in behavior.
|
||||
pub fn deferred_archive_obligations(&self) -> Vec<&DeferredArchive> {
|
||||
let mut seen = std::collections::HashSet::new();
|
||||
self.deferred_archives
|
||||
.iter()
|
||||
.filter(|d| seen.insert((d.scope_id.as_str(), d.agent_pubkey.as_str())))
|
||||
.collect()
|
||||
}
|
||||
}
|
||||
|
||||
/// `(origin scope_id, origin slug)` — the share idempotency key (§2.3).
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)]
|
||||
#[serde(deny_unknown_fields)]
|
||||
@@ -455,15 +494,19 @@ fn validate_entry_bindings(entry: &LibraryEntry) -> Result<(), String> {
|
||||
|
||||
/// Build the document-wide identity index over the individually valid entries
|
||||
/// and return the indices of every collider (§2.1 rule; P6-I3). Collisions on
|
||||
/// (a) `library_id`, (b) live (non-`deleted`) [`OriginKey`], or (c) a
|
||||
/// non-terminal `(scope_id, local_slug)` projection claim group-quarantine ALL
|
||||
/// participants — picking a winner would silently rewrite a scope.
|
||||
/// (a) `library_id`, (b) live (non-`deleted`) [`OriginKey`], (c) a non-terminal
|
||||
/// `(scope_id, local_slug)` projection claim, or (d) one `agent_pubkey` bound
|
||||
/// under two DIFFERENT owners anywhere in the document group-quarantine ALL
|
||||
/// participants — picking a winner would silently rewrite a scope or alias one
|
||||
/// global keyring identity across owner authorities.
|
||||
fn identity_collisions<'a>(
|
||||
entries: impl Iterator<Item = &'a LibraryEntry>,
|
||||
) -> std::collections::HashSet<usize> {
|
||||
let mut by_library_id: HashMap<&str, Vec<usize>> = HashMap::new();
|
||||
let mut by_origin: HashMap<&OriginKey, Vec<usize>> = HashMap::new();
|
||||
let mut by_scope_slug: HashMap<(&str, &str), Vec<usize>> = HashMap::new();
|
||||
// agent_pubkey → every (entry index, owner) it is bound under document-wide.
|
||||
let mut by_bound_agent: HashMap<&str, Vec<(usize, &str)>> = HashMap::new();
|
||||
|
||||
for (index, entry) in entries.enumerate() {
|
||||
by_library_id
|
||||
@@ -481,6 +524,12 @@ fn identity_collisions<'a>(
|
||||
.push(index);
|
||||
}
|
||||
}
|
||||
for (owner_hex, binding) in &entry.identity_bindings {
|
||||
by_bound_agent
|
||||
.entry(binding.agent_pubkey.as_str())
|
||||
.or_default()
|
||||
.push((index, owner_hex.as_str()));
|
||||
}
|
||||
}
|
||||
|
||||
let mut colliders = std::collections::HashSet::new();
|
||||
@@ -493,6 +542,19 @@ fn identity_collisions<'a>(
|
||||
colliders.extend(group.iter().copied());
|
||||
}
|
||||
}
|
||||
// (d) Cross-owner identity aliasing (§2.5, spec lines 1765-1766): the
|
||||
// keyring is process-global by pubkey, so one `agent_pubkey` bound under two
|
||||
// different owners aliases a single agent identity across owner authorities.
|
||||
// Group-quarantine every entry that binds a colliding pubkey; same-owner
|
||||
// reuse across entries is explicitly permitted and never collides. (A single
|
||||
// entry binding one pubkey under two owners is already rejected in pass 1.)
|
||||
for occurrences in by_bound_agent.values() {
|
||||
let distinct_owners: std::collections::HashSet<&str> =
|
||||
occurrences.iter().map(|(_, owner)| *owner).collect();
|
||||
if distinct_owners.len() > 1 {
|
||||
colliders.extend(occurrences.iter().map(|(index, _)| *index));
|
||||
}
|
||||
}
|
||||
colliders
|
||||
}
|
||||
|
||||
|
||||
@@ -13,10 +13,12 @@
|
||||
//! degrade loudly (they cannot journal a fresh key); no other scope and no
|
||||
//! library operation is affected.
|
||||
//! - `deploy-intents.json` unreadable OR semantically invalid (unknown version,
|
||||
//! syntax failure, or a duplicate `agent_pubkey` row — the at-most-one mutex
|
||||
//! is VALIDATED on read and group-rejected, never first-wins) → the scope's
|
||||
//! syntax failure, a duplicate `agent_pubkey` row — the at-most-one mutex is
|
||||
//! VALIDATED on read and group-rejected, never first-wins — or a row whose
|
||||
//! `provider_config` fails `validate_provider_config`) → the scope's
|
||||
//! destructive removal and new-deploy paths FAIL CLOSED, because an unreadable
|
||||
//! journal may hide an intent equivalent to a live remote deployment.
|
||||
//! journal may hide an intent equivalent to a live remote deployment, and an
|
||||
//! unvalidated row must never be exposed as authoritative routing.
|
||||
//!
|
||||
//! Unlike `library.json`, a corrupt journal is NOT copied to `.invalid`: both
|
||||
//! failure modes above refuse to write, so the corrupt file is never
|
||||
@@ -249,17 +251,42 @@ pub(crate) fn load_deploy_intents(definitions_dir: &Path) -> DeployIntentsLoad {
|
||||
"deploy-intents.json has duplicate rows for {pubkey}: the at-most-one mutex is violated"
|
||||
));
|
||||
}
|
||||
if let Err(reason) = validate_intent_routing(&journal.intents) {
|
||||
return DeployIntentsLoad::Unreadable(reason);
|
||||
}
|
||||
DeployIntentsLoad::Loaded(journal)
|
||||
}
|
||||
|
||||
/// Persist `deploy-intents.json`: atomic temp-file + rename + `0o600`.
|
||||
/// Persist `deploy-intents.json`: atomic temp-file + rename + `0o600`. Rejects
|
||||
/// any row whose `provider_config` fails `validate_provider_config`, so a
|
||||
/// crate-internal caller can never persist an invalid row that a later read
|
||||
/// would refuse (§2.1 row contract; the read-side check would otherwise fail a
|
||||
/// scope's destructive paths closed against state this process itself wrote).
|
||||
pub(crate) fn save_deploy_intents(
|
||||
definitions_dir: &Path,
|
||||
journal: &DeployIntentsJournal,
|
||||
) -> Result<(), String> {
|
||||
validate_intent_routing(&journal.intents)?;
|
||||
save_journal(&deploy_intents_path(definitions_dir), journal)
|
||||
}
|
||||
|
||||
/// Validate every row's `provider_config` (§2.1: "validated by
|
||||
/// `validate_provider_config`, never secret"). A malformed or secret-bearing
|
||||
/// hand-edited row must never be exposed as authoritative routing.
|
||||
fn validate_intent_routing(intents: &[DeployIntent]) -> Result<(), String> {
|
||||
for intent in intents {
|
||||
crate::managed_agents::backend::validate_provider_config(&intent.provider_config).map_err(
|
||||
|reason| {
|
||||
format!(
|
||||
"deploy-intents.json row for {} has invalid provider_config: {reason}",
|
||||
intent.agent_pubkey
|
||||
)
|
||||
},
|
||||
)?;
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// The first `agent_pubkey` that appears in more than one row, if any.
|
||||
fn duplicate_pubkey(intents: &[DeployIntent]) -> Option<&str> {
|
||||
let mut counts: HashMap<&str, usize> = HashMap::new();
|
||||
|
||||
@@ -308,6 +308,80 @@ fn test_one_agent_bound_under_two_owners_quarantines() {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_agent_bound_under_two_owners_across_entries_group_quarantines() {
|
||||
let dir = tempdir().unwrap();
|
||||
let owner_a = Keys::generate();
|
||||
let owner_b = Keys::generate();
|
||||
let agent = Keys::generate();
|
||||
// Two individually-valid entries with distinct library_ids AND distinct
|
||||
// origins, each binding the SAME agent pubkey but under a DIFFERENT owner.
|
||||
// The keyring is process-global by pubkey, so this aliases one agent
|
||||
// identity across owner authorities — a document-wide collision that the
|
||||
// per-entry `seen_agents` pass cannot see (§2.5, spec lines 1761-1762).
|
||||
let a = value_of(&entry(
|
||||
"lib-cross-a",
|
||||
"scope-a",
|
||||
"aria",
|
||||
Some((&owner_a, &agent)),
|
||||
));
|
||||
let b = value_of(&entry(
|
||||
"lib-cross-b",
|
||||
"scope-b",
|
||||
"bram",
|
||||
Some((&owner_b, &agent)),
|
||||
));
|
||||
// A healthy sibling binding a DIFFERENT agent stays fully usable.
|
||||
let sibling_owner = Keys::generate();
|
||||
let sibling_agent = Keys::generate();
|
||||
let sibling = value_of(&entry(
|
||||
"lib-sibling",
|
||||
"scope-s",
|
||||
"sara",
|
||||
Some((&sibling_owner, &sibling_agent)),
|
||||
));
|
||||
write_doc(dir.path(), &doc(vec![a.clone(), b.clone(), sibling]));
|
||||
|
||||
match load_library(dir.path()) {
|
||||
LibraryLoad::Loaded(loaded) => {
|
||||
assert_eq!(loaded.healthy.len(), 1);
|
||||
assert_eq!(loaded.healthy[0].library_id, "lib-sibling");
|
||||
assert_eq!(loaded.quarantined.len(), 2);
|
||||
assert!(loaded.quarantined.contains(&a));
|
||||
assert!(loaded.quarantined.contains(&b));
|
||||
}
|
||||
other => panic!("expected Loaded, got {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_same_owner_agent_reuse_across_entries_stays_healthy() {
|
||||
let dir = tempdir().unwrap();
|
||||
let owner = Keys::generate();
|
||||
let agent = Keys::generate();
|
||||
// The SAME owner binding the SAME agent across multiple entries is the
|
||||
// identity auto-carry across a same-owner's workspaces (§2.5) — explicitly
|
||||
// permitted, NOT a cross-owner alias. Neither entry may quarantine.
|
||||
let a = value_of(&entry(
|
||||
"lib-reuse-a",
|
||||
"scope-a",
|
||||
"aria",
|
||||
Some((&owner, &agent)),
|
||||
));
|
||||
let b = value_of(&entry(
|
||||
"lib-reuse-b",
|
||||
"scope-b",
|
||||
"bram",
|
||||
Some((&owner, &agent)),
|
||||
));
|
||||
write_doc(dir.path(), &doc(vec![a, b]));
|
||||
|
||||
match load_library(dir.path()) {
|
||||
LibraryLoad::Loaded(loaded) => assert_eq!(loaded.healthy.len(), 2),
|
||||
other => panic!("expected Loaded, got {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_duplicate_library_id_group_quarantines_all_colliders() {
|
||||
let dir = tempdir().unwrap();
|
||||
@@ -525,6 +599,61 @@ fn test_apply_shared_definition_empty_prompt_maps_to_none() {
|
||||
assert_eq!(record.system_prompt, None);
|
||||
}
|
||||
|
||||
// ── §2.3: deferred_archives SET semantics (P15-MINOR) ───────────────────────────
|
||||
|
||||
#[test]
|
||||
fn test_upsert_deferred_archive_is_idempotent_across_retries() {
|
||||
let mut e = entry("lib-def", "scope-d", "dora", None);
|
||||
// Simulate §3.6's crash-then-re-consent retry: the same obligation write
|
||||
// re-runs three times. SET semantics keep exactly one marker.
|
||||
for _ in 0..3 {
|
||||
e.upsert_deferred_archive("scope-x".to_string(), "agent-pk".to_string());
|
||||
}
|
||||
assert_eq!(e.deferred_archives.len(), 1);
|
||||
assert_eq!(e.deferred_archive_obligations().len(), 1);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_upsert_deferred_archive_reports_new_versus_existing() {
|
||||
let mut e = entry("lib-def", "scope-d", "dora", None);
|
||||
assert!(e.upsert_deferred_archive("scope-x".to_string(), "agent-a".to_string()));
|
||||
// Same coordinate → no-op.
|
||||
assert!(!e.upsert_deferred_archive("scope-x".to_string(), "agent-a".to_string()));
|
||||
// A different scope or agent is a distinct obligation.
|
||||
assert!(e.upsert_deferred_archive("scope-y".to_string(), "agent-a".to_string()));
|
||||
assert!(e.upsert_deferred_archive("scope-x".to_string(), "agent-b".to_string()));
|
||||
assert_eq!(e.deferred_archives.len(), 3);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_legacy_duplicate_deferred_archives_read_as_one_obligation() {
|
||||
// A legacy on-disk entry with duplicate rows (written before the upsert
|
||||
// API existed) must READ as one obligation per (scope_id, agent_pubkey).
|
||||
let mut e = entry("lib-legacy", "scope-l", "leo", None);
|
||||
e.deferred_archives = vec![
|
||||
DeferredArchive {
|
||||
scope_id: "scope-x".to_string(),
|
||||
agent_pubkey: "agent-a".to_string(),
|
||||
},
|
||||
DeferredArchive {
|
||||
scope_id: "scope-x".to_string(),
|
||||
agent_pubkey: "agent-a".to_string(),
|
||||
},
|
||||
DeferredArchive {
|
||||
scope_id: "scope-y".to_string(),
|
||||
agent_pubkey: "agent-a".to_string(),
|
||||
},
|
||||
];
|
||||
let obligations = e.deferred_archive_obligations();
|
||||
assert_eq!(obligations.len(), 2);
|
||||
assert!(obligations
|
||||
.iter()
|
||||
.any(|d| d.scope_id == "scope-x" && d.agent_pubkey == "agent-a"));
|
||||
assert!(obligations
|
||||
.iter()
|
||||
.any(|d| d.scope_id == "scope-y" && d.agent_pubkey == "agent-a"));
|
||||
}
|
||||
|
||||
// ── scope-local journals (P9-I1 / P10-C3) ───────────────────────────────────────
|
||||
|
||||
#[test]
|
||||
@@ -599,7 +728,7 @@ fn test_deploy_intents_round_trip_preserves_phase() {
|
||||
},
|
||||
},
|
||||
provider_id: "fly".to_string(),
|
||||
provider_config: json!(null),
|
||||
provider_config: json!({"region": "sjc"}),
|
||||
created_at: "2026-08-11T00:01:00Z".to_string(),
|
||||
},
|
||||
],
|
||||
@@ -650,6 +779,110 @@ fn test_deploy_intents_unknown_field_is_unreadable() {
|
||||
));
|
||||
}
|
||||
|
||||
/// A `Running` deploy row with the given `provider_config`, for routing-
|
||||
/// validation fixtures.
|
||||
fn intent_with_config(pubkey: &str, provider_config: Value) -> DeployIntent {
|
||||
DeployIntent {
|
||||
agent_pubkey: pubkey.to_string(),
|
||||
phase: DeployIntentPhase::Running {
|
||||
attempt_id: "a".to_string(),
|
||||
},
|
||||
provider_id: "fly".to_string(),
|
||||
provider_config,
|
||||
created_at: "t".to_string(),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_deploy_intents_non_object_provider_config_is_unreadable() {
|
||||
let dir = tempdir().unwrap();
|
||||
// A hand-edited row whose provider_config is a bare scalar, not an object.
|
||||
// `validate_provider_config` rejects it; the row must not be exposed as
|
||||
// authoritative routing (§2.1 row contract).
|
||||
std::fs::write(
|
||||
deploy_intents_path(dir.path()),
|
||||
serde_json::to_vec(&DeployIntentsJournal {
|
||||
version: SUPPORTED_JOURNAL_VERSION,
|
||||
intents: vec![intent_with_config("pk", json!("us-east"))],
|
||||
})
|
||||
.unwrap(),
|
||||
)
|
||||
.unwrap();
|
||||
match load_deploy_intents(dir.path()) {
|
||||
DeployIntentsLoad::Unreadable(reason) => assert!(reason.contains("provider_config")),
|
||||
other => panic!("expected Unreadable, got {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_deploy_intents_nested_provider_config_is_unreadable() {
|
||||
let dir = tempdir().unwrap();
|
||||
// Nested (non-scalar) values are rejected — routing must be a flat object.
|
||||
std::fs::write(
|
||||
deploy_intents_path(dir.path()),
|
||||
serde_json::to_vec(&DeployIntentsJournal {
|
||||
version: SUPPORTED_JOURNAL_VERSION,
|
||||
intents: vec![intent_with_config("pk", json!({"region": {"nested": 1}}))],
|
||||
})
|
||||
.unwrap(),
|
||||
)
|
||||
.unwrap();
|
||||
assert!(matches!(
|
||||
load_deploy_intents(dir.path()),
|
||||
DeployIntentsLoad::Unreadable(_)
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_deploy_intents_secret_key_provider_config_is_unreadable() {
|
||||
let dir = tempdir().unwrap();
|
||||
// A secret-like key is rejected — provider_config is never secret (§2.1).
|
||||
std::fs::write(
|
||||
deploy_intents_path(dir.path()),
|
||||
serde_json::to_vec(&DeployIntentsJournal {
|
||||
version: SUPPORTED_JOURNAL_VERSION,
|
||||
intents: vec![intent_with_config("pk", json!({"api_key": "leak"}))],
|
||||
})
|
||||
.unwrap(),
|
||||
)
|
||||
.unwrap();
|
||||
assert!(matches!(
|
||||
load_deploy_intents(dir.path()),
|
||||
DeployIntentsLoad::Unreadable(_)
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_deploy_intents_valid_scalar_object_provider_config_loads() {
|
||||
let dir = tempdir().unwrap();
|
||||
// A flat object of scalar, non-secret values is the healthy shape.
|
||||
let journal = DeployIntentsJournal {
|
||||
version: SUPPORTED_JOURNAL_VERSION,
|
||||
intents: vec![intent_with_config(
|
||||
"pk",
|
||||
json!({"region": "iad", "size": 2}),
|
||||
)],
|
||||
};
|
||||
save_deploy_intents(dir.path(), &journal).unwrap();
|
||||
match load_deploy_intents(dir.path()) {
|
||||
DeployIntentsLoad::Loaded(j) => assert_eq!(j, journal),
|
||||
other => panic!("expected Loaded, got {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_save_deploy_intents_rejects_invalid_provider_config() {
|
||||
let dir = tempdir().unwrap();
|
||||
// The writer boundary refuses an invalid row, so no crate-internal caller
|
||||
// can persist state a later read would fail closed against.
|
||||
let journal = DeployIntentsJournal {
|
||||
version: SUPPORTED_JOURNAL_VERSION,
|
||||
intents: vec![intent_with_config("pk", json!({"secret": "x"}))],
|
||||
};
|
||||
assert!(save_deploy_intents(dir.path(), &journal).is_err());
|
||||
assert!(!deploy_intents_path(dir.path()).exists());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_journals_written_at_0600() {
|
||||
let dir = tempdir().unwrap();
|
||||
|
||||
@@ -480,7 +480,7 @@ pub fn save_personas<R: tauri::Runtime>(
|
||||
records: &[AgentDefinition],
|
||||
) -> Result<(), String> {
|
||||
let existing = crate::managed_agents::storage::load_agent_definitions(app)?;
|
||||
let definitions = merge_preserving_definitions(existing, records);
|
||||
let definitions = merge_preserving_definitions(existing, records)?;
|
||||
crate::managed_agents::storage::save_agent_definitions(app, &definitions)
|
||||
}
|
||||
|
||||
@@ -492,10 +492,60 @@ pub(crate) fn save_personas_at(
|
||||
records: &[AgentDefinition],
|
||||
) -> Result<(), String> {
|
||||
let existing = crate::managed_agents::storage::load_agent_definitions_at(definitions_dir)?;
|
||||
let definitions = merge_preserving_definitions(existing, records);
|
||||
let definitions = merge_preserving_definitions(existing, records)?;
|
||||
crate::managed_agents::storage::save_agent_definitions_at(definitions_dir, &definitions)
|
||||
}
|
||||
|
||||
/// The canonical raw-record-by-slug lookup (§2.7, §8 Phase 0). One place
|
||||
/// resolves a slug to its authoritative on-disk record so the library-aware
|
||||
/// routing decision below is identical for delete, inbound upsert/tombstone,
|
||||
/// and snapshot/team import. Consumed by §3's removal and inbound branches once
|
||||
/// the projection state machine lands; used by [`MutationRoute::for_slug`] now.
|
||||
#[allow(dead_code)]
|
||||
pub(crate) fn raw_record_by_slug<'a>(
|
||||
records: &'a [ManagedAgentRecord],
|
||||
slug: &str,
|
||||
) -> Option<&'a ManagedAgentRecord> {
|
||||
records
|
||||
.iter()
|
||||
.find(|record| record.slug.as_deref() == Some(slug))
|
||||
}
|
||||
|
||||
/// Where a persona mutation on a slug must be routed (§2.7). A keyless record
|
||||
/// carrying `library_ref` is a library projection: deleting it or overwriting
|
||||
/// its authoritative shared slots is a library operation that must go through
|
||||
/// the §3.4 workspace-remove state machine (`ExcludePending` intent first) or
|
||||
/// the §3 library-authoritative inbound branch — NEVER the plain local writer.
|
||||
/// A record with no `library_ref` (and a brand-new persona) takes the
|
||||
/// head-identical plain path.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub(crate) enum MutationRoute {
|
||||
/// Plain keyless record (or a new persona): the head-identical local path.
|
||||
Plain,
|
||||
/// Library-projected record: routing is a §3 deliverable. Until it lands,
|
||||
/// the projected path fails closed so no plain writer can silently drop or
|
||||
/// overwrite a shared definition.
|
||||
LibraryProjected,
|
||||
}
|
||||
|
||||
impl MutationRoute {
|
||||
/// Classify a mutation by its currently-stored record. `None` (no existing
|
||||
/// record — a create) is always [`Plain`](Self::Plain).
|
||||
fn for_record(record: Option<&ManagedAgentRecord>) -> Self {
|
||||
match record {
|
||||
Some(record) if record.library_ref.is_some() => Self::LibraryProjected,
|
||||
_ => Self::Plain,
|
||||
}
|
||||
}
|
||||
|
||||
/// The route for a mutation targeting `slug` against the raw store — the
|
||||
/// decision delete/inbound/import consult before taking the plain path.
|
||||
#[allow(dead_code)]
|
||||
pub(crate) fn for_slug(records: &[ManagedAgentRecord], slug: &str) -> Self {
|
||||
Self::for_record(raw_record_by_slug(records, slug))
|
||||
}
|
||||
}
|
||||
|
||||
/// Build the definition half of a persona save so that every field living only
|
||||
/// on [`ManagedAgentRecord`] — `library_ref`, `library_applied_revision`,
|
||||
/// `last_completed_deploy_attempt_id`, and any future non-view slot — survives
|
||||
@@ -514,31 +564,69 @@ pub(crate) fn save_personas_at(
|
||||
/// update, activation toggle, delete, inbound upsert/tombstone, snapshot/team
|
||||
/// import, team deletion) merge-preserving *by construction*.
|
||||
///
|
||||
/// A raw record absent from `views` is a deletion, dropped from the result —
|
||||
/// the head-identical plain path. Library-aware deletion routing (§3.4's
|
||||
/// `ExcludePending` state machine) and the library-authoritative inbound branch
|
||||
/// (§2.7 P4-C2) layer on at §3, where `apply_shared_definition` exists; at
|
||||
/// Phase 0 no production path authors `library_ref`, so the plain path is the
|
||||
/// only reachable one and matches head exactly.
|
||||
/// **Library-aware routing (§2.7, resolves P4-C1/P4-C2).** A [`LibraryProjected`]
|
||||
/// record must not be mutated by this plain path:
|
||||
/// - a projected record absent from `views` is a deletion that must advance the
|
||||
/// §3.4 `ExcludePending` state machine, not silently drop the row;
|
||||
/// - a `views` entry that would change a projected record's shared slots is a
|
||||
/// shared-definition edit (ruling 3a) or a library-linked inbound upsert
|
||||
/// (P4-C2) that must route through the library, not overwrite the local cache
|
||||
/// with no revision.
|
||||
///
|
||||
/// Both fail closed until §3 wires the state machine. A view that leaves a
|
||||
/// projected record unchanged (an unrelated writer re-passing the projected
|
||||
/// view, stripped of its metadata) is not a mutation and rides through intact —
|
||||
/// the reason `library_ref` survives every writer above. No production path
|
||||
/// authors `library_ref` at Phase 0, so the projected branch is an unreachable
|
||||
/// guard rail until §3 populates it.
|
||||
///
|
||||
/// [`LibraryProjected`]: MutationRoute::LibraryProjected
|
||||
fn merge_preserving_definitions(
|
||||
existing: Vec<crate::managed_agents::ManagedAgentRecord>,
|
||||
views: &[AgentDefinition],
|
||||
) -> Vec<crate::managed_agents::ManagedAgentRecord> {
|
||||
let mut by_slug: std::collections::HashMap<String, _> = existing
|
||||
) -> Result<Vec<crate::managed_agents::ManagedAgentRecord>, String> {
|
||||
let mut by_slug: std::collections::HashMap<String, ManagedAgentRecord> = existing
|
||||
.into_iter()
|
||||
.filter_map(|record| record.slug.clone().map(|slug| (slug, record)))
|
||||
.collect();
|
||||
|
||||
views
|
||||
.iter()
|
||||
.map(|view| match by_slug.remove(&view.id) {
|
||||
let mut merged = Vec::with_capacity(views.len());
|
||||
for view in views {
|
||||
match by_slug.remove(&view.id) {
|
||||
Some(mut record) => {
|
||||
record.apply_definition_view(view);
|
||||
record
|
||||
if MutationRoute::for_record(Some(&record)) == MutationRoute::LibraryProjected {
|
||||
let mut candidate = record.clone();
|
||||
candidate.apply_definition_view(view);
|
||||
if candidate != record {
|
||||
return Err(format!(
|
||||
"persona '{}' is a library projection: shared-content edits must \
|
||||
route through the library, not a plain persona save (§2.7)",
|
||||
view.id
|
||||
));
|
||||
}
|
||||
} else {
|
||||
record.apply_definition_view(view);
|
||||
}
|
||||
merged.push(record);
|
||||
}
|
||||
None => view.clone().into_agent_record(),
|
||||
})
|
||||
.collect()
|
||||
None => merged.push(view.clone().into_agent_record()),
|
||||
}
|
||||
}
|
||||
|
||||
// Every record left in `by_slug` is absent from `views` — a deletion. A
|
||||
// plain record drops exactly as at head; a projected record must fail closed
|
||||
// (§3.4 removal routing not yet wired).
|
||||
for leftover in by_slug.values() {
|
||||
if MutationRoute::for_record(Some(leftover)) == MutationRoute::LibraryProjected {
|
||||
return Err(format!(
|
||||
"persona '{}' is a library projection: removal must route through the §3.4 \
|
||||
ExcludePending state machine, not a plain persona save (§2.7)",
|
||||
leftover.slug.as_deref().unwrap_or("<unknown>")
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
Ok(merged)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
||||
@@ -536,11 +536,87 @@ fn merge_preserving_save_keeps_library_metadata_across_every_writer_shape() {
|
||||
.iter()
|
||||
.filter_map(|record| record.to_definition_view())
|
||||
.collect();
|
||||
let saved = merge_preserving_definitions(existing, &transform(views));
|
||||
let saved = merge_preserving_definitions(existing, &transform(views))
|
||||
.unwrap_or_else(|e| panic!("{label}: unrelated writer must not fail: {e}"));
|
||||
assert_projection_survived(&saved, label);
|
||||
}
|
||||
}
|
||||
|
||||
/// §2.7 fail-closed routing (P4-C1/P4-C2): a plain persona save must not be
|
||||
/// able to DELETE a library-projected record. Dropping the projected view from
|
||||
/// the save vector is a removal that must advance the §3.4 `ExcludePending`
|
||||
/// state machine; until that lands the seam rejects it rather than silently
|
||||
/// dropping the row.
|
||||
#[test]
|
||||
fn merge_preserving_save_rejects_deleting_a_projected_record() {
|
||||
let existing = vec![
|
||||
projected_record("shared", 3),
|
||||
custom_persona("custom:plain", "Plain").into_agent_record(),
|
||||
];
|
||||
// The save vector omits "shared" entirely — a deletion of the projection.
|
||||
let views = vec![custom_persona("custom:plain", "Plain")];
|
||||
let err = merge_preserving_definitions(existing, &views)
|
||||
.expect_err("deleting a projected record must fail closed");
|
||||
assert!(err.contains("shared") && err.contains("removal"), "{err}");
|
||||
}
|
||||
|
||||
/// §2.7 fail-closed routing (P4-C2): a plain persona save must not overwrite a
|
||||
/// projected record's shared content. A view that changes a shared slot is a
|
||||
/// library edit / library-linked inbound upsert that must route through the
|
||||
/// library with a revision, not become an unjournaled local edit.
|
||||
#[test]
|
||||
fn merge_preserving_save_rejects_editing_a_projected_records_shared_slot() {
|
||||
let existing = vec![projected_record("shared", 3)];
|
||||
// Re-pass the projected view but mutate a shared slot (display name).
|
||||
let mut view = existing[0].to_definition_view().expect("view");
|
||||
view.display_name = "Hijacked".to_string();
|
||||
let err = merge_preserving_definitions(existing, &[view])
|
||||
.expect_err("editing projected shared content must fail closed");
|
||||
assert!(err.contains("shared") && err.contains("edits"), "{err}");
|
||||
}
|
||||
|
||||
/// The no-op case that keeps the every-writer guarantee honest: re-passing a
|
||||
/// projected record's own view (metadata stripped, no field changed) is NOT a
|
||||
/// mutation and must ride through intact — otherwise an unrelated writer that
|
||||
/// re-serializes the whole store would trip the fail-closed guard.
|
||||
#[test]
|
||||
fn merge_preserving_save_passes_unchanged_projected_record_through() {
|
||||
let existing = vec![
|
||||
projected_record("shared", 3),
|
||||
custom_persona("custom:plain", "Plain").into_agent_record(),
|
||||
];
|
||||
let views: Vec<AgentDefinition> = existing
|
||||
.iter()
|
||||
.filter_map(|record| record.to_definition_view())
|
||||
.collect();
|
||||
let saved =
|
||||
merge_preserving_definitions(existing, &views).expect("unchanged projection must pass");
|
||||
assert_projection_survived(&saved, "noop_reserialize");
|
||||
}
|
||||
|
||||
/// `MutationRoute::for_slug` classifies a projected slug as `LibraryProjected`,
|
||||
/// a plain slug and an unknown slug (a create) as `Plain` — the decision every
|
||||
/// §3 delete/inbound/import caller consults before the plain path.
|
||||
#[test]
|
||||
fn mutation_route_classifies_projected_plain_and_unknown_slugs() {
|
||||
let records = vec![
|
||||
projected_record("shared", 3),
|
||||
custom_persona("custom:plain", "Plain").into_agent_record(),
|
||||
];
|
||||
assert_eq!(
|
||||
super::MutationRoute::for_slug(&records, "shared"),
|
||||
super::MutationRoute::LibraryProjected,
|
||||
);
|
||||
assert_eq!(
|
||||
super::MutationRoute::for_slug(&records, "custom:plain"),
|
||||
super::MutationRoute::Plain,
|
||||
);
|
||||
assert_eq!(
|
||||
super::MutationRoute::for_slug(&records, "does-not-exist"),
|
||||
super::MutationRoute::Plain,
|
||||
);
|
||||
}
|
||||
|
||||
/// The same guarantee through the on-disk `_at` seam — proving the storage
|
||||
/// layer and the built-in-merge write-back preserve the metadata too — plus the
|
||||
/// §2.7 read-side exposure (P4-C1): `load_persona_views_at` surfaces the
|
||||
|
||||
@@ -583,6 +583,71 @@ fn empty_prompt_folds_to_none() {
|
||||
assert_eq!(persona.into_agent_record().system_prompt, None);
|
||||
}
|
||||
|
||||
/// P13-I3 / invariant 4: a legacy head-serialized record (no `library_ref`,
|
||||
/// no `library_applied_revision`, no `last_completed_deploy_attempt_id`) reads
|
||||
/// with all three `None` and re-serializes WITHOUT resurrecting the keys —
|
||||
/// `skip_serializing_if` keeps it byte-identical to head, so a device that
|
||||
/// never touched the library is untouched by the fold.
|
||||
#[test]
|
||||
fn legacy_record_without_deploy_provenance_round_trips_byte_identically() {
|
||||
let legacy = r#"{
|
||||
"pubkey": "abcd1234",
|
||||
"name": "test-agent",
|
||||
"private_key_nsec": "nsec1fake",
|
||||
"relay_url": "wss://localhost:3000",
|
||||
"acp_command": "buzz-acp",
|
||||
"agent_command": "goose",
|
||||
"agent_args": [],
|
||||
"mcp_command": "",
|
||||
"turn_timeout_seconds": 320,
|
||||
"system_prompt": null,
|
||||
"created_at": "2026-01-01T00:00:00Z",
|
||||
"updated_at": "2026-01-01T00:00:00Z",
|
||||
"last_started_at": null,
|
||||
"last_stopped_at": null,
|
||||
"last_exit_code": null,
|
||||
"last_error": null
|
||||
}"#;
|
||||
let record: ManagedAgentRecord =
|
||||
serde_json::from_str(legacy).expect("legacy record deserializes");
|
||||
assert_eq!(record.library_ref, None);
|
||||
assert_eq!(record.library_applied_revision, None);
|
||||
assert_eq!(record.last_completed_deploy_attempt_id, None);
|
||||
|
||||
let reserialized = serde_json::to_value(&record).expect("serialize");
|
||||
let object = reserialized.as_object().expect("record is an object");
|
||||
assert!(!object.contains_key("library_ref"));
|
||||
assert!(!object.contains_key("library_applied_revision"));
|
||||
assert!(!object.contains_key("last_completed_deploy_attempt_id"));
|
||||
}
|
||||
|
||||
/// P14-I2: the deploy-attempt stamp is not projection metadata, so a fresh
|
||||
/// projection carries it as `None`, and an ordinary persona save — which
|
||||
/// applies a definition VIEW onto a canonical record — must never shed a
|
||||
/// non-`None` stamp (the view cannot carry it, so `apply_definition_view` must
|
||||
/// leave it untouched).
|
||||
#[test]
|
||||
fn deploy_attempt_stamp_survives_into_record_and_apply_view() {
|
||||
// Fresh projection: no stamp.
|
||||
let mut record = sample_persona().into_agent_record();
|
||||
assert_eq!(record.last_completed_deploy_attempt_id, None);
|
||||
|
||||
// Seed a landed deploy stamp, then apply an unrelated definition edit.
|
||||
record.last_completed_deploy_attempt_id = Some("attempt-42".to_string());
|
||||
record.backend_agent_id = Some("backend-7".to_string());
|
||||
let mut edited = sample_persona();
|
||||
edited.display_name = "Renamed".to_string();
|
||||
record.apply_definition_view(&edited);
|
||||
|
||||
assert_eq!(record.display_name.as_deref(), Some("Renamed"));
|
||||
assert_eq!(
|
||||
record.last_completed_deploy_attempt_id.as_deref(),
|
||||
Some("attempt-42"),
|
||||
"apply_definition_view must never shed the deploy-attempt stamp",
|
||||
);
|
||||
assert_eq!(record.backend_agent_id.as_deref(), Some("backend-7"));
|
||||
}
|
||||
|
||||
// ── Mint-time behavioral defaults (B5 quad activation) ──────────────────────
|
||||
|
||||
use super::resolve_mint_behavioral_defaults;
|
||||
|
||||
Reference in New Issue
Block a user