diff --git a/scripts/smoke-test.sh b/scripts/smoke-test.sh index d4ffcb80d3..90efe52da3 100755 --- a/scripts/smoke-test.sh +++ b/scripts/smoke-test.sh @@ -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 diff --git a/src/cli/cli.c b/src/cli/cli.c index 6f13448170..70ed44a11d 100644 --- a/src/cli/cli.c +++ b/src/cli/cli.c @@ -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 Set a config value\n"); printf(" reset 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 \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 \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 \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]); diff --git a/src/daemon/frontend.c b/src/daemon/frontend.c index 8d97adbe7b..f5116934cf 100644 --- a/src/daemon/frontend.c +++ b/src/daemon/frontend.c @@ -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) { diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index 34773bed05..191823ddac 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -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": } 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": } — + * 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": } 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); diff --git a/tests/test_cli.c b/tests/test_cli.c index f35dd9d4c7..076ee2196f 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -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); diff --git a/tests/test_daemon_frontend.c b/tests/test_daemon_frontend.c index 56040b71b8..2044b30c1d 100644 --- a/tests/test_daemon_frontend.c +++ b/tests/test_daemon_frontend.c @@ -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); diff --git a/tests/test_mcp.c b/tests/test_mcp.c index cdb1ac62ea..8543300531 100644 --- a/tests/test_mcp.c +++ b/tests/test_mcp.c @@ -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": } 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": } 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": } 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);