Skip to content

Commit 5115df8

Browse files
tcconnallytcconnally
authored
fix: mimir_remember/mimir_recall reject explicit JSON null on optional fields (#330) (#334)
Fixes #330. mimir_remember rejected the documented optional topic_path field (and every other optional field with a custom serde default) whenever a caller sent explicit JSON null instead of omitting the key -- serde's #[serde(default = "...")] only fires on an ABSENT key, so a present null still hits the field's real type and fails with 'invalid type: null, expected a string/boolean/f64/...'. Many MCP clients send explicit null for unset optional fields because the tool schema documents them as optional/defaulted, so this was a systemic bug affecting both mimir_remember and mimir_recall, not just topic_path. - Added null_as_default / null_as_named_default! deserialize_with helpers that treat an explicit null the same as an absent key. - Applied to every non-Option field with a #[serde(default...)] in RememberArgs (status, type, tags, importance, topic_path, recall_when, always_on, certainty, workspace_hash, agent_id, visibility) and RecallArgs (limit, offset, min_decay, include_archived, expansion, mode, content_weight, trust_weight, diversity_halving, include_confidence). - The original report's misleading error (topic_path: null reported as 'missing field category') was a side effect of the null failure mode; a genuinely missing category still correctly names category in the error (covered by a new regression test). - 5 new unit tests covering both structs plus the missing-field regression guard. Full suite: 137 passed / 0 failed. - Version bump 2.11.0 -> 2.11.1.
1 parent a6da23d commit 5115df8

3 files changed

Lines changed: 210 additions & 24 deletions

File tree

‎Cargo.lock‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
[package]
22
name = "perseus-vault"
3-
version = "2.11.0"
3+
version = "2.11.1"
44
edition = "2021"
55
description = "Persistent memory engine for AI agents — MCP JSON-RPC stdio server (formerly Mneme/Mimir)"
66
repository = "https://github.com/Perseus-Computing-LLC/perseus-vault"

‎src/tools.rs‎

Lines changed: 208 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -10,33 +10,67 @@ use crate::models::{
1010

1111
// ─── Deserialization structs ────────────────────────────────────
1212

13+
/// #330: many MCP clients send explicit JSON `null` for an optional field
14+
/// they didn't set (rather than omitting the key), because the tool schema
15+
/// lists the field as optional/defaulted. serde's `#[serde(default = "...")]`
16+
/// only fires when the key is *absent*; a present `null` still hits the
17+
/// field's real type and fails with a misleading "invalid type: null,
18+
/// expected a string/boolean/f64/..." error that names the wrong field
19+
/// entirely once combined with `#[serde(deny_unknown_fields)]`-style
20+
/// confusion. This helper treats an explicit `null` the same as an absent
21+
/// key by falling through to `Default::default()` for the field type; pair
22+
/// it with `#[serde(default = "...", deserialize_with = "null_as_default")]`
23+
/// when the field also needs a non-Default::default() default value.
24+
fn null_as_default<'de, D, T>(deserializer: D) -> Result<T, D::Error>
25+
where
26+
D: serde::Deserializer<'de>,
27+
T: Deserialize<'de> + Default,
28+
{
29+
Ok(Option::<T>::deserialize(deserializer)?.unwrap_or_default())
30+
}
31+
1332
#[derive(Debug, Deserialize)]
1433
pub struct RememberArgs {
1534
pub category: String,
1635
pub key: String,
1736
pub body_json: String,
18-
#[serde(default = "default_status")]
37+
#[serde(
38+
default = "default_status",
39+
deserialize_with = "null_as_default_status"
40+
)]
1941
pub status: String,
20-
#[serde(default = "default_entity_type")]
21-
#[serde(rename = "type")]
42+
#[serde(
43+
default = "default_entity_type",
44+
rename = "type",
45+
deserialize_with = "null_as_default_entity_type"
46+
)]
2247
pub entity_type: String,
23-
#[serde(default)]
48+
#[serde(default, deserialize_with = "null_as_default")]
2449
pub tags: Vec<String>,
25-
#[serde(default = "default_importance")]
50+
#[serde(
51+
default = "default_importance",
52+
deserialize_with = "null_as_default_importance"
53+
)]
2654
pub importance: f64,
27-
#[serde(default)]
55+
#[serde(default, deserialize_with = "null_as_default")]
2856
pub topic_path: String,
29-
#[serde(default)]
57+
#[serde(default, deserialize_with = "null_as_default")]
3058
pub recall_when: Vec<String>,
31-
#[serde(default)]
59+
#[serde(default, deserialize_with = "null_as_default")]
3260
pub always_on: bool,
33-
#[serde(default = "default_certainty")]
61+
#[serde(
62+
default = "default_certainty",
63+
deserialize_with = "null_as_default_certainty"
64+
)]
3465
pub certainty: f64,
35-
#[serde(default)]
66+
#[serde(default, deserialize_with = "null_as_default")]
3667
pub workspace_hash: String,
37-
#[serde(default)]
68+
#[serde(default, deserialize_with = "null_as_default")]
3869
pub agent_id: String,
39-
#[serde(default = "default_visibility")]
70+
#[serde(
71+
default = "default_visibility",
72+
deserialize_with = "null_as_default_visibility"
73+
)]
4074
pub visibility: String,
4175
#[serde(default)]
4276
pub layer: Option<String>,
@@ -62,6 +96,26 @@ fn default_importance() -> f64 {
6296
0.5
6397
}
6498

