Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 39 additions & 3 deletions Server/src/services/tools/read_console.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

from services.registry import mcp_for_unity_tool
from services.tools import get_unity_instance_from_context
from services.tools.utils import coerce_int, coerce_bool
from services.tools.utils import coerce_int, coerce_bool, parse_json_payload
from transport.unity_transport import send_with_unity_instance
from transport.legacy.unity_connection import async_send_command_with_retry

Expand All @@ -31,7 +31,8 @@ async def read_console(
action: Annotated[Literal['get', 'clear'],
"Get or clear the Unity Editor console. Defaults to 'get' if omitted."] | None = None,
types: Annotated[list[Literal['error', 'warning',
'log', 'all']], "Message types to get"] | None = None,
'log', 'all']] | str,
"Message types to get (accepts list or JSON string)"] | None = None,
count: Annotated[int | str,
"Max messages to return in non-paging mode (accepts int or string, e.g., 5 or '5'). Ignored when paging with page_size/cursor."] | None = None,
filter_text: Annotated[str, "Text filter for messages"] | None = None,
Expand All @@ -51,7 +52,42 @@ async def read_console(
unity_instance = get_unity_instance_from_context(ctx)
# Set defaults if values are None
action = action if action is not None else 'get'
types = types if types is not None else ['error', 'warning', 'log']

# Parse types if it's a JSON string (handles client compatibility issue #561)
if isinstance(types, str):
types = parse_json_payload(types)
# Validate types is a list after parsing
if types is not None and not isinstance(types, list):
return {
"success": False,
"message": (
f"types must be a list, got {type(types).__name__}. "
"If passing as JSON string, use format: '[\"error\", \"warning\"]'"
)
}
if types is not None:
allowed_types = {"error", "warning", "log", "all"}
normalized_types = []
for entry in types:
if not isinstance(entry, str):
return {
"success": False,
"message": f"types entries must be strings, got {type(entry).__name__}"
}
normalized = entry.strip().lower()
if normalized not in allowed_types:
return {
"success": False,
"message": (
f"invalid types entry '{entry}'. "
f"Allowed values: {sorted(allowed_types)}"
)
}
normalized_types.append(normalized)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: duplicate entries in types list aren't deduplicated (e.g. ["error", "error"] passes through as-is). Consider using list(dict.fromkeys(normalized_types)) to preserve order while deduplicating.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix With AI
This is a comment left during a code review.
Path: Server/src/services/tools/read_console.py
Line: 85:85

Comment:
**style:** duplicate entries in `types` list aren't deduplicated (e.g. `["error", "error"]` passes through as-is). Consider using `list(dict.fromkeys(normalized_types))` to preserve order while deduplicating.

<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>

How can I resolve this? If you propose a fix, please make it concise.

types = normalized_types
else:
types = ['error', 'warning', 'log']

format = format if format is not None else 'plain'
# Coerce booleans defensively (strings like 'true'/'false')

Expand Down
77 changes: 77 additions & 0 deletions Server/tests/integration/test_read_console_truncate.py
Original file line number Diff line number Diff line change
Expand Up @@ -183,3 +183,80 @@ async def fake_send(_cmd, params, **_kwargs):
assert len(resp["data"]["items"]) == 5
assert resp["data"]["truncated"] is False
assert resp["data"]["nextCursor"] is None


@pytest.mark.asyncio
async def test_read_console_types_json_string(monkeypatch):
"""Test that read_console handles types parameter as JSON string (fixes issue #561)."""
tools = setup_tools()
read_console = tools["read_console"]

captured = {}

async def fake_send_with_unity_instance(_send_fn, _unity_instance, _command_type, params, **_kwargs):
captured["params"] = params
return {
"success": True,
"data": {"lines": [{"level": "error", "message": "test error"}]},
}

import services.tools.read_console as read_console_mod
monkeypatch.setattr(
read_console_mod,
"send_with_unity_instance",
fake_send_with_unity_instance,
)

# Test with types as JSON string (the problematic case from issue #561)
resp = await read_console(ctx=DummyContext(), action="get", types='["error", "warning", "all"]')
assert resp["success"] is True
# Verify types was parsed correctly and sent as a list
assert isinstance(captured["params"]["types"], list)
assert captured["params"]["types"] == ["error", "warning", "all"]

# Test case normalization to lowercase
captured.clear()
resp = await read_console(ctx=DummyContext(), action="get", types='["ERROR", "Warning", "LOG"]')
assert resp["success"] is True
assert captured["params"]["types"] == ["error", "warning", "log"]

# Test with types as actual list (should still work)
captured.clear()
resp = await read_console(ctx=DummyContext(), action="get", types=["error", "warning"])
assert resp["success"] is True
assert isinstance(captured["params"]["types"], list)
assert captured["params"]["types"] == ["error", "warning"]


@pytest.mark.asyncio
async def test_read_console_types_validation(monkeypatch):
"""Test that read_console validates types entries and rejects invalid values."""
tools = setup_tools()
read_console = tools["read_console"]

captured = {}

async def fake_send_with_unity_instance(_send_fn, _unity_instance, _command_type, params, **_kwargs):
captured["params"] = params
return {"success": True, "data": {"lines": []}}

import services.tools.read_console as read_console_mod
monkeypatch.setattr(
read_console_mod,
"send_with_unity_instance",
fake_send_with_unity_instance,
)

# Invalid entry in list should return a clear error and not send.
captured.clear()
resp = await read_console(ctx=DummyContext(), action="get", types='["error", "nope"]')
assert resp["success"] is False
assert "invalid types entry" in resp["message"]
assert captured == {}

# Non-string entry should return a clear error and not send.
captured.clear()
resp = await read_console(ctx=DummyContext(), action="get", types='[1, "error"]')
assert resp["success"] is False
assert "types entries must be strings" in resp["message"]
assert captured == {}