diff --git a/crates/khive-pack-kg/src/handlers/common.rs b/crates/khive-pack-kg/src/handlers/common.rs index d6ecdb510..b4d78bbb5 100644 --- a/crates/khive-pack-kg/src/handlers/common.rs +++ b/crates/khive-pack-kg/src/handlers/common.rs @@ -1047,6 +1047,70 @@ pub(crate) fn render_query_result(result: QueryResult) -> Value { Value::Object(out) } +/// Name a JSON value's type the way a caller's schema names it. +fn json_type_name(value: &Value) -> &'static str { + match value { + Value::Null => "null", + Value::Bool(_) => "boolean", + Value::Number(_) => "number", + Value::String(_) => "string", + Value::Array(_) => "array", + Value::Object(_) => "object", + } +} + +/// Refuse a value for a parameter this pack declares as an object. +/// +/// `param_type` is a promise to the caller and nothing was checking it. It is +/// rendered into the JSON schema handed to a model and then never compared +/// against the argument that arrives, so `properties: "not-an-object"` was +/// accepted and persisted, and every later reader found a string where the +/// schema said map. For an agent that is worse than a refusal: a success +/// return gives it nothing to correct on, so it proceeds believing the write +/// landed in the shape it intended. +/// +/// Absent and explicit null are not type errors. Null is how a caller clears +/// the field on the update path, and absent means unchanged. +pub(crate) fn require_object_param(value: Option<&Value>, param: &str) -> Result<(), RuntimeError> { + match value { + None | Some(Value::Null) | Some(Value::Object(_)) => Ok(()), + Some(other) => Err(RuntimeError::InvalidInput(format!( + "{param} must be an object; got {}", + json_type_name(other) + ))), + } +} + +#[cfg(test)] +mod param_contract_tests { + use super::*; + + #[test] + fn an_object_parameter_refuses_every_non_object_and_names_what_it_got() { + // The shape that was accepted and persisted. + let err = require_object_param(Some(&json!("not-an-object")), "properties") + .expect_err("a string is not an object"); + let message = err.to_string(); + assert!(message.contains("properties"), "{message}"); + assert!(message.contains("string"), "names what arrived: {message}"); + + // An array is the near miss a caller is most likely to send next, so it + // must be refused by type rather than by a map-specific probe. + assert!(require_object_param(Some(&json!([1, 2])), "properties").is_err()); + assert!(require_object_param(Some(&json!(7)), "properties").is_err()); + assert!(require_object_param(Some(&json!(true)), "properties").is_err()); + } + + #[test] + fn absent_and_explicit_null_are_not_type_errors() { + // Absent means unchanged and null is how the update path clears a field; + // an implementation that refuses anything that is not an object breaks both. + require_object_param(None, "properties").expect("absent is allowed"); + require_object_param(Some(&Value::Null), "properties").expect("null is allowed"); + require_object_param(Some(&json!({"a": 1})), "properties").expect("an object is allowed"); + } +} + #[cfg(test)] mod note_projection_tests { use super::*; diff --git a/crates/khive-pack-kg/src/handlers/create.rs b/crates/khive-pack-kg/src/handlers/create.rs index 1d757e780..b490982e0 100644 --- a/crates/khive-pack-kg/src/handlers/create.rs +++ b/crates/khive-pack-kg/src/handlers/create.rs @@ -155,6 +155,10 @@ impl KgPack { let mut specs: Vec = Vec::with_capacity(attempted); let mut entity_type_normalized: Vec = Vec::new(); for (idx, entry) in entries.into_iter().enumerate() { + super::common::require_object_param( + entry.properties.as_ref(), + &format!("items[{idx}].properties"), + )?; // Resolve the item's own kind. let item_kind_spec = resolve_kind_spec(&entry.kind, registry).map_err(|e| { RuntimeError::InvalidInput(format!("items[{idx}].kind: {e}")) @@ -378,6 +382,7 @@ impl KgPack { } let p: CreateParams = deser(params.clone())?; + super::common::require_object_param(p.properties.as_ref(), "properties")?; if p.kind != "note" && (p.key.is_some() || p.embed.is_some() || p.fence.is_some()) { return Err(RuntimeError::InvalidInput( "key, embed and fence apply only to notes".into(), diff --git a/crates/khive-pack-kg/src/handlers/search.rs b/crates/khive-pack-kg/src/handlers/search.rs index 44c4d0652..1c3a7e1f4 100644 --- a/crates/khive-pack-kg/src/handlers/search.rs +++ b/crates/khive-pack-kg/src/handlers/search.rs @@ -59,6 +59,7 @@ impl ValidatedSearchRequest { /// Parse and validate the canonical KG search wire contract. pub fn from_value(params: Value, registry: &VerbRegistry) -> Result { let p: SearchParams = deser(params)?; + super::common::require_object_param(p.properties.as_ref(), "properties")?; let kind_raw = p .kind .as_deref() @@ -74,7 +75,20 @@ impl ValidatedSearchRequest { }; let tags = p.tags.unwrap_or_default(); let limit = p.limit.unwrap_or(10).min(100); - let min_score = p.min_score.unwrap_or(0.0).max(0.0); + // The declared range is 0.0 to 1.0 and neither end was enforced. A floor + // above 1.0 was honoured and returned an empty result, which a caller cannot + // tell from no such record; a negative floor was silently clamped to 0.0, so + // the value the caller passed was not the value that ran. Refuse both and + // name the range, the way the other input refusals on this surface do. + let min_score = match p.min_score { + None => 0.0, + Some(value) if value.is_finite() && (0.0..=1.0).contains(&value) => value, + Some(value) => { + return Err(RuntimeError::InvalidInput(format!( + "min_score must be between 0.0 and 1.0; got {value}" + ))) + } + }; let source = match p.source.as_deref() { None => None, Some("text") => Some(SearchSource::Text), diff --git a/crates/khive-pack-kg/src/handlers/update.rs b/crates/khive-pack-kg/src/handlers/update.rs index b2f3a08b7..12ac2ebd0 100644 --- a/crates/khive-pack-kg/src/handlers/update.rs +++ b/crates/khive-pack-kg/src/handlers/update.rs @@ -182,6 +182,7 @@ impl KgPack { registry: &VerbRegistry, ) -> Result { let p: UpdateParams = deser(params.clone())?; + super::common::require_object_param(p.properties.as_ref(), "properties")?; if p.entity_kind.is_some() { return Err(RuntimeError::InvalidInput( "entity_kind is immutable; to change kind, delete then re-create the entity, or use merge() if this is a deduplication correction".into(), @@ -300,6 +301,7 @@ impl KgPack { .prepare_note_update_hook(&self.runtime, token, ¬e, &mut params) .await?; let p: UpdateParams = deser(params)?; + super::common::require_object_param(p.properties.as_ref(), "properties")?; let patch = NotePatch::new( optional_string_patch(p.name, "name")?, p.content, diff --git a/crates/khive-pack-kg/tests/integration.rs b/crates/khive-pack-kg/tests/integration.rs index 4e811ccee..8624b8581 100644 --- a/crates/khive-pack-kg/tests/integration.rs +++ b/crates/khive-pack-kg/tests/integration.rs @@ -14933,3 +14933,117 @@ async fn update_empty_string_property_survives_agent_echo_and_readback() { "Agent readback must retain the same empty-string value: {agent_readback}" ); } + +/// A parameter this pack declares as an object must refuse a scalar rather than +/// store it. The declared type is rendered into the schema a caller is handed and +/// was never compared against the argument that arrived, so a string persisted and +/// every later reader found a string where the schema promised a map. The success +/// return is the harm: an agent has nothing to correct on. +#[tokio::test] +async fn create_refuses_a_scalar_where_properties_declares_an_object() { + let pack = pack(); + + let error = pack + .dispatch( + "create", + json!({ + "kind": "entity", + "entity_kind": "concept", + "name": "ObjectParamScalar", + "properties": "not-an-object" + }), + ) + .await + .expect_err("a string properties must be refused, not stored"); + assert!( + is_invalid_input(&error), + "must be an input refusal, got {error:?}" + ); + let message = error.to_string(); + assert!( + message.contains("properties") && message.contains("object"), + "the refusal must name the parameter and the expected shape: {message}" + ); + + // Control in the same test: the identical call with a real object succeeds, so + // the refusal is about the shape and not about the field being present at all. + pack.dispatch( + "create", + json!({ + "kind": "entity", + "entity_kind": "concept", + "name": "ObjectParamControl", + "properties": {"domain": "inference"} + }), + ) + .await + .expect("an object properties must still be accepted"); + + // A batch names the offending item rather than the batch, since one error is + // returned for N records. + let error = pack + .dispatch( + "create", + json!({"items": [ + {"kind": "entity", "entity_kind": "concept", "name": "BatchOk", + "properties": {"domain": "inference"}}, + {"kind": "entity", "entity_kind": "concept", "name": "BatchBad", + "properties": "not-an-object"} + ]}), + ) + .await + .expect_err("a scalar properties inside a batch must be refused"); + assert!( + error.to_string().contains("items[1]"), + "the refusal must name which item: {error}" + ); +} + +/// `min_score` is documented as a 0.0-1.0 floor and neither end was enforced: a +/// floor above the range was honoured and returned an empty result a caller +/// cannot tell from "no such record", and a negative floor was silently clamped, +/// so the value passed was not the value that ran. +#[tokio::test] +async fn search_refuses_a_score_floor_outside_the_declared_range() { + let pack = pack(); + for name in ["ScoreFloorOne", "ScoreFloorTwo"] { + pack.dispatch( + "create", + json!({"kind": "entity", "entity_kind": "concept", "name": name}), + ) + .await + .unwrap(); + } + + // Load-bearing control: the rows ARE findable at a sane floor. Without this the + // empty result at an out-of-range floor could be an empty corpus. + let hits = pack + .dispatch( + "search", + json!({"kind": "entity", "query": "ScoreFloor", "min_score": 0.0, "limit": 10}), + ) + .await + .expect("a floor inside the range must be accepted"); + assert!( + !hits.as_array().expect("array").is_empty(), + "control: the rows must be findable at a sane floor" + ); + + for floor in [json!(7), json!(-3), json!(1.5)] { + let error = pack + .dispatch( + "search", + json!({"kind": "entity", "query": "ScoreFloor", "min_score": floor, "limit": 10}), + ) + .await + .expect_err("a floor outside 0.0-1.0 must be refused, not honoured"); + assert!( + is_invalid_input(&error), + "must be an input refusal, got {error:?}" + ); + assert!( + error.to_string().contains("min_score"), + "the refusal must name the parameter: {error}" + ); + } +} diff --git a/tests/khive-contract/tests/test_coordinator_fanout.py b/tests/khive-contract/tests/test_coordinator_fanout.py index 1c9078766..3ef2f2769 100644 --- a/tests/khive-contract/tests/test_coordinator_fanout.py +++ b/tests/khive-contract/tests/test_coordinator_fanout.py @@ -321,16 +321,21 @@ def test_search_min_score_filters_all_below_threshold( khive_session: KhiveMcpSession, temp_namespace: str, ) -> None: - """search(kind="entity", min_score=2.0) returns empty results for any real entity. + """search(kind="entity", min_score=1.0) returns empty results for any real entity. Source: crates/kkernel/src/coordinator/tests.rs t7c_multi_backend_search_min_score_applied - RRF scores for any real hit are always <= 1/(60+1) ~= 0.016. A min_score - of 2.0 is above any achievable RRF score. If the coordinator or handler - ignores min_score, the seeded entity would be returned and this test fails. - An empty result proves min_score is applied (search.rs line 138, - score_floor = p.min_score.unwrap_or(0.0).max(0.0)). + RRF scores for any real hit are always <= 1/(60+1) ~= 0.016, so a floor of + 1.0 is above any achievable score. If the coordinator or handler ignores + min_score, the seeded entity would be returned and this test fails; an empty + result proves the floor is applied. + + This port previously used 2.0, which is outside the documented 0.0-1.0 range + and was only accepted because the range was unenforced. Its own Rust source + uses 1.0. The intent, a floor above every achievable score, is expressible + inside the contract, so the out-of-range value bought nothing and hid the + fact that the range was a promise with nothing behind it. """ ns = temp_namespace @@ -355,13 +360,13 @@ def test_search_min_score_filters_all_below_threshold( hits = khive_session.verb("search", { "kind": "entity", "query": "cft7c_minscore_probe", - "min_score": 2.0, + "min_score": 1.0, "namespace": ns, }) assert isinstance(hits, list), f"search must return a list; got {type(hits)}" assert hits == [], ( - "min_score=2.0 must exclude all results (no real RRF score can reach 2.0); " + "min_score=1.0 must exclude all results (no real RRF score can reach 1.0); " f"got {len(hits)} hit(s): {hits}" )