1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
//! Supersede-path tests for the MCP surface — the `store_memory` tool an agent
//! calls, which is the frame that decides which supersede wrapper a `supersedes`
//! call reaches.
//!
//! Split out of `tests.rs` for issue #271 so both files stay inside the
//! per-file size cap. The gate must keep the `feature = "mcp"` half: the tool
//! handler these tests drive does not exist in the `--no-default-features` lane
//! CI runs. The shared fixtures (`seed_row`, `store_memory_args`,
//! `tool_result_id`) come from `tests.rs` behind `pub(super)`.
#[cfg(all(test, feature = "mcp"))]
mod mcp_supersede {
use crate::config::Config;
use crate::mcp::store_wrapper::StoreWrapper;
use crate::mcp::tests::{seed_row, store_memory_args, tool_result_id};
use crate::mcp::tools::ToolHandler;
use crate::memory::lifecycle::MemoryImportance;
use crate::memory::{MemoryStore, NewMemory};
use std::sync::{Arc, Mutex};
/// Issue #258: `store_memory`'s `supersedes` arm parsed and validated
/// `status` and then called the status-less `StoreWrapper::supersede`, so an
/// agent passing `status: "candidate"` together with `supersedes` got an
/// `active` replacement row with no warning — the MCP twin of the CLI drop.
///
/// Drives the `store_memory` TOOL HANDLER itself (the frame where the status
/// was dropped, and the frame that decides which wrapper the supersedes arm
/// reaches), not the wrapper below it, and reads the stored column back via
/// `StoreWrapper::get`, the read path that applies no status filter.
#[tokio::test]
async fn store_memory_supersedes_honours_the_requested_status() {
use crate::memory::lifecycle::{MemoryStatus, MemoryType};
let (_dir, store) = MemoryStore::test_store();
let store = Arc::new(Mutex::new(store));
let wrapper = StoreWrapper::new(store.clone());
let handler = ToolHandler::new(store, "status-proj".to_string(), Config::default());
let seeded = seed_row(&wrapper, "status-proj", "original content");
let old_id = seeded.id.clone();
// The MCP equivalent of `add --supersedes <id> --status candidate`,
// called the way an agent calls it: through the tool router, so the
// wiring inside `store_memory` is what is under test.
let result = handler
.call_tool_for_test(
"store_memory",
store_memory_args("replacement content", Some("candidate"), Some(&old_id)),
)
.await
.expect("store_memory with supersedes must succeed");
let new_id = tool_result_id(&result);
let replacement = wrapper
.get(&new_id, "status-proj")
.expect("get")
.expect("replacement row stored");
assert_eq!(
replacement.status,
MemoryStatus::Candidate.as_str(),
"the tool handler must hand its validated status to the store, not the wrapper's default"
);
assert_eq!(replacement.content, "replacement content");
// The old row keeps the lifecycle the supersede transaction owns.
let replaced_row = wrapper
.get(&old_id, "status-proj")
.expect("get")
.expect("superseded row still readable");
assert_eq!(replaced_row.status, "superseded");
assert_eq!(replaced_row.superseded_by.as_deref(), Some(new_id.as_str()));
// Identical outcome to the CLI: a candidate replacement is invisible to
// the active-only default list on this surface too. A second, unrelated
// active row is seeded as the control — the superseded original is not
// listed either — so the guard cannot be satisfied by a list call that
// returned nothing.
let control = wrapper
.ingest_with_lifecycle(
&NewMemory {
project_id: "status-proj",
content: "unrelated active control",
metadata: None,
memory_type: MemoryType::Fact,
status: MemoryStatus::Active,
},
true,
MemoryImportance::Medium,
)
.expect("seed control");
let control_id = control["id"].as_str().expect("control id").to_string();
let listed: Vec<serde_json::Value> =
serde_json::from_value(wrapper.list("status-proj", 20, None, None).expect("list"))
.expect("list payload is an array");
let listed_ids: Vec<&str> = listed
.iter()
.map(|m| m["id"].as_str().expect("listed id"))
.collect();
assert!(
listed_ids.contains(&control_id.as_str()),
"the default list itself works, so its silence about the candidate is meaningful: {listed_ids:?}"
);
assert!(
!listed_ids.contains(&new_id.as_str()),
"a candidate replacement must stay out of the default list: {listed_ids:?}"
);
// And the status-less entry point (`supersede_memory`, which has no
// `status` parameter at all) still writes `active` by name — the same
// column value the `status: "active"` case above pins from the tool.
let plain = wrapper
.supersede(
"status-proj",
"plain replacement",
None,
MemoryType::Fact,
&new_id,
)
.expect("level-less supersede");
let plain_id = plain["id"].as_str().expect("plain id").to_string();
assert_eq!(
wrapper
.get(&plain_id, "status-proj")
.expect("get")
.expect("plain row stored")
.status,
MemoryStatus::Active.as_str(),
"the status-less wrapper keeps writing active explicitly"
);
}
/// The `status` an agent names on a `supersedes` call must reach the new
/// row for **both** insertable spellings. `store_memory_supersedes_honours_the
/// _requested_status` pins `candidate`; without an `active` case a handler
/// that hardcoded `active` — the #258 defect — would still pass, since
/// `active` is also the wrapper's own default. Driven through the router, so
/// the frame under test is the one that chooses the wrapper.
#[tokio::test]
async fn store_memory_supersedes_honours_an_explicit_active_status() {
use crate::memory::lifecycle::{MemoryStatus, MemoryType};
let (_dir, store) = MemoryStore::test_store();
let store = Arc::new(Mutex::new(store));
let wrapper = StoreWrapper::new(store.clone());
let handler = ToolHandler::new(store, "active-proj".to_string(), Config::default());
let old = seed_row(&wrapper, "active-proj", "original content");
let result = handler
.call_tool_for_test(
"store_memory",
store_memory_args("replacement content", Some("active"), Some(&old.id)),
)
.await
.expect("store_memory with supersedes must succeed");
let new_id = tool_result_id(&result);
let replacement = wrapper
.get(&new_id, "active-proj")
.expect("get")
.expect("replacement row stored");
assert_eq!(replacement.status, MemoryStatus::Active.as_str());
assert_eq!(replacement.content, "replacement content");
assert_eq!(replacement.memory_type, MemoryType::Fact.as_str());
let replaced_row = wrapper
.get(&old.id, "active-proj")
.expect("get")
.expect("superseded row still readable");
assert_eq!(replaced_row.status, "superseded");
assert_eq!(replaced_row.superseded_by.as_deref(), Some(new_id.as_str()));
}
/// Issue #305: `supersede_memory` serialised an absent/`null` metadata
/// argument to the 4-character string `"null"` and handed it to the store as
/// a `&str`, so the replacement row's metadata column held that literal while
/// `store_memory` stored SQL NULL for the same input. Every read path renders
/// both spellings as JSON `null` (`parse_metadata`, and the `trim() != "null"`
/// guards in the wrapper's search/list), so the difference is invisible in
/// tool output — this test reads the `Option<String>` **column** back through
/// `StoreWrapper::get`.
///
/// Drives the `supersede_memory` TOOL (nothing else does) through the router
/// for every shape its handler can receive — including the non-object scalars
/// and strings, which a "simplify" to an object-only match would silently null
/// out — and pins parity with a `store_memory` row whose `metadata` key was
/// likewise omitted.
#[tokio::test]
async fn supersede_memory_stores_no_metadata_for_omitted_or_null_metadata() {
use serde_json::json;
let (_dir, store) = MemoryStore::test_store();
let store = Arc::new(Mutex::new(store));
let wrapper = StoreWrapper::new(store.clone());
let handler = ToolHandler::new(store.clone(), "meta-proj".to_string(), Config::default());
// Parity control: a `store_memory` row with the metadata key omitted.
// Its column must equal every no-metadata row the tool loop stores below,
// so one read of it replaces a separate equality assertion at the end.
let stored = handler
.call_tool_for_test("store_memory", json!({ "text": "stored control" }))
.await
.expect("store_memory must succeed");
let stored_row = wrapper
.get(&tool_result_id(&stored), "meta-proj")
.expect("get")
.expect("stored row");
assert_eq!(
stored_row.metadata, None,
"the parity control must itself store no metadata"
);
// (arguments sent to the tool, metadata column expected on the new row).
let cases = [
("omitted", json!({}), None),
("explicit null", json!({ "metadata": null }), None),
(
"json-null string",
json!({ "metadata": "null" }),
Some("\"null\""),
),
("a number", json!({ "metadata": 0 }), Some("0")),
("a boolean", json!({ "metadata": false }), Some("false")),
("an empty string", json!({ "metadata": "" }), Some(r#""""#)),
(
"a real document",
json!({ "metadata": { "topic": "auth" } }),
Some(r#"{"topic":"auth"}"#),
),
];
for (label, extra, expected) in cases {
let old = seed_row(&wrapper, "meta-proj", "original content");
let mut args = json!({
"new_text": "replacement content",
"old_memory_id": old.id,
});
for (key, value) in extra.as_object().expect("extra args are an object") {
args[key] = value.clone();
}
let result = handler
.call_tool_for_test("supersede_memory", args)
.await
.unwrap_or_else(|err| panic!("{label}: supersede_memory must succeed: {err:?}"));
let new_id = tool_result_id(&result);
let row = wrapper
.get(&new_id, "meta-proj")
.expect("get")
.unwrap_or_else(|| panic!("{label}: replacement row stored"));
assert_eq!(
row.metadata.as_deref(),
expected,
"{label}: the metadata column must hold what the handler was given"
);
assert_eq!(row.content, "replacement content");
assert_eq!(row.status, "active");
// The retirement half of the same transaction: a handler that
// inserted the replacement without marking the old row would still
// pass every metadata assertion above.
let retired = wrapper
.get(&old.id, "meta-proj")
.expect("get")
.unwrap_or_else(|| panic!("{label}: superseded row still readable"));
assert_eq!(
retired.status, "superseded",
"{label}: the supersede call must retire the old row"
);
assert_eq!(
retired.superseded_by.as_deref(),
Some(new_id.as_str()),
"{label}: the old row must link to the row that replaced it"
);
}
}
/// Issue #271: the re-supersede guard has to answer an agent in the tool's
/// own error shape, not as a bare database message. Superseding a row the
/// supersede transaction already retired would fork the chain, so the tool
/// refuses and writes nothing.
#[tokio::test]
async fn store_memory_refuses_to_supersede_a_superseded_row() {
let (_dir, store) = MemoryStore::test_store();
let store = Arc::new(Mutex::new(store));
let wrapper = StoreWrapper::new(store.clone());
let handler = ToolHandler::new(store, "chain-proj".to_string(), Config::default());
let old = seed_row(&wrapper, "chain-proj", "original content");
let first = handler
.call_tool_for_test(
"store_memory",
store_memory_args("first replacement", None, Some(&old.id)),
)
.await
.expect("first supersede must succeed");
let first_id = tool_result_id(&first);
let err = handler
.call_tool_for_test(
"store_memory",
store_memory_args("second replacement", None, Some(&old.id)),
)
.await
.expect_err("re-superseding a superseded row must be refused");
assert!(
err.message.contains("superseded") && err.message.contains(&first_id),
"the refusal must name the current status and its superseded_by: {err:?}"
);
// Nothing written: the chain still points at the first replacement and
// the project holds exactly those two rows.
let replaced = wrapper
.get(&old.id, "chain-proj")
.expect("get")
.expect("superseded row still readable");
assert_eq!(replaced.superseded_by.as_deref(), Some(first_id.as_str()));
let listed: Vec<serde_json::Value> =
serde_json::from_value(wrapper.list("chain-proj", 20, None, None).expect("list"))
.expect("list payload is an array");
assert_eq!(
listed.len(),
1,
"only the live replacement is listed: {listed:?}"
);
let all: Vec<serde_json::Value> = serde_json::from_value(
wrapper
.list(
"chain-proj",
20,
None,
Some(["active", "candidate", "superseded", "deprecated"].as_slice()),
)
.expect("list"),
)
.expect("list payload is an array");
assert_eq!(all.len(), 2, "the refused call inserted nothing: {all:?}");
}
}