fix(mcp): make every tool reply client-usable — structuredContent, pipelining, config get (#1522)

Three regressions shipped in 0.10.0 share one failure shape: an empty result
with a success status, indistinguishable from "nothing found" for the LLM
clients that are cbm's primary consumers.

1) structuredContent {} on the whole tree-format surface (#1522 bug 1).
   #1488 replaced the duplicated payload with an EMPTY structuredContent
   object while every tool still declared a blanket permissive outputSchema.
   Spec-honoring clients (Claude Code among them) treat structuredContent as
   THE result when a schema is declared, so search_graph, trace_path,
   query_graph, get_architecture, search_code, and detect_changes all rendered
   as literally "{}" on their DEFAULT format, on every platform. The corrected
   contract: no tool declares an outputSchema (tool output is
   format-parameter-polymorphic — no static schema is truthful), JSON-object
   payloads keep their parsed structuredContent, errors keep
   structuredContent.error, and text-shaped payloads carry NO structuredContent
   key at all — which also preserves #1375's no-duplication win.

2) Frontend queue overflow killed the session (found by the #1522 sweep).
   Any 7+ requests pipelined in one stdin burst — an agent issuing parallel
   tool calls does exactly this — overflowed the 8-frame frontend queue, which
   failed the whole session: rc=1 with ZERO bytes of output, every buffered
   response lost. A full queue is now backpressure: the stdin reader blocks
   until the worker drains (bounded by the same stop/fail flags every teardown
   path already sets); only a single frame larger than the entire 12 MiB byte
   budget — which could never be admitted — remains a hard failure.

3) config get printed "" with exit 0 for every unset and every unknown key
   (#1522 bug 2). list printed stored-or-DEFAULT while get printed
   stored-or-EMPTY, and no subcommand validated key names, so a typo was
   indistinguishable from a correctly-read setting. One config-key table now
   drives help, list, get, set, and reset: get prints the stored value or the
   key's real default (the same fallback the runtime readers use), and unknown
   keys error with exit 1 on get, set, and reset alike.

Tests — each RED on the pre-fix tree and RED again on revert:
  * test_mcp.c: text results carry no structuredContent key; tools/list
    declares no outputSchema; the tool-table guard now binds all three
    branches (absent / parsed-object / error) for every registered tool.
  * test_daemon_frontend.c: the over-capacity contract flips from
    "session fails" to "backpressure without loss" — the held first request
    plus all 32 over-capacity frames are answered and the run closes cleanly.
  * test_cli.c: the config command contract — defaults, round-trip, reset,
    and unknown-key rejection on all three subcommands.
  * smoke-test.sh: Phase 3z rewritten to the corrected structuredContent
    contract, new Phase 3z1 (default-format replies usable in schema-honoring
    clients, no outputSchema advertised), 3z2 (24 pipelined calls all
    answered), 3z3 (config defaults + unknown-key rejection) — all asserted
    against the SHIPPED artifact, where #1488's smoke phase previously pinned
    the empty-object behavior as correct.

Verified end-to-end on the locally built production binary: all 15 tools
declare no schema; text tools return ABSENT structuredContent, object tools
populated, error envelopes intact; 7/24/64-deep pipelined bursts all answered
with rc=0; config get prints real defaults and exits 1 on unknown keys.
hook_augment (structuredContent.projects) and index_resilience
(structuredContent.status) consume object payloads and are unaffected.

Fixes #1522.

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
This commit is contained in:
Martin Vogel committed 2026-08-11 01:12:59 +02:00
1 parent 61b3b1b2ed
commit f59d24fd10
7 files changed
+449 -121

No files matched your search

+117 -7
View File
@@ -392,7 +392,7 @@ if [ "$TOTAL" -lt 1 ]; then
fi
echo ""
echo "=== Phase 3z: no tool duplicates its payload into structuredContent (#1375) ==="
echo "=== Phase 3z: structuredContent carries structure or is absent — never {} and never a copy (#1375, #1522) ==="
# Asserts on the SHIPPED artifact what the unit suite asserts from source: a
# non-JSON payload must travel ONCE. It used to appear twice — content[0].text
# plus an identical structuredContent.text — costing 2.05x the bytes on a large
@@ -421,21 +421,32 @@ if d.get("isError"):
print("skip"); raise SystemExit
content = d.get("content") or []
text = content[0].get("text", "") if content else ""
sc = d.get("structuredContent")
if not isinstance(sc, dict):
print("no-structured"); raise SystemExit
sc = d.get("structuredContent", "ABSENT")
try:
payload_is_object = isinstance(json.loads(text), dict)
except Exception:
payload_is_object = False
if payload_is_object:
print("skip"); raise SystemExit
print("dup" if sc.get("text") == text and text else "ok")
# Object payloads must carry the parsed, NON-EMPTY object.
print("ok" if isinstance(sc, dict) and sc else "object-lost"); raise SystemExit
# Text payloads: no structuredContent key at all. {} rendered as the whole
# result in outputSchema-honoring clients (#1522); {"text": ...} duplicated
# the wire (#1375).
if sc == "ABSENT":
print("ok")
elif isinstance(sc, dict) and not sc:
print("empty-lie")
elif isinstance(sc, dict) and sc.get("text") == text and text:
print("dup")
else:
print("unexpected")
')
case "$VERDICT" in
dup) echo "FAIL: $(echo "$TOOL_ARGS" | cut -d" " -f1) repeats its payload in structuredContent (#1375)"; DUP_TOOLS=$((DUP_TOOLS+1)) ;;
empty-lie) echo "FAIL: $(echo "$TOOL_ARGS" | cut -d" " -f1) ships structuredContent {} beside a non-empty payload (#1522)"; DUP_TOOLS=$((DUP_TOOLS+1)) ;;
object-lost) echo "FAIL: $(echo "$TOOL_ARGS" | cut -d" " -f1) dropped the parsed object from structuredContent"; DUP_TOOLS=$((DUP_TOOLS+1)) ;;
unexpected) echo "FAIL: $(echo "$TOOL_ARGS" | cut -d" " -f1) has an unexpected structuredContent shape"; DUP_TOOLS=$((DUP_TOOLS+1)) ;;
ok) DUP_CHECKED=$((DUP_CHECKED+1)) ;;
no-structured) echo "FAIL: $(echo "$TOOL_ARGS" | cut -d" " -f1) has no structuredContent object (outputSchema requires one)"; DUP_TOOLS=$((DUP_TOOLS+1)) ;;
esac
done
if [ "$DUP_TOOLS" -ne 0 ]; then
@@ -448,6 +459,105 @@ if [ "$DUP_CHECKED" -eq 0 ]; then
fi
echo "OK: $DUP_CHECKED tool(s) deliver their payload exactly once"
echo "=== Phase 3z1: default-format MCP replies are usable in schema-honoring clients (#1522) ==="
# The exact end-to-end failure shape of #1522: a spec-compliant MCP client
# (Claude Code) reads structuredContent as THE result when a tool declares an
# outputSchema. Drive the REAL stdio server the way such a client does and
# assert a default-format search_graph reply cannot render as "{}": either
# structuredContent is absent (client falls back to content text) or it is a
# non-empty object. Also assert no tool declares an outputSchema anymore.
MCP_1522=$(python3 - "$BINARY" "$PROJECT" <<'PYMCP'
import json, subprocess, sys
BIN, PROJECT = sys.argv[1], sys.argv[2]
def rpc(i, m, p): return json.dumps({"jsonrpc": "2.0", "id": i, "method": m, "params": p})
reqs = "\n".join([
rpc(1, "initialize", {"protocolVersion": "2025-06-18", "capabilities": {},
"clientInfo": {"name": "smoke-1522", "version": "0"}}),
json.dumps({"jsonrpc": "2.0", "method": "notifications/initialized"}),
rpc(2, "tools/list", {}),
rpc(3, "tools/call", {"name": "search_graph", "arguments":
{"project": PROJECT, "name_pattern": "compute"}}),
]) + "\n"
out = subprocess.run([BIN], input=reqs, capture_output=True, text=True, timeout=180).stdout
verdict = []
for line in out.splitlines():
try:
d = json.loads(line)
except Exception:
continue
if d.get("id") == 2:
schemas = [t["name"] for t in d.get("result", {}).get("tools", []) if "outputSchema" in t]
if schemas:
verdict.append("FAIL: outputSchema declared by: " + ",".join(schemas))
if d.get("id") == 3:
r = d.get("result", {})
text = (r.get("content") or [{}])[0].get("text", "")
sc = r.get("structuredContent", "ABSENT")
if not text:
verdict.append("FAIL: search_graph content text is empty")
if isinstance(sc, dict) and not sc:
verdict.append("FAIL: search_graph ships structuredContent {} (#1522)")
if not verdict:
verdict.append("OK")
print("; ".join(verdict))
PYMCP
)
case "$MCP_1522" in
OK) echo "OK: default-format MCP reply is client-usable, no outputSchema declared" ;;
*) echo "$MCP_1522"; exit 1 ;;
esac
echo "=== Phase 3z2: pipelined requests beyond queue capacity all get answers ==="
# Any 7+ requests written in one stdin burst used to kill the server with
# rc=1 and ZERO bytes of output — the frontend queue (capacity 8 frames,
# initialize + notification included) failed the whole session at overflow
# instead of backpressuring the reader. Agent clients issuing parallel tool
# calls pipeline exactly like this.
PIPELINE=$(python3 - "$BINARY" <<'PYPIPE'
import json, subprocess, sys
BIN = sys.argv[1]
N = 24
def rpc(i, m, p): return json.dumps({"jsonrpc": "2.0", "id": i, "method": m, "params": p})
lines = [rpc(1, "initialize", {"protocolVersion": "2025-06-18", "capabilities": {},
"clientInfo": {"name": "smoke-pipe", "version": "0"}}),
json.dumps({"jsonrpc": "2.0", "method": "notifications/initialized"})]
for i in range(N):
lines.append(rpc(100 + i, "tools/call", {"name": "list_projects", "arguments": {}}))
r = subprocess.run([BIN], input="\n".join(lines) + "\n",
capture_output=True, text=True, timeout=300)
answered = set()
for line in r.stdout.splitlines():
try:
d = json.loads(line)
except Exception:
continue
if isinstance(d.get("id"), int) and d["id"] >= 100:
answered.add(d["id"])
if r.returncode == 0 and len(answered) == N:
print("OK")
else:
print(f"FAIL: rc={r.returncode} answered={len(answered)}/{N} stdout_bytes={len(r.stdout)}")
PYPIPE
)
case "$PIPELINE" in
OK) echo "OK: 24 pipelined tool calls all answered, clean exit" ;;
*) echo "$PIPELINE"; exit 1 ;;
esac
echo "=== Phase 3z3: config get prints real defaults and rejects unknown keys (#1522) ==="
CFG_HOME=$(smoke_mktemp_dir)
CFG_WATCH=$(CBM_CACHE_DIR="$CFG_HOME" "$BINARY" config get auto_watch 2>/dev/null || true)
if [ "$CFG_WATCH" != "true" ] && [ "$CFG_WATCH" != "false" ]; then
echo "FAIL: config get auto_watch printed '$CFG_WATCH' (expected the stored value or the default 'true')"
rm -rf "$CFG_HOME"; exit 1
fi
if CBM_CACHE_DIR="$CFG_HOME" "$BINARY" config get totally_bogus_key >/dev/null 2>&1; then
echo "FAIL: config get of an unknown key exited 0"
rm -rf "$CFG_HOME"; exit 1
fi
rm -rf "$CFG_HOME"
echo "OK: config get returns defaults and errors on unknown keys"
echo "OK: search_graph found $TOTAL result(s) for 'compute'"
# 3b: trace_path — verify compute has callers
+61 -17
View File
@@ -6510,6 +6510,43 @@ int cbm_config_delete(cbm_config_t *cfg, const char *key) {
/* ── Config CLI subcommand ────────────────────────────────────── */
/* THE config-key table. list, get, help, and key validation all read this one
* definition, so a key cannot exist with different defaults in different
* subcommands again: `config list` used to print stored-or-DEFAULT while
* `config get` printed stored-or-EMPTY — every unset key (and every typo'd
* key, at exit 0) "got" an empty string, indistinguishable from a real value
* (#1522). The defaults here mirror the runtime readers' fallbacks
* (cbm_config_get_bool/int call sites); keep them in sync. */
typedef struct {
const char *key;
const char *default_value;
const char *help;
} config_key_def_t;
static const config_key_def_t CONFIG_KEYS[] = {
{CBM_CONFIG_AUTO_INDEX, "false", "Enable auto-indexing on MCP session start"},
{CBM_CONFIG_AUTO_INDEX_LIMIT, "50000", "Max files for auto-indexing new projects"},
{CBM_CONFIG_AUTO_WATCH, "true", "Register background git watcher on session connect"},
{CBM_CONFIG_UI_LANG, "auto", "Pin graph UI language: en, zh, or auto"},
};
static const config_key_def_t *config_key_lookup(const char *key) {
for (size_t i = 0; key && i < sizeof(CONFIG_KEYS) / sizeof(CONFIG_KEYS[0]); i++) {
if (strcmp(CONFIG_KEYS[i].key, key) == 0) {
return &CONFIG_KEYS[i];
}
}
return NULL;
}
static void config_print_unknown_key(const char *key) {
(void)fprintf(stderr, "error: unknown config key: %s\nKnown keys:", key ? key : "");
for (size_t i = 0; i < sizeof(CONFIG_KEYS) / sizeof(CONFIG_KEYS[0]); i++) {
(void)fprintf(stderr, " %s", CONFIG_KEYS[i].key);
}
(void)fprintf(stderr, "\n");
}
int cbm_cmd_config(int argc, char **argv) {
/* NULL argv with a nonzero argc previously slipped past this guard (the
* inner `argv &&` shielded only the help comparison) and dereferenced
@@ -6522,14 +6559,10 @@ int cbm_cmd_config(int argc, char **argv) {
printf(" set <key> <val> Set a config value\n");
printf(" reset <key> Reset a key to default\n\n");
printf("Config keys:\n");
printf(" %-25s default=%-10s %s\n", CBM_CONFIG_AUTO_INDEX, "false",
"Enable auto-indexing on MCP session start");
printf(" %-25s default=%-10s %s\n", CBM_CONFIG_AUTO_INDEX_LIMIT, "50000",
"Max files for auto-indexing new projects");
printf(" %-25s default=%-10s %s\n", CBM_CONFIG_AUTO_WATCH, "true",
"Register background git watcher on session connect");
printf(" %-25s default=%-10s %s\n", CBM_CONFIG_UI_LANG, "auto",
"Pin graph UI language: en, zh, or auto");
for (size_t i = 0; i < sizeof(CONFIG_KEYS) / sizeof(CONFIG_KEYS[0]); i++) {
printf(" %-25s default=%-10s %s\n", CONFIG_KEYS[i].key, CONFIG_KEYS[i].default_value,
CONFIG_KEYS[i].help);
}
return 0;
}
@@ -6551,25 +6584,33 @@ int cbm_cmd_config(int argc, char **argv) {
int rc = 0;
if (strcmp(argv[0], "list") == 0 || strcmp(argv[0], "ls") == 0) {
printf("Configuration:\n");
printf(" %-25s = %-10s\n", CBM_CONFIG_AUTO_INDEX,
cbm_config_get(cfg, CBM_CONFIG_AUTO_INDEX, "false"));
printf(" %-25s = %-10s\n", CBM_CONFIG_AUTO_INDEX_LIMIT,
cbm_config_get(cfg, CBM_CONFIG_AUTO_INDEX_LIMIT, "50000"));
printf(" %-25s = %-10s\n", CBM_CONFIG_AUTO_WATCH,
cbm_config_get(cfg, CBM_CONFIG_AUTO_WATCH, "true"));
printf(" %-25s = %-10s\n", CBM_CONFIG_UI_LANG,
cbm_config_get(cfg, CBM_CONFIG_UI_LANG, "auto"));
for (size_t i = 0; i < sizeof(CONFIG_KEYS) / sizeof(CONFIG_KEYS[0]); i++) {
printf(" %-25s = %-10s\n", CONFIG_KEYS[i].key,
cbm_config_get(cfg, CONFIG_KEYS[i].key, CONFIG_KEYS[i].default_value));
}
} else if (strcmp(argv[0], "get") == 0) {
if (argc < MIN_ARGC_GET) {
(void)fprintf(stderr, "Usage: config get <key>\n");
rc = CLI_TRUE;
} else {
printf("%s\n", cbm_config_get(cfg, argv[CLI_SKIP_ONE], ""));
const config_key_def_t *def = config_key_lookup(argv[CLI_SKIP_ONE]);
if (!def) {
config_print_unknown_key(argv[CLI_SKIP_ONE]);
rc = CLI_TRUE;
} else {
/* Stored value or the key's real default — the same fallback
* the runtime readers use. `""` here was #1522's bug 2: every
* unset key read as empty with exit 0. */
printf("%s\n", cbm_config_get(cfg, def->key, def->default_value));
}
}
} else if (strcmp(argv[0], "set") == 0) {
if (argc < MIN_ARGC_CMD) {
(void)fprintf(stderr, "Usage: config set <key> <value>\n");
rc = CLI_TRUE;
} else if (!config_key_lookup(argv[CLI_SKIP_ONE])) {
config_print_unknown_key(argv[CLI_SKIP_ONE]);
rc = CLI_TRUE;
} else {
if (cbm_config_set(cfg, argv[CLI_SKIP_ONE], argv[CLI_PAIR_LEN]) == 0) {
printf("%s = %s\n", argv[CLI_SKIP_ONE], argv[CLI_PAIR_LEN]);
@@ -6582,6 +6623,9 @@ int cbm_cmd_config(int argc, char **argv) {
if (argc < MIN_ARGC_GET) {
(void)fprintf(stderr, "Usage: config reset <key>\n");
rc = CLI_TRUE;
} else if (!config_key_lookup(argv[CLI_SKIP_ONE])) {
config_print_unknown_key(argv[CLI_SKIP_ONE]);
rc = CLI_TRUE;
} else {
cbm_config_delete(cfg, argv[CLI_SKIP_ONE]);
printf("%s reset to default\n", argv[CLI_SKIP_ONE]);
+47 -24
View File
@@ -492,33 +492,56 @@ static bool frontend_enqueue(frontend_state_t *state, char *message, bool conten
return false;
}
}
cbm_mutex_lock(&state->mutex);
bool stopped = state->stopping || state->failed;
bool capacity = state->count < FRONTEND_QUEUE_CAPACITY &&
state->queued_bytes <= FRONTEND_QUEUE_BYTES_MAX &&
length <= FRONTEND_QUEUE_BYTES_MAX - state->queued_bytes;
if (!stopped && capacity) {
size_t tail = (state->head + state->count) % FRONTEND_QUEUE_CAPACITY;
state->queue[tail] = (frontend_item_t){
.message = message,
.length = length,
.content_length_framed = content_length_framed,
.has_id = has_id,
.id = id,
.id_str = id_str,
};
state->count++;
state->queued_bytes += length;
/* A frame that exceeds the whole byte budget can never be admitted no
* matter how long we wait; only that case is a hard failure. */
if (length > FRONTEND_QUEUE_BYTES_MAX) {
cbm_mutex_lock(&state->mutex);
if (!state->stopping && !state->failed) {
state->failed = true;
state->stopping = true;
}
cbm_mutex_unlock(&state->mutex);
return true;
free(id_str);
return false;
}
if (!stopped) {
state->failed = true;
state->stopping = true;
/* A full queue is BACKPRESSURE, not failure. This runs on the stdin reader
* thread, so blocking here stops further reads and lets the kernel pipe
* absorb the client's burst — a pipelining client is a client going fast,
* not a client going wrong. The old code failed the whole session at frame
* capacity+1, killing it with every buffered response unwritten: any 7+
* pipelined requests (e.g. an agent issuing parallel tool calls) died with
* rc=1 and zero output (#1522 follow-up). Waits are bounded only by the
* stop/fail flags, which every teardown path already sets — the same
* condition the polling worker and watchdogs key on. */
for (;;) {
cbm_mutex_lock(&state->mutex);
bool stopped = state->stopping || state->failed;
if (stopped) {
cbm_mutex_unlock(&state->mutex);
free(id_str);
return false;
}
bool capacity = state->count < FRONTEND_QUEUE_CAPACITY &&
state->queued_bytes <= FRONTEND_QUEUE_BYTES_MAX &&
length <= FRONTEND_QUEUE_BYTES_MAX - state->queued_bytes;
if (capacity) {
size_t tail = (state->head + state->count) % FRONTEND_QUEUE_CAPACITY;
state->queue[tail] = (frontend_item_t){
.message = message,
.length = length,
.content_length_framed = content_length_framed,
.has_id = has_id,
.id = id,
.id_str = id_str,
};
state->count++;
state->queued_bytes += length;
cbm_mutex_unlock(&state->mutex);
return true;
}
cbm_mutex_unlock(&state->mutex);
cbm_usleep(FRONTEND_WAIT_US);
}
cbm_mutex_unlock(&state->mutex);
free(id_str);
return false;
}
static bool frontend_stop_begin(frontend_state_t *state) {
+29 -28
View File
@@ -301,35 +301,32 @@ char *cbm_mcp_text_result(const char *text, bool is_error) {
yyjson_doc_free(structured_doc);
}
}
if (!has_structured_content) {
/* Every advertised MCP tool declares an object outputSchema, so a
* conforming structuredContent object is mandatory even for compact
* TOON/plain-text and error results. What is NOT mandatory is putting
* the whole payload in it a second time.
if (!has_structured_content && is_error) {
/* structuredContent has now been wrong in both directions, so the rule
* is spelled out here in full:
*
* It used to. For any result that is not a JSON object — which is every
* TOON answer, i.e. the large ones — structuredContent was
* {"text": <the entire payload>} sitting beside an identical
* content[0].text. Measured at 2.05x the payload on a 20k-node
* query_graph, so half of every reply was redundant bytes: half the
* usable transport budget, and double the tokens billed to every LLM
* caller (#1375).
* - JSON-object payload -> structuredContent = the PARSED object
* (the branch above; the spec's structured+serialized pattern).
* - error -> structuredContent = {"error": <text>} —
* bounded, small, and the only machine-readable failure form.
* - anything else -> NO structuredContent key at all.
*
* Nothing is lost by dropping it. structuredContent exists to carry
* STRUCTURE, and a string re-wrapped in a one-key object has none —
* a client parsing structuredContent.text learns exactly what
* content[0].text already told it. The empty object still satisfies
* outputSchema ({"type":"object","additionalProperties":true}), and the
* JSON branch above is untouched: when a tool really does return an
* object, callers still get it parsed.
* Pre-#1488 the "anything else" case duplicated the payload verbatim
* ({"text": <payload>} beside an identical content[0].text — 2.05x the
* bytes on a 20k-node query_graph, #1375). #1488 replaced that with an
* EMPTY object on the theory that it "still satisfies outputSchema" —
* but clients that honor a declared outputSchema treat structuredContent
* as THE authoritative result, so every tree-format reply rendered as
* literally "{}" in Claude Code and friends (#1522). An empty object
* beside a non-empty payload is not conservative; it is a wrong answer.
*
* Errors keep their payload. They are bounded and small, the duplication
* costs nothing measurable, and structuredContent.error is the only
* machine-readable form of a failure a client has. */
* The key is therefore OMITTED for text-shaped payloads, and no tool
* declares an outputSchema anymore (see mcp_add_tool_def): output is
* format-parameter-polymorphic, so a static schema was never truthful.
* tests/test_mcp.c binds all three branches; scripts/smoke-test.sh
* asserts the same contract on the shipped binary. */
yyjson_mut_val *structured = yyjson_mut_obj(doc);
if (is_error) {
yyjson_mut_obj_add_str(doc, structured, "error", text ? text : "");
}
yyjson_mut_obj_add_str(doc, structured, "error", text ? text : "");
yyjson_mut_obj_add_val(doc, root, "structuredContent", structured);
}
yyjson_mut_obj_add_bool(doc, root, "isError", is_error);
@@ -693,8 +690,6 @@ static const tool_def_t TOOLS[] = {
static const int TOOL_COUNT = sizeof(TOOLS) / sizeof(TOOLS[0]);
static const char MCP_TOOL_OUTPUT_SCHEMA[] = "{\"type\":\"object\",\"additionalProperties\":true}";
typedef struct {
const char *name;
bool read_only;
@@ -752,7 +747,13 @@ static void mcp_add_tool_def(yyjson_mut_doc *doc, yyjson_mut_val *tools, int i)
yyjson_mut_obj_add_str(doc, tool, "description", TOOLS[i].description);
mcp_add_json_schema(doc, tool, "inputSchema", TOOLS[i].input_schema);
mcp_add_json_schema(doc, tool, "outputSchema", MCP_TOOL_OUTPUT_SCHEMA);
/* Deliberately NO outputSchema. Tool output is format-parameter-polymorphic
* (tree text by default, a JSON object under format:"json"), so no static
* schema is truthful — and a declared schema makes spec-honoring clients
* read structuredContent as the authoritative result, which is exactly how
* the empty-object regression rendered every tree reply as "{}" (#1522).
* The blanket {"type":"object","additionalProperties":true} it replaced
* validated anything and informed nobody. */
const tool_annotation_def_t *def = mcp_tool_annotations(TOOLS[i].name);
yyjson_mut_val *annotations = yyjson_mut_obj(doc);
+93
View File
@@ -11460,6 +11460,98 @@ TEST(cli_config_get_set) {
PASS();
}
/* Capture stdout across one cbm_cmd_config invocation. Returns the command's
* rc and copies captured stdout (NUL-terminated, truncated to cap) out.
* tmpfile+dup2+rewind is the file's established portable capture idiom —
* pread/mkstemp/setenv do not exist on the MinGW leg. */
static int cli_config_cmd_capture(int argc, char **argv, char *out, size_t cap) {
out[0] = '\0';
FILE *capture = tmpfile();
int saved = capture ? dup(fileno(stdout)) : -1;
if (!capture || saved < 0) {
if (capture)
fclose(capture);
if (saved >= 0)
close(saved);
return -1000;
}
fflush(stdout);
if (dup2(fileno(capture), fileno(stdout)) < 0) {
fclose(capture);
close(saved);
return -1000;
}
int rc = cbm_cmd_config(argc, argv);
fflush(stdout);
(void)dup2(saved, fileno(stdout));
close(saved);
rewind(capture);
size_t got = fread(out, 1, cap - 1, capture);
out[got] = '\0';
fclose(capture);
return rc;
}
/* The user-facing config contract (#1522 bug 2): `get` of an UNSET key prints
* that key's real default — the same fallback the runtime readers use — with
* exit 0; a stored value round-trips; an unknown key is an ERROR (non-zero,
* nothing on stdout) for get, set, and reset alike. The old code printed ""
* with exit 0 for every unset key AND every typo, so a misspelled key was
* indistinguishable from a correctly-read setting. */
TEST(cli_config_cmd_get_prints_defaults_and_rejects_unknown_keys) {
char tmpdir[512];
snprintf(tmpdir, sizeof(tmpdir), "%s/cli-cfg-cmd-XXXXXX", cbm_tmpdir());
if (!cbm_mkdtemp(tmpdir))
FAIL("cbm_mkdtemp failed");
char *saved_cache = NULL;
const char *prior = getenv("CBM_CACHE_DIR");
if (prior) {
saved_cache = strdup(prior);
}
cbm_setenv("CBM_CACHE_DIR", tmpdir, 1);
char out[512];
/* Unset key -> its real default, exit 0. */
char *get_watch[] = {"get", "auto_watch"};
ASSERT_EQ(cli_config_cmd_capture(2, get_watch, out, sizeof(out)), 0);
ASSERT_STR_EQ(out, "true\n");
char *get_limit[] = {"get", "auto_index_limit"};
ASSERT_EQ(cli_config_cmd_capture(2, get_limit, out, sizeof(out)), 0);
ASSERT_STR_EQ(out, "50000\n");
/* Stored value round-trips. */
char *set_watch[] = {"set", "auto_watch", "false"};
ASSERT_EQ(cli_config_cmd_capture(3, set_watch, out, sizeof(out)), 0);
ASSERT_EQ(cli_config_cmd_capture(2, get_watch, out, sizeof(out)), 0);
ASSERT_STR_EQ(out, "false\n");
/* Reset returns the key to its default. */
char *reset_watch[] = {"reset", "auto_watch"};
ASSERT_EQ(cli_config_cmd_capture(2, reset_watch, out, sizeof(out)), 0);
ASSERT_EQ(cli_config_cmd_capture(2, get_watch, out, sizeof(out)), 0);
ASSERT_STR_EQ(out, "true\n");
/* Unknown keys are errors on every subcommand, with clean stdout. */
char *get_bogus[] = {"get", "totally_bogus_key"};
ASSERT_TRUE(cli_config_cmd_capture(2, get_bogus, out, sizeof(out)) != 0);
ASSERT_STR_EQ(out, "");
char *set_bogus[] = {"set", "totally_bogus_key", "x"};
ASSERT_TRUE(cli_config_cmd_capture(3, set_bogus, out, sizeof(out)) != 0);
ASSERT_STR_EQ(out, "");
char *reset_bogus[] = {"reset", "totally_bogus_key"};
ASSERT_TRUE(cli_config_cmd_capture(2, reset_bogus, out, sizeof(out)) != 0);
ASSERT_STR_EQ(out, "");
if (saved_cache) {
cbm_setenv("CBM_CACHE_DIR", saved_cache, 1);
free(saved_cache);
} else {
cbm_unsetenv("CBM_CACHE_DIR");
}
test_rmdir_r(tmpdir);
PASS();
}
typedef struct {
cbm_config_t *config;
const char *key;
@@ -12346,6 +12438,7 @@ SUITE(cli) {
/* Config store (7 tests — group F) */
RUN_TEST(cli_config_open_close);
RUN_TEST(cli_config_get_set);
RUN_TEST(cli_config_cmd_get_prints_defaults_and_rejects_unknown_keys);
RUN_TEST(cli_config_get_result_storage_is_per_thread);
RUN_TEST(cli_config_get_bool);
RUN_TEST(cli_config_get_int);
+45 -7
View File
@@ -89,6 +89,12 @@ typedef struct {
atomic_int second_session_cancels;
atomic_bool block_first_request;
atomic_bool first_request_started;
/* Overflow mode: set by the writer AFTER stdin is fully pumped and closed,
* releasing the deliberately-held first request. The hold is what forces
* the input queue to actually fill (proving enqueue backpressures instead
* of failing the session); the release is a state transition, not a
* timeout, so the drain that follows is deterministic. */
atomic_bool release_first_request;
int request_observed_fd;
int session_cancel_fd;
} frontend_eof_application_context_t;
@@ -162,10 +168,14 @@ static cbm_daemon_runtime_application_status_t frontend_eof_application_request(
if (request_index == 0 &&
atomic_load_explicit(&context->block_first_request, memory_order_acquire)) {
atomic_store_explicit(&context->first_request_started, true, memory_order_release);
while (!atomic_load_explicit(&session->cancelled, memory_order_acquire)) {
while (!atomic_load_explicit(&session->cancelled, memory_order_acquire) &&
!atomic_load_explicit(&context->release_first_request, memory_order_acquire)) {
cbm_usleep(1000);
}
return CBM_DAEMON_RUNTIME_APPLICATION_CANCELLED;
if (atomic_load_explicit(&session->cancelled, memory_order_acquire)) {
return CBM_DAEMON_RUNTIME_APPLICATION_CANCELLED;
}
/* Released: fall through and answer normally like every later item. */
}
if (request_length == 0) {
return CBM_DAEMON_RUNTIME_APPLICATION_OK;
@@ -271,6 +281,14 @@ static void *frontend_eof_writer(void *opaque) {
}
ok = close(writer->fd) == 0 && ok;
writer->fd = -1;
if (writer->overflow) {
/* Everything — 32 over-capacity frames AND the EOF behind them — is now
* committed to the pipe. Only now release the held first request: the
* queue was provably full while it blocked, so the drain that follows
* exercises enqueue-wait, not a conveniently fast worker. */
atomic_store_explicit(&writer->application->release_first_request, true,
memory_order_release);
}
atomic_store_explicit(&writer->succeeded, ok, memory_order_release);
atomic_store_explicit(&writer->finished, true, memory_order_release);
return NULL;
@@ -285,6 +303,7 @@ static bool frontend_eof_fixture_start(frontend_eof_fixture_t *fixture, const ch
atomic_init(&fixture->application.second_session_cancels, 0);
atomic_init(&fixture->application.block_first_request, true);
atomic_init(&fixture->application.first_request_started, false);
atomic_init(&fixture->application.release_first_request, false);
fixture->application.request_observed_fd = -1;
fixture->application.session_cancel_fd = -1;
char key[CBM_DAEMON_KEY_SIZE];
@@ -408,13 +427,31 @@ static int frontend_eof_child_run(const char *parent, bool overflow) {
atomic_load_explicit(&fixture.application.first_request_started, memory_order_acquire);
bool session_cancelled =
atomic_load_explicit(&fixture.application.session_cancels, memory_order_acquire) > 0;
int requests_observed =
atomic_load_explicit(&fixture.application.requests, memory_order_acquire);
bool input_closed = fclose(input) == 0;
bool output_closed = fclose(output) == 0;
bool streams_closed = input_closed && output_closed;
bool fixture_closed = frontend_eof_fixture_finish(&fixture);
bool result_matches_contract = overflow ? result < 0 : result == 0;
return result_matches_contract && joined && writer_ok && request_started && session_cancelled &&
streams_closed && fixture_closed
bool result_matches_contract;
if (overflow) {
/* Over-capacity pipelined input is a client going FAST, not a client
* going WRONG (#1522 follow-up): enqueue must backpressure the stdin
* reader until the worker drains, then every queued frame — the held
* first request plus all 32 over-capacity frames — is answered and the
* session closes cleanly. The old contract pinned the defect: enqueue
* overflow failed the whole session (result < 0, session cancelled,
* every buffered response lost). */
/* session_cancel also fires on the ordinary client disconnect at EOF
* teardown, so it cannot discriminate this contract; the loss-free
* property is the request count plus the clean run result. */
result_matches_contract =
result == 0 && requests_observed == 1 + FRONTEND_EOF_TEST_OVERFLOW_MESSAGES;
} else {
result_matches_contract = result == 0 && session_cancelled;
}
return result_matches_contract && joined && writer_ok && request_started && streams_closed &&
fixture_closed
? 0
: 73;
}
@@ -583,6 +620,7 @@ static int frontend_backpressure_daemon_run(const char *parent, int ready_fd, in
atomic_init(&application.second_session_cancels, 0);
atomic_init(&application.block_first_request, false);
atomic_init(&application.first_request_started, false);
atomic_init(&application.release_first_request, false);
application.request_observed_fd = ready_fd;
application.session_cancel_fd = cancel_fd;
cbm_daemon_runtime_application_callbacks_t callbacks = {
@@ -1332,7 +1370,7 @@ TEST(daemon_local_participant_monitor_allows_supervisor_containment_window) {
* prevents the sole reader from observing that already-pending EOF, so neither
* the request nor its daemon session is cancelled. Overload must instead fail
* the frontend promptly and close/cancel the authenticated session. */
TEST(daemon_frontend_over_capacity_input_cannot_hide_eof_behind_active_request) {
TEST(daemon_frontend_over_capacity_input_backpressures_without_loss) {
ASSERT_TRUE(frontend_eof_run_isolated("overflow", true));
PASS();
}
@@ -1390,7 +1428,7 @@ SUITE(daemon_frontend) {
RUN_TEST(daemon_frontend_maintenance_exits_while_stdio_reader_is_blocked);
RUN_TEST(daemon_local_participant_monitor_cancels_then_bounds_active_operation);
RUN_TEST(daemon_local_participant_monitor_allows_supervisor_containment_window);
RUN_TEST(daemon_frontend_over_capacity_input_cannot_hide_eof_behind_active_request);
RUN_TEST(daemon_frontend_over_capacity_input_backpressures_without_loss);
RUN_TEST(daemon_frontend_eof_drain_timeout_cancels_and_returns_success);
RUN_TEST(daemon_frontend_stdout_backpressure_eof_fail_stops_and_cancels_session);
RUN_TEST(daemon_frontend_stdout_backpressure_maintenance_stops_and_cancels_session);
+57 -38
View File
@@ -825,8 +825,13 @@ TEST(mcp_tools_list_latest_metadata) {
ASSERT_NOT_NULL(strstr(json, "\"title\":\"Search graph\""));
ASSERT_NOT_NULL(strstr(json, "\"title\":\"Index repository\""));
ASSERT_NOT_NULL(strstr(json, "\"title\":\"Check index coverage\""));
ASSERT_NOT_NULL(strstr(json, "\"outputSchema\":{\"type\":\"object\""));
ASSERT_NOT_NULL(strstr(json, "\"additionalProperties\":true"));
/* No tool may declare an outputSchema. The blanket permissive schema
* ({"type":"object","additionalProperties":true}) carried zero information
* for clients, but its presence made spec-compliant clients read
* structuredContent as the authoritative result — which turned every
* text-shaped (tree/TOON) reply into a rendered "{}" (#1522). Tool output
* here is format-parameter-polymorphic, so no static schema is truthful. */
ASSERT_NULL(strstr(json, "\"outputSchema\""));
/* search_graph's compact degree columns intentionally count the graph
* relationships used for call/reference/type centrality, not every edge
* family (for example DEFINES or CONTAINS_FILE). Keep the public contract
@@ -1093,19 +1098,24 @@ TEST(mcp_text_result) {
PASS();
}
TEST(mcp_text_result_does_not_duplicate_plain_text_into_structured_content) {
/* A non-JSON payload used to be repeated verbatim as
* structuredContent {"text": <payload>} beside content[0].text — 2.05x the
* payload measured on a 20k-node query_graph, i.e. half the transport budget
* and double the tokens for every LLM caller (#1375).
TEST(mcp_text_result_omits_structured_content_for_plain_text) {
/* A non-JSON payload must not produce a structuredContent key AT ALL.
*
* structuredContent carries STRUCTURE; a string rewrapped in a one-key
* object has none, so the empty object is the honest answer and still
* satisfies the permissive outputSchema. The payload stays in content. */
* History, because this field has now been wrong in both directions:
* pre-#1488 it duplicated the whole payload ({"text": <payload>} beside an
* identical content[0].text — 2.05x the bytes). #1488 replaced that with an
* EMPTY object — and spec-compliant clients (Claude Code among them) treat
* structuredContent as THE result whenever the tool declares an
* outputSchema, so every default-format search_graph/trace_path rendered as
* literally "{}" (#1522). Empty is not honest; it is a second lie.
*
* The corrected contract: no duplication AND no empty-object placeholder.
* A text payload travels once, in content[0].text, and the envelope simply
* has no structuredContent. (Real JSON objects and error envelopes keep
* theirs — that is structure, not padding.) */
char *json = cbm_mcp_text_result("plain text", false);
ASSERT_NOT_NULL(json);
ASSERT_NOT_NULL(strstr(json, "\"structuredContent\":{}"));
ASSERT_NULL(strstr(json, "\"structuredContent\":{\"text\""));
ASSERT_NULL(strstr(json, "\"structuredContent\""));
/* The payload is still delivered — exactly once. */
ASSERT_NOT_NULL(strstr(json, "\"text\":\"plain text\""));
ASSERT_NOT_NULL(strstr(json, "\"isError\":false"));
@@ -1832,10 +1842,13 @@ TEST(tool_get_code_snippet_clips_whole_file_node) {
* catch it in whatever tool is added next. So this enumerates the tool table
* itself — a new tool is covered the moment it is registered, with no test edit.
*
* The invariant: for a NON-error result whose payload is not a JSON object,
* structuredContent must not carry the payload a second time. Errors are exempt
* and deliberately so — bounded, small, and structuredContent.error is the only
* machine-readable form of a failure a client gets. */
* The invariant, tightened by #1522: for a NON-error result whose payload is
* not a JSON object, the envelope must carry NO structuredContent key — not the
* payload a second time (#1375's duplication), and not an empty object either
* (#1488's replacement, which spec-compliant clients rendered as the entire
* result: "{}"). Object payloads keep their parsed structuredContent; errors
* keep structuredContent.error — bounded, small, and the only machine-readable
* form of a failure a client gets. */
TEST(mcp_every_tool_result_is_duplication_free) {
char tmp[256];
cbm_mcp_server_t *srv = setup_snippet_server(tmp, sizeof(tmp));
@@ -1863,30 +1876,36 @@ TEST(mcp_every_tool_result_is_duplication_free) {
const char *text = text_val ? yyjson_get_str(text_val) : NULL;
yyjson_val *structured = yyjson_obj_get(root, "structuredContent");
/* outputSchema is declared for every tool, so this stays mandatory. */
ASSERT_NOT_NULL(structured);
ASSERT_TRUE(yyjson_is_obj(structured));
yyjson_val *is_error = yyjson_obj_get(root, "isError");
bool errored = is_error && yyjson_is_true(is_error);
if (text && text[0] && !errored) {
/* If the payload is itself a JSON object, structuredContent is the
* PARSED form and legitimately holds the same data — that is the
* spec's structured+serialized pattern, not waste. Only the
* non-object case is checked here. */
if (errored) {
/* Errors keep machine-readable structure: either the wrapped
* {"error": <text>} form, or — when the error payload is itself a
* JSON object — that object parsed. Non-empty either way; an empty
* object is the #1522 lie in error clothing. */
ASSERT_NOT_NULL(structured);
ASSERT_TRUE(yyjson_is_obj(structured));
ASSERT_TRUE(yyjson_obj_size(structured) > 0);
} else if (text && text[0]) {
yyjson_doc *as_json = yyjson_read(text, strlen(text), 0);
bool payload_is_object = as_json && yyjson_is_obj(yyjson_doc_get_root(as_json));
if (as_json) {
yyjson_doc_free(as_json);
}
if (!payload_is_object) {
yyjson_val *dup = yyjson_obj_get(structured, "text");
if (dup && yyjson_is_str(dup)) {
const char *dup_str = yyjson_get_str(dup);
/* The exact defect: same bytes, twice, in one reply. */
ASSERT_TRUE(!(dup_str && strcmp(dup_str, text) == 0));
}
if (payload_is_object) {
/* JSON-object payloads: structuredContent is the PARSED form —
* the spec's structured+serialized pattern, not waste. It must
* be present and non-empty (an empty object beside a non-empty
* payload is exactly the #1522 lie). */
ASSERT_NOT_NULL(structured);
ASSERT_TRUE(yyjson_is_obj(structured));
ASSERT_TRUE(yyjson_obj_size(structured) > 0);
} else {
/* Text-shaped payloads (tree/TOON): NO structuredContent key.
* {} rendered as the whole result in schema-honoring clients
* (#1522); {"text": payload} doubled the wire cost (#1375). */
ASSERT_NULL(structured);
checked++;
}
}
@@ -1919,11 +1938,11 @@ TEST(tool_search_graph_includes_node_properties) {
"\"arguments\":{\"project\":\"test-project\",\"label\":\"Function\","
"\"name_pattern\":\"HandleRequest\",\"limit\":5}}}");
ASSERT_NOT_NULL(resp);
/* TOON is not a JSON object, so structuredContent stays empty rather than
* repeating the whole table a second time (#1375). The payload travels once,
* in content. */
ASSERT_NOT_NULL(strstr(resp, "\"structuredContent\":{}"));
ASSERT_NULL(strstr(resp, "\"structuredContent\":{\"text\":"));
/* TOON is not a JSON object, so the envelope has no structuredContent at
* all: {} was rendered as the entire result by schema-honoring clients
* (#1522), and {"text": ...} doubled the wire cost (#1375). The payload
* travels once, in content. */
ASSERT_NULL(strstr(resp, "\"structuredContent\""));
char *inner = extract_text_content(resp);
ASSERT_NOT_NULL(inner);
ASSERT_NOT_NULL(strstr(inner, "results:")); /* TOON table header */
@@ -10451,7 +10470,7 @@ SUITE(mcp) {
RUN_TEST(mcp_ingest_traces_items_disallow_additional_properties_issue731);
RUN_TEST(mcp_get_architecture_aspects_schema_enum_pr560);
RUN_TEST(mcp_text_result);
RUN_TEST(mcp_text_result_does_not_duplicate_plain_text_into_structured_content);
RUN_TEST(mcp_text_result_omits_structured_content_for_plain_text);
RUN_TEST(mcp_every_tool_result_is_duplication_free);
RUN_TEST(mcp_cancel_matches_request_id);
RUN_TEST(mcp_text_result_error);