Skip to content

Fix on config and log #921

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Scriptwonder merged 1 commit into from
Mar 11, 2026
+31 −50
Merged

Fix on config and log #921

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion MCPForUnity/Editor/Clients/Configurators/KiloCodeConfigurator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ public KiloCodeConfigurator() : base(new McpClient
windowsConfigPath = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData), "Code", "User", "globalStorage", "kilocode.kilo-code", "settings", "mcp_settings.json"),
macConfigPath = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), "Library", "Application Support", "Code", "User", "globalStorage", "kilocode.kilo-code", "settings", "mcp_settings.json"),
linuxConfigPath = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), ".config", "Code", "User", "globalStorage", "kilocode.kilo-code", "settings", "mcp_settings.json"),
IsVsCodeLayout = true
IsVsCodeLayout = false
})
{ }

Expand Down
31 changes: 8 additions & 23 deletions MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,20 +92,17 @@ private static void PopulateUnityNode(JObject unity, string uvPath, McpClient cl
if (unity["headers"] != null) unity.Remove("headers");
}

if (isVSCode)
// Cline expects streamableHttp for HTTP endpoints.
if (isCline)
{
unity["type"] = "http";
unity["type"] = "streamableHttp";
}
// Also add type for Claude Code (uses mcpServers layout but needs type field)
else if (client?.name == "Claude Code")
else
{
// "type" is standard MCP protocol; include for all clients to avoid
// clients that default to SSE when they see a URL without a type field.
unity["type"] = "http";
}
Comment on lines +95 to 105
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

This change introduces/retains a Cline-specific transport type ("streamableHttp") when using HTTP. There isn’t corresponding test coverage in WriteToConfigTests to lock in this behavior, so it’s easy to regress while refactoring the config builder. Consider adding a test case for a McpClient{name="Cline"} that asserts the generated config uses type=="streamableHttp" in HTTP mode (and still uses stdio type when HTTP is disabled).

Copilot uses AI. Check for mistakes.
// Cline expects streamableHttp for HTTP endpoints.
else if (isCline)
{
unity["type"] = "streamableHttp";
}
}
else
{
Expand All @@ -121,20 +118,8 @@ private static void PopulateUnityNode(JObject unity, string uvPath, McpClient cl
if (unity["url"] != null) unity.Remove("url");
if (unity["serverUrl"] != null) unity.Remove("serverUrl");

if (isVSCode)
{
unity["type"] = "stdio";
}
else if (isCline)
{
unity["type"] = "stdio";
}
}

// Remove type for non-VSCode clients (except clients that explicitly require it)
if (!isVSCode && client?.name != "Claude Code" && !isCline && unity["type"] != null)
{
unity.Remove("type");
// Include type for all clients — standard MCP protocol field.
unity["type"] = "stdio";
}

bool requiresEnv = client?.EnsureEnvObject == true;
Expand Down
10 changes: 9 additions & 1 deletion MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,15 @@ private static void ProcessCommand(string id, PendingCommand pending)
}

sw?.Stop();
McpLogRecord.Log(command.type, parameters, logType, "SUCCESS", sw?.ElapsedMilliseconds ?? 0);

string syncLogStatus = "SUCCESS";
string syncLogError = null;
if (result is ErrorResponse errResp)
{
syncLogStatus = "ERROR";
syncLogError = errResp.Error;
}
McpLogRecord.Log(command.type, parameters, logType, syncLogStatus, sw?.ElapsedMilliseconds ?? 0, syncLogError);
Comment on lines +406 to +413
Copy link
Contributor

@sourcery-ai sourcery-ai bot Mar 11, 2026

Choose a reason for hiding this comment

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

issue (bug_risk): Logged error status is inconsistent with the success status returned to the caller.

Because syncLogStatus can be "ERROR" while the returned anonymous object always has status = "success", external callers will treat error payloads as successful responses, while logs treat them as failures. Unless this is deliberate, update the returned status to reflect ErrorResponse (e.g., reuse syncLogStatus or map ErrorResponse to a non-success status).


var response = new { status = "success", result };
pending.TrySetResult(JsonConvert.SerializeObject(response));
Expand Down
11 changes: 7 additions & 4 deletions MCPForUnity/Editor/Tools/ReadConsole.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,10 +248,13 @@ bool includeStacktrace

