Skip to content

Commit e16c4af

Browse files
authored
Merge pull request #1488 from DeusData/fix/mcp-payload-duplication
perf(mcp): stop repeating the payload in structuredContent (#1375)
2 parents 399ea7c + f27f0a2 commit e16c4af

3 files changed

Lines changed: 183 additions & 9 deletions

File tree

‎scripts/smoke-test.sh‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -390,6 +390,64 @@ if [ "$TOTAL" -lt 1 ]; then
390390
echo "FAIL: search_graph for 'compute' returned 0 results"
391391
exit 1
392392
fi
393+
394+
echo ""
395+
echo "=== Phase 3z: no tool duplicates its payload into structuredContent (#1375) ==="
396+
# Asserts on the SHIPPED artifact what the unit suite asserts from source: a
397+
# non-JSON payload must travel ONCE. It used to appear twice — content[0].text
398+
# plus an identical structuredContent.text — costing 2.05x the bytes on a large
399+
# query_graph, i.e. half the 10 MiB transport budget and double the tokens billed
400+
# to every LLM caller.
401+
#
402+
# In smoke as well as the unit suite because this is a WIRE-FORMAT property: it
403+
# is what a real client actually receives from the real binary, and a from-source
404+
# test cannot prove the released artifact behaves the same way.
405+
DUP_TOOLS=0
406+
DUP_CHECKED=0
407+
for TOOL_ARGS in "search_graph --project $PROJECT --name-pattern compute" \
408+
"search_code --project $PROJECT --query compute" \
409+
"get_architecture --project $PROJECT" \
410+
"index_status --project $PROJECT"; do
411+
# shellcheck disable=SC2086
412+
ENVELOPE=$("$BINARY" cli $TOOL_ARGS --json 2>/dev/null || true)
413+
[ -z "$ENVELOPE" ] && continue
414+
VERDICT=$(printf '%s' "$ENVELOPE" | python3 -c '
415+
import json,sys
416+
try:
417+
d = json.loads(sys.stdin.read())
418+
except Exception:
419+
print("skip"); raise SystemExit
420+
if d.get("isError"):
421+
print("skip"); raise SystemExit
422+
content = d.get("content") or []
423+
text = content[0].get("text", "") if content else ""
424+
sc = d.get("structuredContent")
425+
if not isinstance(sc, dict):
426+
print("no-structured"); raise SystemExit
427+
try:
428+
payload_is_object = isinstance(json.loads(text), dict)
429+
except Exception:
430+
payload_is_object = False
431+
if payload_is_object:
432+
print("skip"); raise SystemExit
433+
print("dup" if sc.get("text") == text and text else "ok")
434+
')
435+
case "$VERDICT" in
436+
dup) echo "FAIL: $(echo "$TOOL_ARGS" | cut -d" " -f1) repeats its payload in structuredContent (#1375)"; DUP_TOOLS=$((DUP_TOOLS+1)) ;;
437+
ok) DUP_CHECKED=$((DUP_CHECKED+1)) ;;
438+
no-structured) echo "FAIL: $(echo "$TOOL_ARGS" | cut -d" " -f1) has no structuredContent object (outputSchema requires one)"; DUP_TOOLS=$((DUP_TOOLS+1)) ;;
439+
esac
440+
done
441+
if [ "$DUP_TOOLS" -ne 0 ]; then
442+
echo "FAIL: $DUP_TOOLS tool(s) duplicate their payload on the shipped binary"
443+
exit 1
444+
fi
445+
if [ "$DUP_CHECKED" -eq 0 ]; then
446+
echo "FAIL: no tool produced a non-JSON payload — this check proved nothing"
447+
exit 1
448+
fi
449+
echo "OK: $DUP_CHECKED tool(s) deliver their payload exactly once"
450+
393451
echo "OK: search_graph found $TOTAL result(s) for 'compute'"
394452

395453
# 3b: trace_path — verify compute has callers

