diff --git a/tests/tools/test_mcp_tool.py b/tests/tools/test_mcp_tool.py index c299e506d1ae..67929d438c09 100644 --- a/tests/tools/test_mcp_tool.py +++ b/tests/tools/test_mcp_tool.py @@ -47,6 +47,46 @@ def _make_mock_server(name, session=None, tools=None): return server +class TestFilterMCPChildren: + def test_filters_gateway_children_by_argv_marker(self, monkeypatch): + """Non-MCP children start with an interpreter/binary, not the marker.""" + import sys + + import tools.mcp_tool as mcp_tool + + cmdlines = { + 101: [ + "/usr/bin/python3", + "-m", + "tui_gateway.slash_worker", + "--session-key", + "abc", + ], + 102: [ + "/usr/bin/java", + "-jar", + "/opt/jdtls/plugins/org.eclipse.equinox.launcher_1.7.0.jar", + ], + 103: ["/usr/bin/node", "server.js"], + } + + class FakeProcess: + def __init__(self, pid): + self.pid = pid + + def cmdline(self): + return cmdlines[self.pid] + + fake_psutil = SimpleNamespace( + Process=FakeProcess, + NoSuchProcess=ProcessLookupError, + AccessDenied=PermissionError, + ) + monkeypatch.setitem(sys.modules, "psutil", fake_psutil) + + assert mcp_tool._filter_mcp_children({101, 102, 103}) == {103} + + # --------------------------------------------------------------------------- # Config loading # --------------------------------------------------------------------------- @@ -235,6 +275,145 @@ def test_nested_definition_refs_are_rewritten_recursively(self): assert schema["parameters"]["properties"]["items"]["items"]["$ref"] == "#/$defs/Entry" assert schema["parameters"]["$defs"]["Entry"]["properties"]["child"]["$ref"] == "#/$defs/Child" + def test_definitions_as_property_name_is_preserved(self): + """A tool parameter literally named ``definitions`` must not be renamed. + + Regression: the rewrite that promotes the legacy ``definitions`` + meta-keyword to ``$defs`` used to fire for *any* key named + ``definitions`` anywhere in the tree, including inside ``properties`` + dicts. That turned user-facing parameter names into ``$defs``, which + Anthropic and OpenAI both reject because ``$`` is not in the + ``^[a-zA-Z0-9_.-]{1,64}$`` property-name pattern. Real-world repro: a + CI/pipelines MCP tool whose ``definitions`` parameter is an array of + pipeline-definition IDs. + """ + from tools.mcp_tool import _convert_mcp_schema + + mcp_tool = _make_mcp_tool( + name="pipelines_build", + description="List pipeline builds", + input_schema={ + "type": "object", + "properties": { + "action": {"type": "string"}, + "definitions": { + "description": "Array of build definition IDs to filter builds.", + }, + "top": {"type": "integer"}, + }, + }, + ) + + schema = _convert_mcp_schema("pipelines", mcp_tool) + + props = schema["parameters"]["properties"] + assert "definitions" in props, "user-facing property name was renamed away" + assert "$defs" not in props, "user-facing property name was rewritten to $defs" + # And the meta-keyword promotion didn't happen at the root either, + # because there was no `definitions` meta-keyword to promote. + assert "$defs" not in schema["parameters"] + assert "definitions" not in schema["parameters"] + + def test_definitions_property_and_meta_keyword_coexist(self): + """``definitions`` as both a property name AND a meta-keyword in the + same schema. The property name stays; the meta-keyword is promoted. + + Note: Python source can't express both keys as literals (the second + would clobber the first), so build the dict explicitly. + """ + from tools.mcp_tool import _convert_mcp_schema + + input_schema = { + "type": "object", + "properties": { + # User-facing parameter literally named "definitions". + "definitions": { + "description": "Array of build definition IDs.", + }, + "payload": {"$ref": "#/definitions/Payload"}, + }, + } + # Meta-keyword (legacy draft-07 reusable defs), set after the literal. + input_schema["definitions"] = { + "Payload": { + "type": "object", + "properties": {"q": {"type": "string"}}, + }, + } + + mcp_tool = _make_mcp_tool( + name="mixed", + description="Schema with both forms of `definitions`", + input_schema=input_schema, + ) + + schema = _convert_mcp_schema("mixed", mcp_tool) + + # Property name preserved. + assert "definitions" in schema["parameters"]["properties"] + assert "$defs" not in schema["parameters"]["properties"] + # Meta-keyword promoted at the root. + assert "$defs" in schema["parameters"] + assert "definitions" not in schema["parameters"] + # The $ref into the legacy location was rewritten too. + assert schema["parameters"]["properties"]["payload"]["$ref"] == "#/$defs/Payload" + + def test_property_named_required_does_not_inject_phantom_params(self): + """A tool parameter literally named ``required`` must not spawn + phantom ``type`` / ``properties`` parameters. + + Regression: the object-shape repair (fill missing ``type``, ensure a + ``properties`` dict) used to recurse into the ``properties`` *map* the + same way it recurses into a schema node. When a tool declared a + parameter literally named ``required`` (or ``properties``), the map + itself looked object-shaped to the repair, so it stamped a ``type: + object`` and an empty ``properties`` onto the map — surfacing as + parameters ``type`` and ``properties`` that the MCP server never + declared, which the model could then fill with junk args. Sibling + ``_rewrite_local_refs`` already gates ``properties``; this path did + not. Real-world repro: a schema-builder / validation MCP tool whose + ``required`` parameter is an array of required field names. + """ + from tools.mcp_tool import _normalize_mcp_input_schema + + schema = _normalize_mcp_input_schema({ + "type": "object", + "properties": {"required": {"type": "boolean"}}, + }) + + assert set(schema["properties"].keys()) == {"required"} + assert schema["properties"]["required"] == {"type": "boolean"} + + def test_property_named_properties_does_not_inject_phantom_type(self): + """A tool parameter literally named ``properties`` is left untouched.""" + from tools.mcp_tool import _normalize_mcp_input_schema + + schema = _normalize_mcp_input_schema({ + "type": "object", + "properties": {"properties": {"type": "string"}}, + }) + + assert set(schema["properties"].keys()) == {"properties"} + assert schema["properties"]["properties"] == {"type": "string"} + + def test_property_named_required_is_preserved_when_nested(self): + """The phantom-param gate also holds inside nested object properties.""" + from tools.mcp_tool import _normalize_mcp_input_schema + + schema = _normalize_mcp_input_schema({ + "type": "object", + "properties": { + "config": { + "type": "object", + "properties": {"required": {"type": "boolean"}}, + }, + }, + }) + + nested = schema["properties"]["config"]["properties"] + assert set(nested.keys()) == {"required"} + assert nested["required"] == {"type": "boolean"} + def test_missing_type_on_object_is_coerced(self): """Schemas that describe an object but omit ``type`` get type='object'.""" from tools.mcp_tool import _normalize_mcp_input_schema diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index c125db62a11f..5dd1991d0b78 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -1867,7 +1867,15 @@ async def _run_stdio(self, config: dict): write_stream, ): # Capture the newly spawned subprocess PID for force-kill cleanup. - new_pids = _snapshot_child_pids() - pids_before + # Filter out non-MCP children that race into the snapshot window: + # slash_worker and LSP servers (jdtls/pyright/yaml-ls) are spawned + # directly by the gateway without start_new_session, so their pgid + # equals the TUI parent PID. If they leak into _stdio_pgids, the + # shutdown sweep's killpg() kills the TUI parent itself. + # See agent/lsp/client.py for the complementary start_new_session fix. + new_pids = _filter_mcp_children( + _snapshot_child_pids() - pids_before + ) if new_pids: # Capture pgid while the child is alive — once it exits we # can no longer call ``os.getpgid`` on it, and the cleanup @@ -3005,6 +3013,56 @@ def _snapshot_child_pids() -> set: return set() +# Non-MCP gateway children that can race into the _snapshot_child_pids() delta +# during stdio MCP server spawn. LSP servers and slash_worker now use +# start_new_session=True too; this remains defense-in-depth for any future +# non-MCP child spawn that briefly appears in the MCP snapshot delta. Match +# argv markers instead of argv[0] because Python/Java children begin with the +# interpreter or binary path. +_NON_MCP_CHILD_CMDLINE_MARKERS: tuple[str, ...] = ( + "tui_gateway.slash_worker", + "tui_gateway.entry", + "-dorg.eclipse.equinox.launcher", # jdtls (legacy arg style) + "eclipse.jdt.ls", + "org.eclipse.equinox.launcher_", +) + + +def _filter_mcp_children(pids: set) -> set: + """Remove non-MCP children from a PID snapshot delta. + + _snapshot_child_pids() returns *all* direct children of the gateway. When + a stdio MCP server spawns concurrently with a slash_worker or LSP server + spawn, the delta ``_snapshot_child_pids() - pids_before`` can include + PIDs that are NOT the MCP server. Tracking those PIDs in _stdio_pgids is + catastrophic if a future child lacks start_new_session: its pgid can be the + TUI parent's PID, so the shutdown sweep's killpg() kills the TUI itself. + """ + if not pids: + return pids + try: + import psutil + except ImportError: + # psutil unavailable — keep all PIDs (preserves prior behavior). + return pids + filtered: set = set() + for pid in pids: + try: + argv = psutil.Process(pid).cmdline() + except (psutil.NoSuchProcess, psutil.AccessDenied, OSError): + # Process raced away or is a zombie — skip it; it cannot be the + # MCP server we just spawned and is not safe to track. + continue + if any( + marker in arg + for arg in argv[1:] + for marker in _NON_MCP_CHILD_CMDLINE_MARKERS + ): + continue + filtered.add(pid) + return filtered + + def _mcp_loop_exception_handler(loop, context): """Suppress benign 'Event loop is closed' noise during shutdown. @@ -3731,11 +3789,43 @@ def _normalize_mcp_input_schema(schema: dict | None) -> dict: return {"type": "object", "properties": {}} def _rewrite_local_refs(node): + """Walk the schema, promoting legacy ``definitions`` to ``$defs``. + + The promotion is contextual: ``definitions`` is renamed only when it + appears as a JSON Schema *meta-keyword* (sibling of ``properties`` / + ``$ref`` at a schema node), never when it appears as the *name of a + property* (i.e., as a key inside a ``properties`` dict). + + Without this gate, MCP servers that legitimately expose a tool + parameter named ``definitions`` (e.g. a CI/pipelines tool that uses + ``definitions`` for an array of pipeline-definition IDs) would have + that user-facing property name silently rewritten to ``$defs``. + Anthropic and OpenAI both reject ``$`` in property names + (``^[a-zA-Z0-9_.-]{1,64}$``), so the whole tool array gets a 400 and + every conversation breaks. + + The gate works by treating ``properties`` and ``patternProperties`` + specially during descent: we iterate the property-name -> schema map + directly, leaving the property names verbatim, then recurse into each + property's schema where ordinary JSON Schema semantics resume (so any + legitimately-nested ``definitions`` meta-keyword inside a property's + schema is still promoted). + """ if isinstance(node, dict): normalized = {} for key, value in node.items(): - out_key = "$defs" if key == "definitions" else key - normalized[out_key] = _rewrite_local_refs(value) + if key in ("properties", "patternProperties") and isinstance(value, dict): + # Keys of this dict are user-facing property names, not + # meta-keywords. Preserve them verbatim; recurse only into + # each property's schema, where ``definitions`` again has + # its JSON Schema meaning. + normalized[key] = { + prop_name: _rewrite_local_refs(prop_schema) + for prop_name, prop_schema in value.items() + } + else: + out_key = "$defs" if key == "definitions" else key + normalized[out_key] = _rewrite_local_refs(value) ref = normalized.get("$ref") if isinstance(ref, str) and ref.startswith("#/definitions/"): normalized["$ref"] = "#/$defs/" + ref[len("#/definitions/"):] @@ -3764,7 +3854,23 @@ def _repair_object_shape(node): if not isinstance(node, dict): return node - repaired = {k: _repair_object_shape(v) for k, v in node.items()} + # Recurse, but treat ``properties`` / ``patternProperties`` as + # name -> schema maps: their KEYS are user-facing parameter names, not + # JSON Schema meta-keywords. Recursing into such a map as if it were a + # schema node makes the object-shape repair below fire on the map + # itself whenever a tool declares a parameter literally named + # ``required`` or ``properties``, injecting phantom ``type`` / + # ``properties`` parameters the server never declared. This mirrors the + # identical gate in ``_rewrite_local_refs`` above. + repaired = {} + for k, v in node.items(): + if k in ("properties", "patternProperties") and isinstance(v, dict): + repaired[k] = { + prop_name: _repair_object_shape(prop_schema) + for prop_name, prop_schema in v.items() + } + else: + repaired[k] = _repair_object_shape(v) # Coerce missing / null type when the shape is clearly an object # (has properties or required but no type).