fix: script execute void return no structured value error and valued return name is null error#783
Conversation
Previously, when executing a script that returns void (null result), the tool returned null which caused MCP error -32600. Now it returns a valid SerializedMember with name='result', typeName='System.Void', and value='Success'.
- Void return: return SerializedMember with name='result', typeName='System.Void' - Non-void return: set name to 'result' if serialize returns null/empty name
|
@tianlang0704 need to cover these three scenarios with tests. Please don't use "2" in the variable name, it has a smell. |
There was a problem hiding this comment.
Pull request overview
This PR updates the script-execute Unity MCP tool to avoid MCP structured-content errors when dynamically executed scripts return void or serialized values without a name.
Changes:
- Returns a
SerializedMembersuccess payload instead ofnullfor null/void execution results. - Backfills missing serialized return names with
"result"for reflected non-SerializedMemberreturn values.
- Void return: return SerializedMember with name='result', typeName='System.Void', value='Success' - Non-void null return: return SerializedMember with proper typeName and value='null' (as string) - Non-null return: ensure name is never null/empty, default to 'result'
- Test void return with typeName='System.Void' and value='Success' - Test value return (int) validates result structure - Test null return (string?) validates string typeName - Test body-only mode void return (isMethodBody=true) - Test compilation errors with LogAssert.Expect
|
added non-void return value path, returns actual type and "null" instead of Void and "Success" |
|
I have doubts about how this solution will work in a case where the C# code returns a data model class instead of just a primitive string. Still need to test it out with AI agents:
And with multiple different return types:
And the same thing just with arrays:
|
Test Results 12 files 564 suites 47m 45s ⏱️ Results for commit 4b84676. ♻️ This comment has been updated with latest results. |
There could be only two paths, null (which includes void type here) and not-null return values. Behavior only change when return value is null, which causes the MCP error, and in that case the value will always be "null", and the type name is whatever the type name string is (per copilot review requests, returning the correct type name is important, which makes sense). The not-null value path is not changed. From my understanding, this is where your concern is. It serializes the same way as before, no code was changed... If it was not tested, it was not tested... and it's not part of this modification. If test about the previous way to serialize different types need to be done, it should be done before and test as regression here; or in later modifications add tests to serialization module and everywhere else needs serialization tests all together... |
|
@tianlang0704 you are right. It looks good then. |
…r construction Merge the void and non-void result-is-null branches in Tool_Script.Execute into a single SerializedMember construction (they differed only in typeName and JSON value), and drop an unused System.Text.RegularExpressions using from the script-execute tests (Regex is referenced fully-qualified). Behavior is unchanged; the EditMode suite stays green (the 5 ai-editor-logs.txt IOException failures are the documented pre-existing TestToolConsole environmental flakes, outside this PR's diff). simplify-pass: 1
…ult member Replace the hardcoded "result" name and "object" typeName-fallback literals in the null-return SerializedMember construction with the existing JsonSchema.Result / JsonSchema.Object constants, matching the idiom already used across the plugin and test suite. Behavior-identical (constants equal the prior literals).
|
@tianlang0704 thank you. It is a good improvement, I just tested everything and it looks good. Going to merge and release soon. |
This is related to issue #779 and pr #781
pr #781 was a over-complicated solution so I closed it.
While looking into the code more closely I found script-execute with valued return also has problem with the name field being null, which causes another MCP error.
The agent tool used is Kilo Code extension for VSCode.
Before:
After:
Code modified should be minimal this time, but it still involves return value changes for exposed api, so need to pay attention to compatibility issue as copilot mentioned in #781.