Fix: UPDATE branch of upsert_entry dropped doc_refs + valid_until #3
@ -252,11 +252,21 @@ pub struct BoostEntry {
|
|||||||
/// Phase 26 — what happened during an upsert. The seed endpoint
|
/// Phase 26 — what happened during an upsert. The seed endpoint
|
||||||
/// returns this shape so the caller sees whether its write was a new
|
/// returns this shape so the caller sees whether its write was a new
|
||||||
/// entry, a merge, or a dedup'd no-op.
|
/// entry, a merge, or a dedup'd no-op.
|
||||||
|
///
|
||||||
|
/// SHAPE NOTE: `#[serde(tag = "mode")]` requires struct-like variants —
|
||||||
|
/// bare `Added(String)` and `Noop(String)` newtype variants would
|
||||||
|
/// panic serialization at runtime. That bug silently 500-ed every
|
||||||
|
/// /seed call from Phase 26 (commit 640db8c) until 2026-04-22 when the
|
||||||
|
/// auditor's hybrid fixture surfaced it. All three variants are now
|
||||||
|
/// struct-like, producing uniform JSON:
|
||||||
|
/// {"mode":"added","playbook_id":"pb-..."}
|
||||||
|
/// {"mode":"updated","playbook_id":"pb-...","merged_names":[...]}
|
||||||
|
/// {"mode":"noop","playbook_id":"pb-..."}
|
||||||
#[derive(Debug, Clone, Serialize)]
|
#[derive(Debug, Clone, Serialize)]
|
||||||
#[serde(tag = "mode", rename_all = "lowercase")]
|
#[serde(tag = "mode", rename_all = "lowercase")]
|
||||||
pub enum UpsertOutcome {
|
pub enum UpsertOutcome {
|
||||||
/// New playbook appended. Carries the new playbook_id.
|
/// New playbook appended. Carries the new playbook_id.
|
||||||
Added(String),
|
Added { playbook_id: String },
|
||||||
/// Existing same-day entry updated. Playbook_id unchanged; names
|
/// Existing same-day entry updated. Playbook_id unchanged; names
|
||||||
/// merged (union, original order preserved, new names appended).
|
/// merged (union, original order preserved, new names appended).
|
||||||
Updated {
|
Updated {
|
||||||
@ -265,7 +275,7 @@ pub enum UpsertOutcome {
|
|||||||
},
|
},
|
||||||
/// Identical same-day entry already exists; nothing changed.
|
/// Identical same-day entry already exists; nothing changed.
|
||||||
/// Returns the stable playbook_id so caller still has a reference.
|
/// Returns the stable playbook_id so caller still has a reference.
|
||||||
Noop(String),
|
Noop { playbook_id: String },
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Phase 27 — shape returned from `revise_entry`. Reports both ends of
|
/// Phase 27 — shape returned from `revise_entry`. Reports both ends of
|
||||||
@ -597,7 +607,7 @@ impl PlaybookMemory {
|
|||||||
drop(state);
|
drop(state);
|
||||||
self.persist().await?;
|
self.persist().await?;
|
||||||
self.rebuild_geo_index().await;
|
self.rebuild_geo_index().await;
|
||||||
Ok(UpsertOutcome::Added(pid))
|
Ok(UpsertOutcome::Added { playbook_id: pid })
|
||||||
}
|
}
|
||||||
Some(i) => {
|
Some(i) => {
|
||||||
let mut existing_names_sorted = state.entries[i].endorsed_names.clone();
|
let mut existing_names_sorted = state.entries[i].endorsed_names.clone();
|
||||||
@ -605,7 +615,7 @@ impl PlaybookMemory {
|
|||||||
if existing_names_sorted == new_names_sorted {
|
if existing_names_sorted == new_names_sorted {
|
||||||
// NOOP — identical data, just report the existing id
|
// NOOP — identical data, just report the existing id
|
||||||
let pid = state.entries[i].playbook_id.clone();
|
let pid = state.entries[i].playbook_id.clone();
|
||||||
Ok(UpsertOutcome::Noop(pid))
|
Ok(UpsertOutcome::Noop { playbook_id: pid })
|
||||||
} else {
|
} else {
|
||||||
// UPDATE — merge names (union, stable order).
|
// UPDATE — merge names (union, stable order).
|
||||||
let existing = state.entries.get_mut(i).ok_or("index invalidated")?;
|
let existing = state.entries.get_mut(i).ok_or("index invalidated")?;
|
||||||
@ -615,15 +625,45 @@ impl PlaybookMemory {
|
|||||||
}
|
}
|
||||||
existing.endorsed_names = merged.clone();
|
existing.endorsed_names = merged.clone();
|
||||||
existing.timestamp = new_entry.timestamp.clone();
|
existing.timestamp = new_entry.timestamp.clone();
|
||||||
// Keep original playbook_id + embedding + schema fingerprint.
|
// Keep original playbook_id. Refresh embedding only
|
||||||
// Refresh embedding only if the caller passed a non-None
|
// if the caller passed a non-None one (indicates
|
||||||
// one (indicates the text shape changed).
|
// the text shape changed).
|
||||||
if new_entry.embedding.is_some() {
|
if new_entry.embedding.is_some() {
|
||||||
existing.embedding = new_entry.embedding.clone();
|
existing.embedding = new_entry.embedding.clone();
|
||||||
}
|
}
|
||||||
|
// Lifecycle / drift-signal fields — refresh only
|
||||||
|
// when the caller provides a new value. Each was
|
||||||
|
// previously either ignored entirely (valid_until,
|
||||||
|
// doc_refs) or only partially handled. Caught by
|
||||||
|
// the auditor's hybrid fixture on 2026-04-22 when
|
||||||
|
// re-seeding with fresh doc_refs silently dropped
|
||||||
|
// them on the UPDATE path.
|
||||||
if new_entry.schema_fingerprint.is_some() {
|
if new_entry.schema_fingerprint.is_some() {
|
||||||
existing.schema_fingerprint = new_entry.schema_fingerprint.clone();
|
existing.schema_fingerprint = new_entry.schema_fingerprint.clone();
|
||||||
}
|
}
|
||||||
|
if new_entry.valid_until.is_some() {
|
||||||
|
existing.valid_until = new_entry.valid_until.clone();
|
||||||
|
}
|
||||||
|
if !new_entry.doc_refs.is_empty() {
|
||||||
|
// Merge by tool (case-insensitive). A new ref
|
||||||
|
// for the same tool supersedes the old one —
|
||||||
|
// caller is asserting "this is what I used
|
||||||
|
// now." Refs for tools the existing entry
|
||||||
|
// didn't track are appended. This keeps the
|
||||||
|
// cumulative tool history without letting a
|
||||||
|
// single re-seed erase it.
|
||||||
|
let mut merged_refs = existing.doc_refs.clone();
|
||||||
|
for new_ref in &new_entry.doc_refs {
|
||||||
|
let hit = merged_refs.iter().position(|r| {
|
||||||
|
r.tool.eq_ignore_ascii_case(&new_ref.tool)
|
||||||
|
});
|
||||||
|
match hit {
|
||||||
|
Some(idx) => merged_refs[idx] = new_ref.clone(),
|
||||||
|
None => merged_refs.push(new_ref.clone()),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
existing.doc_refs = merged_refs;
|
||||||
|
}
|
||||||
let pid = existing.playbook_id.clone();
|
let pid = existing.playbook_id.clone();
|
||||||
drop(state);
|
drop(state);
|
||||||
self.persist().await?;
|
self.persist().await?;
|
||||||
@ -1600,7 +1640,7 @@ mod upsert_tests {
|
|||||||
let pm = PlaybookMemory::new(Arc::new(InMemory::new()));
|
let pm = PlaybookMemory::new(Arc::new(InMemory::new()));
|
||||||
let e = mk("fill: Welder x2 in Nashville, TN", "2026-04-21", &["Alice Smith"]);
|
let e = mk("fill: Welder x2 in Nashville, TN", "2026-04-21", &["Alice Smith"]);
|
||||||
match pm.upsert_entry(e).await.unwrap() {
|
match pm.upsert_entry(e).await.unwrap() {
|
||||||
UpsertOutcome::Added(_) => {}
|
UpsertOutcome::Added { .. } => {}
|
||||||
other => panic!("expected Added, got {:?}", other),
|
other => panic!("expected Added, got {:?}", other),
|
||||||
}
|
}
|
||||||
assert_eq!(pm.entry_count().await, 1);
|
assert_eq!(pm.entry_count().await, 1);
|
||||||
@ -1613,7 +1653,7 @@ mod upsert_tests {
|
|||||||
let e2 = mk("fill: Welder x2 in Nashville, TN", "2026-04-21", &["Alice Smith", "Bob Jones"]);
|
let e2 = mk("fill: Welder x2 in Nashville, TN", "2026-04-21", &["Alice Smith", "Bob Jones"]);
|
||||||
pm.upsert_entry(e1).await.unwrap();
|
pm.upsert_entry(e1).await.unwrap();
|
||||||
let outcome = pm.upsert_entry(e2).await.unwrap();
|
let outcome = pm.upsert_entry(e2).await.unwrap();
|
||||||
assert!(matches!(outcome, UpsertOutcome::Noop(_)));
|
assert!(matches!(outcome, UpsertOutcome::Noop { .. }));
|
||||||
// Still exactly one entry, no duplicate from the re-seed.
|
// Still exactly one entry, no duplicate from the re-seed.
|
||||||
assert_eq!(pm.entry_count().await, 1);
|
assert_eq!(pm.entry_count().await, 1);
|
||||||
}
|
}
|
||||||
@ -1625,7 +1665,7 @@ mod upsert_tests {
|
|||||||
let e2 = mk("fill: Welder x2 in Nashville, TN", "2026-04-21", &["Alice Smith", "Bob Jones"]);
|
let e2 = mk("fill: Welder x2 in Nashville, TN", "2026-04-21", &["Alice Smith", "Bob Jones"]);
|
||||||
let o1 = pm.upsert_entry(e1).await.unwrap();
|
let o1 = pm.upsert_entry(e1).await.unwrap();
|
||||||
let pid = match o1 {
|
let pid = match o1 {
|
||||||
UpsertOutcome::Added(p) => p,
|
UpsertOutcome::Added { playbook_id: p } => p,
|
||||||
other => panic!("expected Added, got {:?}", other),
|
other => panic!("expected Added, got {:?}", other),
|
||||||
};
|
};
|
||||||
let o2 = pm.upsert_entry(e2).await.unwrap();
|
let o2 = pm.upsert_entry(e2).await.unwrap();
|
||||||
@ -1646,7 +1686,7 @@ mod upsert_tests {
|
|||||||
let e2 = mk("fill: Welder x2 in Nashville, TN", "2026-04-22", &["Alice Smith"]);
|
let e2 = mk("fill: Welder x2 in Nashville, TN", "2026-04-22", &["Alice Smith"]);
|
||||||
pm.upsert_entry(e1).await.unwrap();
|
pm.upsert_entry(e1).await.unwrap();
|
||||||
let o2 = pm.upsert_entry(e2).await.unwrap();
|
let o2 = pm.upsert_entry(e2).await.unwrap();
|
||||||
assert!(matches!(o2, UpsertOutcome::Added(_)), "different day → fresh ADD");
|
assert!(matches!(o2, UpsertOutcome::Added { .. }), "different day → fresh ADD");
|
||||||
assert_eq!(pm.entry_count().await, 2);
|
assert_eq!(pm.entry_count().await, 2);
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -1659,9 +1699,120 @@ mod upsert_tests {
|
|||||||
// A new seed on same day should ADD, not merge into the retired one.
|
// A new seed on same day should ADD, not merge into the retired one.
|
||||||
let e2 = mk("fill: Welder x2 in Nashville, TN", "2026-04-21", &["Carol Davis"]);
|
let e2 = mk("fill: Welder x2 in Nashville, TN", "2026-04-21", &["Carol Davis"]);
|
||||||
let o = pm.upsert_entry(e2).await.unwrap();
|
let o = pm.upsert_entry(e2).await.unwrap();
|
||||||
assert!(matches!(o, UpsertOutcome::Added(_)));
|
assert!(matches!(o, UpsertOutcome::Added { .. }));
|
||||||
assert_eq!(pm.entry_count().await, 2);
|
assert_eq!(pm.entry_count().await, 2);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Regression test for the auditor-discovered bug: UPDATE branch
|
||||||
|
// silently dropped doc_refs, valid_until, and (partially)
|
||||||
|
// schema_fingerprint from the incoming entry.
|
||||||
|
//
|
||||||
|
// Before the fix: a re-seed with different endorsed_names AND new
|
||||||
|
// doc_refs would merge names but discard the new doc_refs. After:
|
||||||
|
// both sets are unioned (doc_refs merged by tool, same-tool
|
||||||
|
// supersedes).
|
||||||
|
|
||||||
|
fn docref(tool: &str, version: &str) -> DocRef {
|
||||||
|
DocRef {
|
||||||
|
tool: tool.into(),
|
||||||
|
version_seen: version.into(),
|
||||||
|
snippet_hash: Some(format!("hash-{tool}-{version}")),
|
||||||
|
source_url: None,
|
||||||
|
seen_at: "2026-04-22T00:00:00Z".into(),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
fn mk_with_docs(op: &str, day: &str, names: &[&str], docs: Vec<DocRef>) -> PlaybookEntry {
|
||||||
|
let mut e = mk(op, day, names);
|
||||||
|
e.doc_refs = docs;
|
||||||
|
e
|
||||||
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn update_merges_doc_refs_with_existing_ones() {
|
||||||
|
let pm = PlaybookMemory::new(Arc::new(InMemory::new()));
|
||||||
|
let day = "2026-04-22";
|
||||||
|
let op = "fill: Welder x2 in Nashville, TN";
|
||||||
|
|
||||||
|
// First seed: Alice + Docker doc ref
|
||||||
|
let e1 = mk_with_docs(op, day, &["Alice"], vec![docref("docker", "24.0.7")]);
|
||||||
|
let o1 = pm.upsert_entry(e1).await.unwrap();
|
||||||
|
assert!(matches!(o1, UpsertOutcome::Added { .. }));
|
||||||
|
|
||||||
|
// Second seed on same (op, day) but new names AND new tool ref
|
||||||
|
let e2 = mk_with_docs(op, day, &["Bob"], vec![docref("terraform", "1.5.0")]);
|
||||||
|
let o2 = pm.upsert_entry(e2).await.unwrap();
|
||||||
|
assert!(matches!(o2, UpsertOutcome::Updated { .. }));
|
||||||
|
assert_eq!(pm.entry_count().await, 1); // still one entry, merged
|
||||||
|
|
||||||
|
let snap = pm.snapshot().await;
|
||||||
|
assert_eq!(snap.len(), 1);
|
||||||
|
let merged = &snap[0];
|
||||||
|
// Names unioned
|
||||||
|
assert!(merged.endorsed_names.contains(&"Alice".to_string()));
|
||||||
|
assert!(merged.endorsed_names.contains(&"Bob".to_string()));
|
||||||
|
// Both doc_refs preserved (different tools → union)
|
||||||
|
assert_eq!(merged.doc_refs.len(), 2);
|
||||||
|
let tools: Vec<_> = merged.doc_refs.iter().map(|r| r.tool.as_str()).collect();
|
||||||
|
assert!(tools.contains(&"docker"));
|
||||||
|
assert!(tools.contains(&"terraform"));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn update_same_tool_supersedes_older_version() {
|
||||||
|
let pm = PlaybookMemory::new(Arc::new(InMemory::new()));
|
||||||
|
let day = "2026-04-22";
|
||||||
|
let op = "fill: Welder x2 in Nashville, TN";
|
||||||
|
|
||||||
|
let e1 = mk_with_docs(op, day, &["Alice"], vec![docref("docker", "24.0.7")]);
|
||||||
|
pm.upsert_entry(e1).await.unwrap();
|
||||||
|
|
||||||
|
// Same day, different name, SAME tool newer version
|
||||||
|
let e2 = mk_with_docs(op, day, &["Bob"], vec![docref("docker", "25.0.1")]);
|
||||||
|
let o2 = pm.upsert_entry(e2).await.unwrap();
|
||||||
|
assert!(matches!(o2, UpsertOutcome::Updated { .. }));
|
||||||
|
|
||||||
|
let snap = pm.snapshot().await;
|
||||||
|
assert_eq!(snap[0].doc_refs.len(), 1, "same tool should supersede, not duplicate");
|
||||||
|
assert_eq!(snap[0].doc_refs[0].version_seen, "25.0.1");
|
||||||
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn update_preserves_existing_doc_refs_when_new_entry_has_none() {
|
||||||
|
let pm = PlaybookMemory::new(Arc::new(InMemory::new()));
|
||||||
|
let day = "2026-04-22";
|
||||||
|
let op = "fill: Welder x2 in Nashville, TN";
|
||||||
|
|
||||||
|
let e1 = mk_with_docs(op, day, &["Alice"], vec![docref("docker", "24.0.7")]);
|
||||||
|
pm.upsert_entry(e1).await.unwrap();
|
||||||
|
|
||||||
|
// Second seed doesn't carry doc_refs — shouldn't wipe the existing
|
||||||
|
let e2 = mk(op, day, &["Bob"]); // no doc_refs
|
||||||
|
pm.upsert_entry(e2).await.unwrap();
|
||||||
|
|
||||||
|
let snap = pm.snapshot().await;
|
||||||
|
assert_eq!(snap[0].doc_refs.len(), 1, "empty new doc_refs shouldn't wipe existing");
|
||||||
|
assert_eq!(snap[0].doc_refs[0].tool, "docker");
|
||||||
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn update_refreshes_valid_until_when_caller_provides_one() {
|
||||||
|
let pm = PlaybookMemory::new(Arc::new(InMemory::new()));
|
||||||
|
let day = "2026-04-22";
|
||||||
|
let op = "fill: Welder x2 in Nashville, TN";
|
||||||
|
|
||||||
|
// First seed with no valid_until
|
||||||
|
let e1 = mk(op, day, &["Alice"]);
|
||||||
|
pm.upsert_entry(e1).await.unwrap();
|
||||||
|
|
||||||
|
// Second seed with a valid_until deadline
|
||||||
|
let mut e2 = mk(op, day, &["Bob"]);
|
||||||
|
e2.valid_until = Some("2026-05-01T00:00:00Z".into());
|
||||||
|
pm.upsert_entry(e2).await.unwrap();
|
||||||
|
|
||||||
|
let snap = pm.snapshot().await;
|
||||||
|
assert_eq!(snap[0].valid_until.as_deref(), Some("2026-05-01T00:00:00Z"));
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user