Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions crates/khive-pack-kg/src/handlers/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand Down
5 changes: 5 additions & 0 deletions crates/khive-pack-kg/src/handlers/create.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,10 @@ impl KgPack {
let mut specs: Vec<EntityCreateSpec> = Vec::with_capacity(attempted);
let mut entity_type_normalized: Vec<Value> = 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}"))
Expand Down Expand Up @@ -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(),
Expand Down
16 changes: 15 additions & 1 deletion crates/khive-pack-kg/src/handlers/search.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ impl ValidatedSearchRequest {
/// Parse and validate the canonical KG search wire contract.
pub fn from_value(params: Value, registry: &VerbRegistry) -> Result<Self, RuntimeError> {
let p: SearchParams = deser(params)?;
super::common::require_object_param(p.properties.as_ref(), "properties")?;
let kind_raw = p
.kind
.as_deref()
Expand All @@ -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),
Expand Down
2 changes: 2 additions & 0 deletions crates/khive-pack-kg/src/handlers/update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ impl KgPack {
registry: &VerbRegistry,
) -> Result<Value, RuntimeError> {
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(),
Expand Down Expand Up @@ -300,6 +301,7 @@ impl KgPack {
.prepare_note_update_hook(&self.runtime, token, &note, &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,
Expand Down
114 changes: 114 additions & 0 deletions crates/khive-pack-kg/tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
);
}
}
21 changes: 13 additions & 8 deletions tests/khive-contract/tests/test_coordinator_fanout.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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}"
)

Expand Down
Loading