Skip to content

test(mcp): drop the field-coverage audit, keep the tool sweep #106

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JosteinGj merged 1 commit into from
Aug 25, 2026
Merged

test(mcp): drop the field-coverage audit, keep the tool sweep #106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 14 additions & 9 deletions AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ backend's `datahub-api/src/main/java/ai/intellistream/datahub/api/mcp/tools/`.

```
cargo test mcp_ # the whole MCP suite (~30s)
cargo test mcp_full_tool_surface # just the field sweep (~8s)
cargo test mcp_full_tool_surface # just the tool sweep (~8s)
```

**These tests are in Rust, not `python_tests/`, on purpose.** The SDK has no MCP client, so the
Expand Down Expand Up @@ -224,15 +224,20 @@ stray it actually left behind:
delete is how the secondary dataset kept surviving: `dataset_delete` does not cascade, it failed
while a resource still pointed at it, and the guard that would have caught that was already off.

### Every advertised field is tested, and that is enforced rather than asserted
### Every tool is driven once; every parameter is not

`McpClient::try_call_tool` records each `(tool, field)` pair it sends, and `mcp_full_tool_surface`
ends by diffing that against the live `tools/list` schema — **125 fields across the 37 tools**. A
parameter added server-side fails the audit until something drives it. That is why the sweep is one
sequential test rather than seventy small ones: `cargo test` runs tests on parallel threads with no
ordering hook, so a registry filled by other tests could not be read reliably at the end of any of
them. Each `sweep_*` helper owns its entities and removes them through the MCP delete tools, which is
also how those get exercised.
`mcp_full_tool_surface` calls all 37 tools and reads each write back. Each `sweep_*` helper owns its
entities and removes them through the MCP delete tools, which is also how those get exercised. It is
one sequential test rather than one per entity group because the helpers share reference data — a
dataset, a label, a relationship type — and a relationship type cannot be deleted once created, so
minting a set per test would grow the tenant's catalogue on every run.

It used to also **audit its own field coverage**: `try_call_tool` recorded every `(tool, field)` pair
sent and the test ended by diffing that against the live `tools/list` schema, so a parameter added
server-side failed the run until something drove it. That was removed. It made an additive api change
fail a test that had nothing to say about whether the new field works, and the number it enforced
(125 fields across 37 tools) moved with the api rather than with the SDK. Consequence worth knowing:
nothing here notices a server-side parameter addition any more — same as on the REST side.

Behaviours worth knowing, each pinned by an assertion:

Expand Down
71 changes: 18 additions & 53 deletions src/mcp_integration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,18 @@
//! conformant client can read. While it is present, every test here fails — which is the honest
//! report, because no tool is reachable.
//!
//! # Every advertised field is tested, and that is enforced rather than asserted
//! # Every tool is driven once
//!
//! [`McpClient::try_call_tool`] records each `(tool, field)` pair it sends, and
//! [`mcp_full_tool_surface`] ends by diffing that against the live `tools/list` schema — 125 fields
//! across the 37 tools today. A parameter added server-side fails the audit until something drives
//! it. That is why the sweep is one sequential test rather than 74 independent ones: `cargo test`
//! runs tests on parallel threads with no ordering hook, so a registry filled by other tests could
//! not be read reliably at the end of any of them.
//! [`mcp_full_tool_surface`] calls all 37, reading each write back. It does **not** check that the
//! arguments it sends exhaust each tool's advertised schema: it used to, by recording every
//! `(tool, field)` pair and diffing that against the live `tools/list`, and that audit was dropped
//! because a field added server-side then failed a test that had nothing to say about whether the
//! new field works — a goalpost that moves with the api rather than a fault in the SDK. Coverage of
//! the *tools* is still the point; coverage of every *parameter* is not.
//!
//! It is one sequential test rather than one per entity group because the `sweep_*` helpers share
//! reference data, and a relationship type cannot be deleted once created (see AGENTS.md) — minting
//! a set per test would grow the tenant's catalogue on every run.
//!
//! # Tests that are red on purpose
//!
Expand All @@ -62,7 +66,7 @@ use crate::{create_api_service, ApiService};
use serde_json::{json, Value};
use std::collections::{BTreeMap, BTreeSet};
use std::sync::atomic::{AtomicI64, Ordering};
use std::sync::{Arc, Mutex};
use std::sync::Arc;

