Codex/optimize and paginate read console tool (#511)

* Optimize read_console defaults and paging

* Fix read_console truncate test expectations

* Reduce read_console default count from 50 to 10

Further optimize token usage by reducing the default count from 50 to 10 entries. Even 10-20 messages with stack traces can be token-heavy. Added tests for default behavior and paging functionality. Updated tool description to document defaults and paging support.

* Fix ReadConsoleTests to include log type messages

The default types filter changed to ['error', 'warning'] (excluding 'log'), so tests using Debug.Log() need to explicitly request log messages. Also added format='detailed' to HandleCommand_Get_Works test since it accesses structured message fields.

* Address CodeRabbit review feedback

- Fix property naming consistency: next_cursor -> nextCursor (C# camelCase)
- Remove redundant EndGettingEntries call from catch block (already in finally)
- Extract stacktrace stripping to helper function (reduce duplication)
- Fix test mock to match actual C# response structure (items, nextCursor, truncated, total)

* perf: add early exit optimization for ReadConsole paging

- Add early exit in paging loop once page is filled, avoiding iteration
  through remaining console entries (total becomes 'at least N')
- Prefix unused mock arguments with underscores in test_read_console_truncate.py
  to suppress Ruff linter warnings

* refactor: give pageSize independent default, clarify count semantics

- Change pageSize resolution from 'pageSize ?? count ?? 50' to 'pageSize ?? 50'
  so pageSize has its own default independent of count
- count now only serves as the non-paging limit
- Add XML docs to GetConsoleEntries with clear parameter descriptions
- Update Python tool annotations to document pageSize default (50) and
  clarify that count is ignored when paging
main
dsarno 2026-01-04 14:46:52 -08:00 committed by GitHub
parent 96b81caf26
commit b0f7a80df0
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 211 additions and 40 deletions

View File

@ -165,13 +165,17 @@ namespace MCPForUnity.Editor.Tools
// Extract parameters for 'get' // Extract parameters for 'get'
var types = var types =
(@params["types"] as JArray)?.Select(t => t.ToString().ToLower()).ToList() (@params["types"] as JArray)?.Select(t => t.ToString().ToLower()).ToList()
?? new List<string> { "error", "warning", "log" }; ?? new List<string> { "error", "warning" };
int? count = @params["count"]?.ToObject<int?>(); int? count = @params["count"]?.ToObject<int?>();
int? pageSize =
@params["pageSize"]?.ToObject<int?>()
?? @params["page_size"]?.ToObject<int?>();
int? cursor = @params["cursor"]?.ToObject<int?>();
string filterText = @params["filterText"]?.ToString(); string filterText = @params["filterText"]?.ToString();
string sinceTimestampStr = @params["sinceTimestamp"]?.ToString(); // TODO: Implement timestamp filtering string sinceTimestampStr = @params["sinceTimestamp"]?.ToString(); // TODO: Implement timestamp filtering
string format = (@params["format"]?.ToString() ?? "detailed").ToLower(); string format = (@params["format"]?.ToString() ?? "plain").ToLower();
bool includeStacktrace = bool includeStacktrace =
@params["includeStacktrace"]?.ToObject<bool?>() ?? true; @params["includeStacktrace"]?.ToObject<bool?>() ?? false;
if (types.Contains("all")) if (types.Contains("all"))
{ {
@ -186,7 +190,15 @@ namespace MCPForUnity.Editor.Tools
// Need a way to get timestamp per log entry. // Need a way to get timestamp per log entry.
} }
return GetConsoleEntries(types, count, filterText, format, includeStacktrace); return GetConsoleEntries(
types,
count,
pageSize,
cursor,
filterText,
format,
includeStacktrace
);
} }
else else
{ {
@ -218,9 +230,22 @@ namespace MCPForUnity.Editor.Tools
} }
} }
/// <summary>
/// Retrieves console log entries with optional filtering and paging.
/// </summary>
/// <param name="types">Log types to include (e.g., "error", "warning", "log").</param>
/// <param name="count">Maximum entries to return in non-paging mode. Ignored when paging is active.</param>
/// <param name="pageSize">Number of entries per page. Defaults to 50 when omitted.</param>
/// <param name="cursor">Starting index for paging (0-based). Defaults to 0.</param>
/// <param name="filterText">Optional text filter (case-insensitive substring match).</param>
/// <param name="format">Output format: "plain", "detailed", or "json".</param>
/// <param name="includeStacktrace">Whether to include stack traces in the output.</param>
/// <returns>A success response with entries, or an error response.</returns>
private static object GetConsoleEntries( private static object GetConsoleEntries(
List<string> types, List<string> types,
int? count, int? count,
int? pageSize,
int? cursor,
string filterText, string filterText,
string format, string format,
bool includeStacktrace bool includeStacktrace
@ -228,6 +253,12 @@ namespace MCPForUnity.Editor.Tools
{ {
List<object> formattedEntries = new List<object>(); List<object> formattedEntries = new List<object>();
int retrievedCount = 0; int retrievedCount = 0;
int totalMatches = 0;
bool usePaging = pageSize.HasValue || cursor.HasValue;
// pageSize defaults to 50 when omitted; count is the overall non-paging limit only
int resolvedPageSize = Mathf.Clamp(pageSize ?? 50, 1, 500);
int resolvedCursor = Mathf.Max(0, cursor ?? 0);
int pageEndExclusive = resolvedCursor + resolvedPageSize;
try try
{ {
@ -338,6 +369,24 @@ namespace MCPForUnity.Editor.Tools
break; break;
} }
totalMatches++;
if (usePaging)
{
if (totalMatches > resolvedCursor && totalMatches <= pageEndExclusive)
{
formattedEntries.Add(formattedEntry);
retrievedCount++;
}
// Early exit: we've filled the page and only need to check if more exist
else if (totalMatches > pageEndExclusive)
{
// We've passed the page; totalMatches now indicates truncation
break;
}
}
else
{
formattedEntries.Add(formattedEntry); formattedEntries.Add(formattedEntry);
retrievedCount++; retrievedCount++;
@ -348,17 +397,11 @@ namespace MCPForUnity.Editor.Tools
} }
} }
} }
}
catch (Exception e) catch (Exception e)
{ {
Debug.LogError($"[ReadConsole] Error while retrieving log entries: {e}"); Debug.LogError($"[ReadConsole] Error while retrieving log entries: {e}");
// Ensure EndGettingEntries is called even if there's an error during iteration // EndGettingEntries will be called in the finally block
try
{
_endGettingEntriesMethod.Invoke(null, null);
}
catch
{ /* Ignore nested exception */
}
return new ErrorResponse($"Error retrieving log entries: {e.Message}"); return new ErrorResponse($"Error retrieving log entries: {e.Message}");
} }
finally finally
@ -375,6 +418,26 @@ namespace MCPForUnity.Editor.Tools
} }
} }
if (usePaging)
{
bool truncated = totalMatches > pageEndExclusive;
string nextCursor = truncated ? pageEndExclusive.ToString() : null;
var payload = new
{
cursor = resolvedCursor,
pageSize = resolvedPageSize,
nextCursor = nextCursor,
truncated = truncated,
total = totalMatches,
items = formattedEntries,
};
return new SuccessResponse(
$"Retrieved {formattedEntries.Count} log entries.",
payload
);
}
// Return the filtered and formatted list (might be empty) // Return the filtered and formatted list (might be empty)
return new SuccessResponse( return new SuccessResponse(
$"Retrieved {formattedEntries.Count} log entries.", $"Retrieved {formattedEntries.Count} log entries.",

View File

@ -11,8 +11,15 @@ from transport.unity_transport import send_with_unity_instance
from transport.legacy.unity_connection import async_send_command_with_retry from transport.legacy.unity_connection import async_send_command_with_retry
def _strip_stacktrace_from_list(items: list) -> None:
"""Remove stacktrace fields from a list of log entries."""
for item in items:
if isinstance(item, dict) and "stacktrace" in item:
item.pop("stacktrace", None)
@mcp_for_unity_tool( @mcp_for_unity_tool(
description="Gets messages from or clears the Unity Editor console. Note: For maximum client compatibility, pass count as a quoted string (e.g., '5')." description="Gets messages from or clears the Unity Editor console. Defaults to 10 most recent entries. Use page_size/cursor for paging. Note: For maximum client compatibility, pass count as a quoted string (e.g., '5')."
) )
async def read_console( async def read_console(
ctx: Context, ctx: Context,
@ -21,10 +28,12 @@ async def read_console(
types: Annotated[list[Literal['error', 'warning', types: Annotated[list[Literal['error', 'warning',
'log', 'all']], "Message types to get"] | None = None, 'log', 'all']], "Message types to get"] | None = None,
count: Annotated[int | str, count: Annotated[int | str,
"Max messages to return (accepts int or string, e.g., 5 or '5')"] | None = None, "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, filter_text: Annotated[str, "Text filter for messages"] | None = None,
since_timestamp: Annotated[str, since_timestamp: Annotated[str,
"Get messages after this timestamp (ISO 8601)"] | None = None, "Get messages after this timestamp (ISO 8601)"] | None = None,
page_size: Annotated[int | str, "Page size for paginated console reads. Defaults to 50 when omitted."] | None = None,
cursor: Annotated[int | str, "Opaque cursor for paging (0-based offset). Defaults to 0."] | None = None,
format: Annotated[Literal['plain', 'detailed', format: Annotated[Literal['plain', 'detailed',
'json'], "Output format"] | None = None, 'json'], "Output format"] | None = None,
include_stacktrace: Annotated[bool | str, include_stacktrace: Annotated[bool | str,
@ -35,11 +44,13 @@ async def read_console(
unity_instance = get_unity_instance_from_context(ctx) unity_instance = get_unity_instance_from_context(ctx)
# Set defaults if values are None # Set defaults if values are None
action = action if action is not None else 'get' action = action if action is not None else 'get'
types = types if types is not None else ['error', 'warning', 'log'] types = types if types is not None else ['error', 'warning']
format = format if format is not None else 'detailed' format = format if format is not None else 'plain'
# Coerce booleans defensively (strings like 'true'/'false') # Coerce booleans defensively (strings like 'true'/'false')
include_stacktrace = coerce_bool(include_stacktrace, default=True) include_stacktrace = coerce_bool(include_stacktrace, default=False)
coerced_page_size = coerce_int(page_size, default=None)
coerced_cursor = coerce_int(cursor, default=None)
# Normalize action if it's a string # Normalize action if it's a string
if isinstance(action, str): if isinstance(action, str):
@ -56,7 +67,7 @@ async def read_console(
count = coerce_int(count) count = coerce_int(count)
if action == "get" and count is None: if action == "get" and count is None:
count = 200 count = 10
# Prepare parameters for the C# handler # Prepare parameters for the C# handler
params_dict = { params_dict = {
@ -65,6 +76,8 @@ async def read_console(
"count": count, "count": count,
"filterText": filter_text, "filterText": filter_text,
"sinceTimestamp": since_timestamp, "sinceTimestamp": since_timestamp,
"pageSize": coerced_page_size,
"cursor": coerced_cursor,
"format": format.lower() if isinstance(format, str) else format, "format": format.lower() if isinstance(format, str) else format,
"includeStacktrace": include_stacktrace "includeStacktrace": include_stacktrace
} }
@ -83,16 +96,13 @@ async def read_console(
# Strip stacktrace fields from returned lines if present # Strip stacktrace fields from returned lines if present
try: try:
data = resp.get("data") data = resp.get("data")
# Handle standard format: {"data": {"lines": [...]}} if isinstance(data, dict):
if isinstance(data, dict) and "lines" in data and isinstance(data["lines"], list): for key in ("lines", "items"):
for line in data["lines"]: if key in data and isinstance(data[key], list):
if isinstance(line, dict) and "stacktrace" in line: _strip_stacktrace_from_list(data[key])
line.pop("stacktrace", None) break
# Handle legacy/direct list format if any
elif isinstance(data, list): elif isinstance(data, list):
for line in data: _strip_stacktrace_from_list(data)
if isinstance(line, dict) and "stacktrace" in line:
line.pop("stacktrace", None)
except Exception: except Exception:
pass pass
return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)} return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)}

View File

@ -36,7 +36,7 @@ async def test_read_console_full_default(monkeypatch):
captured = {} captured = {}
async def fake_send(cmd, params, **kwargs): async def fake_send(_cmd, params, **_kwargs):
captured["params"] = params captured["params"] = params
return { return {
"success": True, "success": True,
@ -54,10 +54,10 @@ async def test_read_console_full_default(monkeypatch):
resp = await read_console(ctx=DummyContext(), action="get", count=10) resp = await read_console(ctx=DummyContext(), action="get", count=10)
assert resp == { assert resp == {
"success": True, "success": True,
"data": {"lines": [{"level": "error", "message": "oops", "stacktrace": "trace", "time": "t"}]}, "data": {"lines": [{"level": "error", "message": "oops", "time": "t"}]},
} }
assert captured["params"]["count"] == 10 assert captured["params"]["count"] == 10
assert captured["params"]["includeStacktrace"] is True assert captured["params"]["includeStacktrace"] is False
@pytest.mark.asyncio @pytest.mark.asyncio
@ -67,7 +67,7 @@ async def test_read_console_truncated(monkeypatch):
captured = {} captured = {}
async def fake_send(cmd, params, **kwargs): async def fake_send(_cmd, params, **_kwargs):
captured["params"] = params captured["params"] = params
return { return {
"success": True, "success": True,
@ -86,3 +86,100 @@ async def test_read_console_truncated(monkeypatch):
assert resp == {"success": True, "data": { assert resp == {"success": True, "data": {
"lines": [{"level": "error", "message": "oops"}]}} "lines": [{"level": "error", "message": "oops"}]}}
assert captured["params"]["includeStacktrace"] is False assert captured["params"]["includeStacktrace"] is False
@pytest.mark.asyncio
async def test_read_console_default_count(monkeypatch):
"""Test that read_console defaults to count=10 when not specified."""
tools = setup_tools()
read_console = tools["read_console"]
captured = {}
async def fake_send(_cmd, params, **_kwargs):
captured["params"] = params
return {
"success": True,
"data": {"lines": [{"level": "error", "message": f"error {i}"} for i in range(15)]},
}
# Patch the send_command_with_retry function in the tools module
import services.tools.read_console
monkeypatch.setattr(
services.tools.read_console,
"async_send_command_with_retry",
fake_send,
)
# Call without specifying count - should default to 10
resp = await read_console(ctx=DummyContext(), action="get")
assert resp["success"] is True
# Verify that the default count of 10 was used
assert captured["params"]["count"] == 10
@pytest.mark.asyncio
async def test_read_console_paging(monkeypatch):
"""Test that read_console paging works with page_size and cursor."""
tools = setup_tools()
read_console = tools["read_console"]
captured = {}
async def fake_send(_cmd, params, **_kwargs):
captured["params"] = params
# Simulate Unity returning paging info matching C# structure
page_size = params.get("pageSize", 10)
cursor = params.get("cursor", 0)
# Simulate 25 total messages
all_messages = [{"level": "error", "message": f"error {i}"} for i in range(25)]
# Return a page of results
start = cursor
end = min(start + page_size, len(all_messages))
messages = all_messages[start:end]
return {
"success": True,
"data": {
"items": messages,
"cursor": cursor,
"pageSize": page_size,
"nextCursor": str(end) if end < len(all_messages) else None,
"truncated": end < len(all_messages),
"total": len(all_messages),
},
}
# Patch the send_command_with_retry function in the tools module
import services.tools.read_console
monkeypatch.setattr(
services.tools.read_console,
"async_send_command_with_retry",
fake_send,
)
# First page - get first 5 entries
resp = await read_console(ctx=DummyContext(), action="get", page_size=5, cursor=0)
assert resp["success"] is True
assert captured["params"]["pageSize"] == 5
assert captured["params"]["cursor"] == 0
assert len(resp["data"]["items"]) == 5
assert resp["data"]["truncated"] is True
assert resp["data"]["nextCursor"] == "5"
assert resp["data"]["total"] == 25
# Second page - get next 5 entries
resp = await read_console(ctx=DummyContext(), action="get", page_size=5, cursor=5)
assert resp["success"] is True
assert captured["params"]["cursor"] == 5
assert len(resp["data"]["items"]) == 5
assert resp["data"]["truncated"] is True
assert resp["data"]["nextCursor"] == "10"
# Last page - get remaining entries
resp = await read_console(ctx=DummyContext(), action="get", page_size=5, cursor=20)
assert resp["success"] is True
assert len(resp["data"]["items"]) == 5
assert resp["data"]["truncated"] is False
assert resp["data"]["nextCursor"] is None

View File

@ -809,7 +809,7 @@ wheels = [
[[package]] [[package]]
name = "mcpforunityserver" name = "mcpforunityserver"
version = "8.6.0" version = "8.7.0"
source = { editable = "." } source = { editable = "." }
dependencies = [ dependencies = [
{ name = "fastapi" }, { name = "fastapi" },

View File

@ -19,7 +19,7 @@ namespace MCPForUnityTests.Editor.Tools
Debug.Log("Log to clear"); Debug.Log("Log to clear");
// Verify content exists before clear // Verify content exists before clear
var getBefore = ToJObject(ReadConsole.HandleCommand(new JObject { ["action"] = "get", ["count"] = 10 })); var getBefore = ToJObject(ReadConsole.HandleCommand(new JObject { ["action"] = "get", ["types"] = new JArray { "error", "warning", "log" }, ["count"] = 10 }));
Assert.IsTrue(getBefore.Value<bool>("success"), getBefore.ToString()); Assert.IsTrue(getBefore.Value<bool>("success"), getBefore.ToString());
var entriesBefore = getBefore["data"] as JArray; var entriesBefore = getBefore["data"] as JArray;
@ -35,7 +35,7 @@ namespace MCPForUnityTests.Editor.Tools
Assert.IsTrue(result.Value<bool>("success"), result.ToString()); Assert.IsTrue(result.Value<bool>("success"), result.ToString());
// Verify clear effect // Verify clear effect
var getAfter = ToJObject(ReadConsole.HandleCommand(new JObject { ["action"] = "get", ["count"] = 10 })); var getAfter = ToJObject(ReadConsole.HandleCommand(new JObject { ["action"] = "get", ["types"] = new JArray { "error", "warning", "log" }, ["count"] = 10 }));
Assert.IsTrue(getAfter.Value<bool>("success"), getAfter.ToString()); Assert.IsTrue(getAfter.Value<bool>("success"), getAfter.ToString());
var entriesAfter = getAfter["data"] as JArray; var entriesAfter = getAfter["data"] as JArray;
Assert.IsTrue(entriesAfter == null || entriesAfter.Count == 0, "Console should be empty after clear."); Assert.IsTrue(entriesAfter == null || entriesAfter.Count == 0, "Console should be empty after clear.");
@ -51,6 +51,8 @@ namespace MCPForUnityTests.Editor.Tools
var paramsObj = new JObject var paramsObj = new JObject
{ {
["action"] = "get", ["action"] = "get",
["types"] = new JArray { "error", "warning", "log" },
["format"] = "detailed",
["count"] = 1000 // Fetch enough to likely catch our message ["count"] = 1000 // Fetch enough to likely catch our message
}; };
@ -88,4 +90,3 @@ namespace MCPForUnityTests.Editor.Tools
} }
} }
} }