‎src/mcp/mcp.c‎

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -302,12 +302,34 @@ char *cbm_mcp_text_result(const char *text, bool is_error) {
302302
}
303303
}
304304
if (!has_structured_content) {
305-
/* Every advertised MCP tool has an object outputSchema, so even compact
306-
* TOON/plain-text and error results must carry a conforming structured
307-
* object. Keep the text Content block above for model visibility and
308-
* backwards compatibility. */
305+
/* Every advertised MCP tool declares an object outputSchema, so a
306+
* conforming structuredContent object is mandatory even for compact
307+
* TOON/plain-text and error results. What is NOT mandatory is putting
308+
* the whole payload in it a second time.
309+
*
310+
* It used to. For any result that is not a JSON object — which is every
311+
* TOON answer, i.e. the large ones — structuredContent was
312+
* {"text": <the entire payload>} sitting beside an identical
313+
* content[0].text. Measured at 2.05x the payload on a 20k-node
314+
* query_graph, so half of every reply was redundant bytes: half the
315+
* usable transport budget, and double the tokens billed to every LLM
316+
* caller (#1375).
317+
*
318+
* Nothing is lost by dropping it. structuredContent exists to carry
319+
* STRUCTURE, and a string re-wrapped in a one-key object has none —
320+
* a client parsing structuredContent.text learns exactly what
321+
* content[0].text already told it. The empty object still satisfies
322+
* outputSchema ({"type":"object","additionalProperties":true}), and the
323+
* JSON branch above is untouched: when a tool really does return an
324+
* object, callers still get it parsed.
325+
*
326+
* Errors keep their payload. They are bounded and small, the duplication
327+
* costs nothing measurable, and structuredContent.error is the only
328+
* machine-readable form of a failure a client has. */
309329
yyjson_mut_val *structured = yyjson_mut_obj(doc);
310-
yyjson_mut_obj_add_str(doc, structured, is_error ? "error" : "text", text ? text : "");
330+
if (is_error) {
331+
yyjson_mut_obj_add_str(doc, structured, "error", text ? text : "");
332+
}
311333
yyjson_mut_obj_add_val(doc, root, "structuredContent", structured);
312334
}
313335
yyjson_mut_obj_add_bool(doc, root, "isError", is_error);

‎tests/test_mcp.c‎

Lines changed: 98 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1093,10 +1093,21 @@ TEST(mcp_text_result) {
10931093
PASS();
10941094
}
10951095

