Raised by an adversarial review of #84 before merge, and verified against that branch. Filed so the findings survive the merge; the first one is cheap to fix while the API is unreleased and breaking to fix afterwards.
1. An explicitly empty dataset scope fails open to every dataset
SearchRequest::datasets is a Vec<String> serialized with skip_serializing_if = "Vec::is_empty" (src/search.rs:52-53), so .with_datasets([]) sends a request with no datasets field — which the runtime treats as "search every searchable dataset".
The omission is deliberate, and the existing test states why (src/search.rs:253-258):
fn test_empty_datasets_omitted() {
// The runtime rejects an empty dataset list with a 400, so an empty
// vector must be omitted rather than sent.That converts a fail-closed runtime rejection into a fail-open client behaviour. A caller that computes a scope — a tenant allow-list, a permission filter — and legitimately arrives at zero datasets gets an unrestricted search across everything the runtime credential can reach, instead of an error or an empty result.
Vec<String> cannot distinguish "never scoped" from "scoped to nothing", which is the root of it. Suggested shape: make the field Option<Vec<String>>, keep None as the explicit search-all default, and have validate() reject Some(vec![]) locally with the same meaning the runtime's 400 carries. Worth a regression test for a computed-empty scope.
2. Defaults on protocol-required response fields hide malformed responses
results and duration_ms (and per-match dataset, _score, matches) deserialize with defaults (src/search.rs:183-188), so a 200 carrying {} or {"error": "..."} succeeds as "zero results in zero milliseconds". Schema drift or a faulty proxy then looks identical to a legitimate no-match search, which hides retrieval failures from callers and from telemetry. Dropping the defaults on fields the protocol always sends — keeping them only for maps the runtime legitimately omits, such as data and primary_key — would make that a deserialization error. Worth negative deserialization tests.
3. cache_control is not forwarded to search requests
SpiceClientBuilder::cache_control reaches the Flight client only; the HTTP path applies auth and JSON headers (src/query.rs:934-937). /v1/search honours Cache-Control, so a client explicitly configured no-cache can still be served a cached result, with the response's cache-status header discarded so callers cannot detect it.
4. with_keywords documents hybrid ranking but the runtime prefilters
The builder and README describe keywords as adding a lexical pass that is fused into a hybrid ranking. The runtime converts supplied keywords into case-insensitive substring predicates ANDed into candidate filtering; full-text generation and score fusion are separate concerns. A caller adding keywords to boost terms would instead silently exclude semantic matches. This one is a docs/naming fix rather than a behaviour change — worth confirming against a running runtime before rewording.
Raised by an adversarial review of #84 before merge, and verified against that branch. Filed so the findings survive the merge; the first one is cheap to fix while the API is unreleased and breaking to fix afterwards.
1. An explicitly empty dataset scope fails open to every dataset
SearchRequest::datasetsis aVec<String>serialized withskip_serializing_if = "Vec::is_empty"(src/search.rs:52-53), so.with_datasets([])sends a request with nodatasetsfield — which the runtime treats as "search every searchable dataset".The omission is deliberate, and the existing test states why (
src/search.rs:253-258):That converts a fail-closed runtime rejection into a fail-open client behaviour. A caller that computes a scope — a tenant allow-list, a permission filter — and legitimately arrives at zero datasets gets an unrestricted search across everything the runtime credential can reach, instead of an error or an empty result.
Vec<String>cannot distinguish "never scoped" from "scoped to nothing", which is the root of it. Suggested shape: make the fieldOption<Vec<String>>, keepNoneas the explicit search-all default, and havevalidate()rejectSome(vec![])locally with the same meaning the runtime's 400 carries. Worth a regression test for a computed-empty scope.2. Defaults on protocol-required response fields hide malformed responses
resultsandduration_ms(and per-matchdataset,_score,matches) deserialize with defaults (src/search.rs:183-188), so a 200 carrying{}or{"error": "..."}succeeds as "zero results in zero milliseconds". Schema drift or a faulty proxy then looks identical to a legitimate no-match search, which hides retrieval failures from callers and from telemetry. Dropping the defaults on fields the protocol always sends — keeping them only for maps the runtime legitimately omits, such asdataandprimary_key— would make that a deserialization error. Worth negative deserialization tests.3.
cache_controlis not forwarded to search requestsSpiceClientBuilder::cache_controlreaches the Flight client only; the HTTP path applies auth and JSON headers (src/query.rs:934-937)./v1/searchhonoursCache-Control, so a client explicitly configuredno-cachecan still be served a cached result, with the response's cache-status header discarded so callers cannot detect it.4.
with_keywordsdocuments hybrid ranking but the runtime prefiltersThe builder and README describe keywords as adding a lexical pass that is fused into a hybrid ranking. The runtime converts supplied keywords into case-insensitive substring predicates ANDed into candidate filtering; full-text generation and score fusion are separate concerns. A caller adding keywords to boost terms would instead silently exclude semantic matches. This one is a docs/naming fix rather than a behaviour change — worth confirming against a running runtime before rewording.