/// The transport requires this exact value; see the module doc.
const ACCEPT: &str = "application/json, text/event-stream";
Expand Down Expand Up @@ -111,8 +115,6 @@ struct McpClient {
api: Arc<ApiService>,
url: String,
next_id: AtomicI64,
/// Every `(tool, field)` pair sent, for [`assert_every_field_was_exercised`].
exercised: Mutex<BTreeMap<String, BTreeSet<String>>>,
}

impl McpClient {
Expand All @@ -123,7 +125,6 @@ impl McpClient {
api,
url,
next_id: AtomicI64::new(0),
exercised: Mutex::new(BTreeMap::new()),
}
}

Expand Down Expand Up @@ -222,8 +223,6 @@ impl McpClient {

/// Invoke a tool, returning the error text instead of panicking — for the negative tests.
async fn try_call_tool(&self, name: &str, arguments: Value) -> Result<Value, String> {
self.record(name, &arguments);

let envelope = self
.rpc("tools/call", Some(json!({"name": name, "arguments": arguments})))
.await;
Expand Down Expand Up @@ -252,15 +251,6 @@ impl McpClient {
self.try_call_tool(name, arguments).await.is_ok()
}

fn record(&self, name: &str, arguments: &Value) {
if let Some(fields) = arguments.as_object() {
let mut exercised = self.exercised.lock().unwrap();
let entry = exercised.entry(name.to_string()).or_default();
for key in fields.keys() {
entry.insert(key.clone());
}
}
}
}

// --------------------------------------------------------------------------- //
Expand Down Expand Up @@ -647,12 +637,13 @@ async fn mcp_enumerating_tools_are_bounded_by_a_limit() {
}

// --------------------------------------------------------------------------- //
// The full field sweep
// The full tool sweep
//
// One sequential test, for the reason given in the module doc: the coverage audit needs the union of
// everything sent, and `cargo test` gives no ordering hook across parallel tests. Each `sweep_*`
// helper owns its entities and deletes them through the MCP delete tools — which is also how those
// tools get exercised.
// Every tool driven once, with each write read back. One sequential test rather than one per entity
// group because the helpers share reference data — a dataset, a label, a relationship type — and a
// relationship type cannot be deleted once created, so minting a set per test would grow the
// tenant's catalogue on every run. Each `sweep_*` helper owns its entities and deletes them through
// the MCP delete tools, which is also how those get exercised.
// --------------------------------------------------------------------------- //

/// A dataset every other entity in the sweep hangs off.
Expand All @@ -664,7 +655,6 @@ struct SweepDataset {
#[tokio::test]
async fn mcp_full_tool_surface() {
let client = McpClient::new();
let tools = client.list_tools().await;

// The label guard is held here, not in `sweep_reference_data`. Dropping it when that helper
// returned deleted the label mid-sweep, and `resource_create` silently re-created it further
Expand All @@ -683,8 +673,6 @@ async fn mcp_full_tool_surface() {
{
dataset_guard.disarm();
}

assert_every_field_was_exercised(&client, &tools);
}

/// `unit_*`, `label_*`, `edge_*_type*`. Returns a label name, a relationship-type name, and the
Expand Down Expand Up @@ -2102,29 +2090,6 @@ async fn sweep_failure_modes(client: &McpClient, dataset: &SweepDataset, label:
guard.disarm();
}

/// Diff what the sweep sent against the live `tools/list` schema.
fn assert_every_field_was_exercised(client: &McpClient, tools: &BTreeMap<String, Value>) {
let exercised = client.exercised.lock().unwrap();
let mut gaps: Vec<String> = Vec::new();

for (name, tool) in tools {
let advertised: BTreeSet<String> = tool["inputSchema"]["properties"]
.as_object()
.map(|properties| properties.keys().cloned().collect())
.unwrap_or_default();
let sent = exercised.get(name).cloned().unwrap_or_default();
let untested: Vec<String> = advertised.difference(&sent).cloned().collect();
if !untested.is_empty() {
gaps.push(format!(" {name}: {}", untested.join(", ")));
}
}

assert!(
gaps.is_empty(),
"advertised tool parameters that the sweep never sent:\n{}",
gaps.join("\n")
);
}

// --------------------------------------------------------------------------- //
// Red on purpose — intended behaviour the api does not yet provide
Expand Down