1096-
TEST(mcp_text_result_wraps_plain_text_as_structured_content) {
1096+
TEST(mcp_text_result_does_not_duplicate_plain_text_into_structured_content) {
1097+
/* A non-JSON payload used to be repeated verbatim as
1098+
* structuredContent {"text": <payload>} beside content[0].text — 2.05x the
1099+
* payload measured on a 20k-node query_graph, i.e. half the transport budget
1100+
* and double the tokens for every LLM caller (#1375).
1101+
*
1102+
* structuredContent carries STRUCTURE; a string rewrapped in a one-key
1103+
* object has none, so the empty object is the honest answer and still
1104+
* satisfies the permissive outputSchema. The payload stays in content. */
10971105
char *json = cbm_mcp_text_result("plain text", false);
10981106
ASSERT_NOT_NULL(json);
1099-
ASSERT_NOT_NULL(strstr(json, "\"structuredContent\":{\"text\":\"plain text\"}"));
1107+
ASSERT_NOT_NULL(strstr(json, "\"structuredContent\":{}"));
1108+
ASSERT_NULL(strstr(json, "\"structuredContent\":{\"text\""));
1109+
/* The payload is still delivered — exactly once. */
1110+
ASSERT_NOT_NULL(strstr(json, "\"text\":\"plain text\""));
11001111
ASSERT_NOT_NULL(strstr(json, "\"isError\":false"));
11011112
free(json);
11021113
PASS();
@@ -1813,6 +1824,84 @@ TEST(tool_get_code_snippet_clips_whole_file_node) {
18131824
PASS();
18141825
}
18151826

1827+
/* EVERY tool, not just the one that was reported.
1828+
*
1829+
* The duplication was invisible per-tool: each result looked reasonable on its
1830+
* own, and only measuring the wire showed half of it was redundant. A guard
1831+
* pinned to query_graph would not have caught it in search_graph, and would not
1832+
* catch it in whatever tool is added next. So this enumerates the tool table
1833+
* itself — a new tool is covered the moment it is registered, with no test edit.
1834+
*
1835+
* The invariant: for a NON-error result whose payload is not a JSON object,
1836+
* structuredContent must not carry the payload a second time. Errors are exempt
1837+
* and deliberately so — bounded, small, and structuredContent.error is the only
1838+
* machine-readable form of a failure a client gets. */
1839+
TEST(mcp_every_tool_result_is_duplication_free) {
1840+
char tmp[256];
1841+
cbm_mcp_server_t *srv = setup_snippet_server(tmp, sizeof(tmp));
1842+
ASSERT_NOT_NULL(srv);
1843+
1844+
int tools = cbm_mcp_tool_count();
1845+
ASSERT_TRUE(tools > 0); /* an empty table would assert nothing at all */
1846+
int checked = 0;
1847+
1848+
for (int i = 0; i < tools; i++) {
1849+
const char *name = cbm_mcp_tool_name(i);
1850+
ASSERT_NOT_NULL(name);
1851+
/* Minimal args: most tools error out, which is fine — an error envelope
1852+
* is still an envelope, and the property must hold for it too. */
1853+
char *envelope = cbm_mcp_handle_tool(srv, name, "{\"project\":\"test-project\"}");
1854+
if (!envelope) {
1855+
continue;
1856+
}
1857+
yyjson_doc *doc = yyjson_read(envelope, strlen(envelope), 0);
1858+
ASSERT_NOT_NULL(doc);
1859+
yyjson_val *root = yyjson_doc_get_root(doc);
1860+
yyjson_val *content = yyjson_obj_get(root, "content");
1861+
yyjson_val *first = content ? yyjson_arr_get(content, 0) : NULL;
1862+
yyjson_val *text_val = first ? yyjson_obj_get(first, "text") : NULL;
1863+
const char *text = text_val ? yyjson_get_str(text_val) : NULL;
1864+
yyjson_val *structured = yyjson_obj_get(root, "structuredContent");
1865+
1866+
/* outputSchema is declared for every tool, so this stays mandatory. */
1867+
ASSERT_NOT_NULL(structured);
1868+
ASSERT_TRUE(yyjson_is_obj(structured));
1869+
1870+
yyjson_val *is_error = yyjson_obj_get(root, "isError");
1871+
bool errored = is_error && yyjson_is_true(is_error);
1872+
1873+
if (text && text[0] && !errored) {
1874+
/* If the payload is itself a JSON object, structuredContent is the
1875+
* PARSED form and legitimately holds the same data — that is the
1876+
* spec's structured+serialized pattern, not waste. Only the
1877+
* non-object case is checked here. */
1878+
yyjson_doc *as_json = yyjson_read(text, strlen(text), 0);
1879+
bool payload_is_object = as_json && yyjson_is_obj(yyjson_doc_get_root(as_json));
1880+
if (as_json) {
1881+
yyjson_doc_free(as_json);
1882+
}
1883+
if (!payload_is_object) {
1884+
yyjson_val *dup = yyjson_obj_get(structured, "text");
1885+
if (dup && yyjson_is_str(dup)) {
1886+
const char *dup_str = yyjson_get_str(dup);
1887+
/* The exact defect: same bytes, twice, in one reply. */
1888+
ASSERT_TRUE(!(dup_str && strcmp(dup_str, text) == 0));
1889+
}
1890+
checked++;
1891+
}
1892+
}
1893+
yyjson_doc_free(doc);
1894+
free(envelope);
1895+
}
1896+
1897+
/* If no tool produced a non-JSON payload, this test proved nothing — fail
1898+
* rather than report a green that was never exercised. */
1899+
ASSERT_TRUE(checked > 0);
1900+
cbm_mcp_server_free(srv);
1901+
th_rmtree(tmp);
1902+
PASS();
1903+
}
1904+
18161905
TEST(tool_search_graph_includes_node_properties) {
18171906
/* Node properties are OPT-IN columns in the default TOON output: the
18181907
* default row is qn/label/file/lines/degrees only, `fields` adds the
@@ -1830,7 +1919,11 @@ TEST(tool_search_graph_includes_node_properties) {
18301919
"\"arguments\":{\"project\":\"test-project\",\"label\":\"Function\","
18311920
"\"name_pattern\":\"HandleRequest\",\"limit\":5}}}");
18321921
ASSERT_NOT_NULL(resp);
1833-
ASSERT_NOT_NULL(strstr(resp, "\"structuredContent\":{\"text\":"));
1922+
/* TOON is not a JSON object, so structuredContent stays empty rather than
1923+
* repeating the whole table a second time (#1375). The payload travels once,
1924+
* in content. */
1925+
ASSERT_NOT_NULL(strstr(resp, "\"structuredContent\":{}"));
1926+
ASSERT_NULL(strstr(resp, "\"structuredContent\":{\"text\":"));
18341927
char *inner = extract_text_content(resp);
18351928
ASSERT_NOT_NULL(inner);
18361929
ASSERT_NOT_NULL(strstr(inner, "results:")); /* TOON table header */
@@ -10358,7 +10451,8 @@ SUITE(mcp) {
1035810451
RUN_TEST(mcp_ingest_traces_items_disallow_additional_properties_issue731);
1035910452
RUN_TEST(mcp_get_architecture_aspects_schema_enum_pr560);
1036010453
RUN_TEST(mcp_text_result);
10361-
RUN_TEST(mcp_text_result_wraps_plain_text_as_structured_content);
10454+
RUN_TEST(mcp_text_result_does_not_duplicate_plain_text_into_structured_content);
10455+
RUN_TEST(mcp_every_tool_result_is_duplication_free);
1036210456
RUN_TEST(mcp_cancel_matches_request_id);
1036310457
RUN_TEST(mcp_text_result_error);
1036410458

0 commit comments

Comments
 (0)