99+
/// #330: same null-tolerance as `null_as_default`, but falls through to a
100+
/// named default function instead of `T::default()` for fields whose
101+
/// "unset" value isn't the type's zero value (e.g. status="active", not "").
102+
macro_rules! null_as_named_default {
103+
($fn_name:ident, $ty:ty, $default_fn:ident) => {
104+
fn $fn_name<'de, D>(deserializer: D) -> Result<$ty, D::Error>
105+
where
106+
D: serde::Deserializer<'de>,
107+
{
108+
Ok(Option::<$ty>::deserialize(deserializer)?.unwrap_or_else($default_fn))
109+
}
110+
};
111+
}
112+
113+
null_as_named_default!(null_as_default_status, String, default_status);
114+
null_as_named_default!(null_as_default_entity_type, String, default_entity_type);
115+
null_as_named_default!(null_as_default_importance, f64, default_importance);
116+
null_as_named_default!(null_as_default_certainty, f64, default_certainty);
117+
null_as_named_default!(null_as_default_visibility, String, default_visibility);
118+
65119
#[derive(Debug, Deserialize)]
66120
pub struct RecallArgs {
67121
pub query: String,
@@ -70,29 +124,38 @@ pub struct RecallArgs {
70124
#[serde(rename = "type")]
71125
#[serde(default)]
72126
pub entity_type: Option<String>,
73-
#[serde(default = "default_limit")]
127+
#[serde(
128+
default = "default_limit",
129+
deserialize_with = "null_as_default_limit"
130+
)]
74131
pub limit: i64,
75-
#[serde(default)]
132+
#[serde(default, deserialize_with = "null_as_default")]
76133
pub offset: i64,
77-
#[serde(default)]
134+
#[serde(default, deserialize_with = "null_as_default")]
78135
pub min_decay: f64,
79136
#[serde(default)]
80137
pub topic_path: Option<String>,
81-
#[serde(default)]
138+
#[serde(default, deserialize_with = "null_as_default")]
82139
pub include_archived: bool,
83-
#[serde(default)]
140+
#[serde(default, deserialize_with = "null_as_default")]
84141
pub expansion: crate::models::QueryExpansionConfig,
85-
#[serde(default)]
142+
#[serde(default, deserialize_with = "null_as_default")]
86143
pub mode: String, // "fts5", "dense", or "hybrid"
87144
#[serde(default)]
88145
pub preview_cap: Option<i64>,
89146
#[serde(default)]
90147
pub always_on: Option<bool>,
91-
#[serde(default)]
148+
#[serde(default, deserialize_with = "null_as_default")]
92149
pub content_weight: f64,
93-
#[serde(default = "crate::models::default_trust_weight")]
150+
#[serde(
151+
default = "crate::models::default_trust_weight",
152+
deserialize_with = "null_as_default_trust_weight"
153+
)]
94154
pub trust_weight: f64,
95-
#[serde(default = "default_halving")]
155+
#[serde(
156+
default = "default_halving",
157+
deserialize_with = "null_as_default_halving"
158+
)]
96159
pub diversity_halving: f64,
97160
/// Recency half-life in seconds for time-aware hybrid ranking (#235).
98161
/// Omit (default) for relevance-only ranking; set to bias toward recent memories.
@@ -107,7 +170,7 @@ pub struct RecallArgs {
107170
/// #287: opt-in. When true, each result gets a normalized `confidence`
108171
/// (0.0–1.0) rolled up from rank, trust, and decay. Default false so
109172
/// existing callers and snapshot tests are unaffected; ranking is unchanged.
110-
#[serde(default)]
173+
#[serde(default, deserialize_with = "null_as_default")]
111174
pub include_confidence: bool,
112175
}
113176

@@ -160,6 +223,18 @@ fn default_limit() -> i64 {
160223
10
161224
}
162225