try
{
// LogEntries requires calling Start/Stop around GetEntries/GetEntryInternal
_startGettingEntriesMethod.Invoke(null, null);

int totalEntries = (int)_getCountMethod.Invoke(null, null);
// LogEntries requires calling Start/Stop around GetEntries/GetEntryInternal.
// StartGettingEntries() returns the entry count — use it instead of GetCount()
// which may return stale values within an active iteration session.
object startResult = _startGettingEntriesMethod.Invoke(null, null);
int totalEntries = startResult is int startCount
? startCount
: (int)_getCountMethod.Invoke(null, null);
// Create instance to pass to GetEntryInternal - Ensure the type is correct
Type logEntryType = typeof(EditorApplication).Assembly.GetType(
"UnityEditor.LogEntry"
Expand Down
27 changes: 6 additions & 21 deletions TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,6 @@ private static void InvokeWriteToConfig(string configPath, McpClient client)
private static void AssertTransportConfiguration(JObject unity, McpClient client)
{
bool useHttp = EditorPrefs.GetBool(UseHttpTransportPrefKey, true);
bool isVSCode = client.IsVsCodeLayout;
bool isWindsurf = string.Equals(client.HttpUrlProperty, "serverUrl", StringComparison.OrdinalIgnoreCase);

if (useHttp)
Expand All @@ -429,16 +428,9 @@ private static void AssertTransportConfiguration(JObject unity, McpClient client
Assert.IsNull(unity["command"], "HTTP transport should remove command");
Assert.IsNull(unity["args"], "HTTP transport should remove args");

if (isVSCode)
{
Assert.AreEqual("http", (string)unity["type"],
"VSCode entries should advertise HTTP transport");
}
else
{
Assert.IsNull(unity["type"],
"Non-VSCode entries should not include type metadata in HTTP mode");
}
// "type" is now included for all clients (standard MCP protocol field).
Assert.AreEqual("http", (string)unity["type"],
"All entries should advertise HTTP transport type");
Comment on lines +432 to +433
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

AssertTransportConfiguration() now assumes HTTP always uses type=="http". Production config generation has a Cline-specific exception (type=="streamableHttp"), so this helper is no longer generally correct for all clients supported by the repo. Consider making the expected type conditional on the client (or passing expectedType in) to keep the helper aligned with ConfigJsonBuilder.

Suggested change
Assert.AreEqual("http", (string)unity["type"],
"All entries should advertise HTTP transport type");
Assert.IsTrue((string)unity["type"] == "http" || (string)unity["type"] == "streamableHttp",
"All HTTP entries should advertise a supported HTTP transport type (\"http\" or \"streamableHttp\")");

Copilot uses AI. Check for mistakes.
}
Comment on lines +431 to 434
Copy link
Contributor

@sourcery-ai sourcery-ai bot Mar 11, 2026

Choose a reason for hiding this comment

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

issue (bug_risk): AssertTransportConfiguration now assumes all HTTP entries use "http" type, which contradicts the new Cline "streamableHttp" behavior.

This helper now conflicts with ConfigJsonBuilder, which sets "streamableHttp" for Cline. If tests use AssertTransportConfiguration with a Cline McpClient, the assertion will be incorrect and mask the client-specific behavior.

Please either make the assertion depend on the client type (e.g., "streamableHttp" for Cline, "http" for others) or introduce a separate helper for client-specific expectations. Also ensure there is at least one test that validates a Cline HTTP configuration end-to-end against "streamableHttp".

else
{
Expand All @@ -458,16 +450,9 @@ private static void AssertTransportConfiguration(JObject unity, McpClient client
Assert.AreEqual("stdio", args[transportIndex + 1],
"--transport should be followed by stdio mode");

if (isVSCode)
{
Assert.AreEqual("stdio", (string)unity["type"],
"VSCode entries should advertise stdio transport");
}
else
{
Assert.IsNull(unity["type"],
"Non-VSCode entries should not include type metadata in stdio mode");
}
// "type" is now included for all clients (standard MCP protocol field).
Assert.AreEqual("stdio", (string)unity["type"],
"All entries should advertise stdio transport type");
}
}

Expand Down
Loading