226+
null_as_named_default!(null_as_default_limit, i64, default_limit);
227+
null_as_named_default!(
228+
null_as_default_trust_weight,
229+
f64,
230+
default_trust_weight_wrapper
231+
);
232+
null_as_named_default!(null_as_default_halving, f64, default_halving);
233+
234+
fn default_trust_weight_wrapper() -> f64 {
235+
crate::models::default_trust_weight()
236+
}
237+
163238
#[derive(Debug, Deserialize)]
164239
pub struct ForgetArgs {
165240
pub category: String,
@@ -1917,3 +1992,114 @@ pub fn handle_purge(db: &Database, args: Value) -> Result<String, String> {
19171992
serde_json::to_string(&report).map_err(|e| format!("Serialization failed: {}", e))
19181993
}
19191994

1995+
#[cfg(test)]
1996+
mod tests {
1997+
use super::*;
1998+
1999+
// #330: mimir_remember rejected the documented optional `topic_path`
2000+
// field (and other optional fields with custom defaults) whenever a
2001+
// caller sent explicit JSON `null` instead of omitting the key. Many
2002+
// MCP clients do this because the tool schema lists the field as
2003+
// optional/defaulted, not because they're being unusual.
2004+
2005+
#[test]
2006+
fn remember_args_accepts_null_topic_path() {
2007+
let v = json!({
2008+
"category": "reference",
2009+
"key": "example-key",
2010+
"body_json": "{}",
2011+
"topic_path": null
2012+
});
2013+
let a: RememberArgs = serde_json::from_value(v).expect("null topic_path must deserialize");
2014+
assert_eq!(a.topic_path, "");
2015+
}
2016+
2017+
#[test]
2018+
fn remember_args_accepts_null_for_every_optional_field_with_custom_default() {
2019+
// Explicit null on each of these must fall back to that field's
2020+
// documented default, not fail deserialization.
2021+
for field in [
2022+
"status",
2023+
"type",
2024+
"tags",
2025+
"importance",
2026+
"topic_path",
2027+
"recall_when",
2028+
"always_on",
2029+
"certainty",
2030+
"workspace_hash",
2031+
"agent_id",
2032+
"visibility",
2033+
] {
2034+
let mut v = json!({
2035+
"category": "reference",
2036+
"key": "example-key",
2037+
"body_json": "{}",
2038+
});
2039+
v.as_object_mut()
2040+
.unwrap()
2041+
.insert(field.to_string(), Value::Null);
2042+
let result: Result<RememberArgs, _> = serde_json::from_value(v);
2043+
assert!(
2044+
result.is_ok(),
2045+
"field `{}` with explicit null should deserialize, got {:?}",
2046+
field,
2047+
result.err()
2048+
);
2049+
}
2050+
}
2051+
2052+
#[test]
2053+
fn remember_args_still_reports_missing_category_correctly() {
2054+
// Regression guard: fixing the null-tolerance bug must not break the
2055+
// genuinely-missing-required-field error path (the original bug
2056+
// report's error message pointed at the wrong field — `category` —
2057+
// when the real offender was `topic_path: null`; once null is
2058+
// handled, a real missing `category` must still be reported as such).
2059+
let v = json!({ "key": "example-key", "body_json": "{}" });
2060+
let result: Result<RememberArgs, _> = serde_json::from_value(v);
2061+
let err = result.expect_err("missing category must fail").to_string();
2062+
assert!(
2063+
err.contains("category"),
2064+
"error should name the actually-missing field `category`, got: {}",
2065+
err
2066+
);
2067+
}
2068+
2069+
#[test]
2070+
fn recall_args_accepts_null_for_every_optional_field_with_custom_default() {
2071+
for field in [
2072+
"limit",
2073+
"offset",
2074+
"min_decay",
2075+
"include_archived",
2076+
"expansion",
2077+
"mode",
2078+
"content_weight",
2079+
"trust_weight",
2080+
"diversity_halving",
2081+
"include_confidence",
2082+
] {
2083+
let mut v = json!({ "query": "test" });
2084+
v.as_object_mut()
2085+
.unwrap()
2086+
.insert(field.to_string(), Value::Null);
2087+
let result: Result<RecallArgs, _> = serde_json::from_value(v);
2088+
assert!(
2089+
result.is_ok(),
2090+
"field `{}` with explicit null should deserialize, got {:?}",
2091+
field,
2092+
result.err()
2093+
);
2094+
}
2095+
}
2096+
2097+
#[test]
2098+
fn recall_args_null_limit_falls_back_to_default_ten() {
2099+
let v = json!({ "query": "test", "limit": null });
2100+
let a: RecallArgs = serde_json::from_value(v).unwrap();
2101+
assert_eq!(a.limit, 10);
2102+
}
2103+
}
2104+
2105+

0 commit comments

Comments
 (0)