fix(execute_code): skip phantom CodeDom BOM error from mcs compiler - #1250
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe ChangesExecuteCode compilation fix
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant CodeDomCompile
participant CodeDomProvider
participant FileSystem
Caller->>CodeDomCompile: invoke with user code
CodeDomCompile->>CodeDomProvider: compile to temp DLL path
CodeDomProvider-->>CodeDomCompile: CompilerResults with errors/warnings
CodeDomCompile->>CodeDomCompile: filter bogus BOM/whitespace errors
alt non-bogus errors remain
CodeDomCompile-->>Caller: return null
else no real errors
CodeDomCompile->>FileSystem: verify DLL exists
CodeDomCompile->>FileSystem: read DLL bytes
CodeDomCompile->>CodeDomCompile: Assembly.Load(bytes)
CodeDomCompile->>FileSystem: delete temp DLL
CodeDomCompile-->>Caller: return loaded assembly
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
MCPForUnity/Editor/Tools/ExecuteCode.cs (1)
297-349: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid fix for the phantom mcs BOM error; consider extracting the filter for readability.
The disk-based compile, BOM/whitespace error filtering, and load-then-cleanup sequence are correct —
Assembly.Loadreads bytes into memory first, so deleting the temp DLL infinally(Lines 344-348) won't race with a file lock. Given the flagged complexity of this block, consider pulling the per-error bogus-check (Lines 323-326) into a small named helper (e.g.IsBogusBomError(CompilerError)) to make the filtering intent self-documenting and independently testable.♻️ Optional extraction
+ private static bool IsBogusBomError(CompilerError error) + { + string text = (error.ErrorText ?? "").Trim('\uFEFF', ' ', '\t', '\r', '\n'); + return string.IsNullOrEmpty(error.ErrorNumber) && string.IsNullOrEmpty(text); + } + bool hasRealErrors = false; foreach (CompilerError error in results.Errors) { if (error.IsWarning) continue; - // The bogus BOM "error": no error number, text is just the BOM/whitespace. - string text = (error.ErrorText ?? "").Trim('\uFEFF', ' ', '\t', '\r', '\n'); - if (string.IsNullOrEmpty(error.ErrorNumber) && string.IsNullOrEmpty(text)) + if (IsBogusBomError(error)) continue;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MCPForUnity/Editor/Tools/ExecuteCode.cs` around lines 297 - 349, The compile-and-filter logic in ExecuteCode is correct, but the BOM/whitespace bogus-error check is too inline and hard to read. Extract the per-error filter used inside the CompilerError loop into a small helper such as IsBogusBomError(CompilerError) and use it from the CompileAssemblyFromSource results handling, so the intent is self-documenting and easier to test without changing the load-and-cleanup flow.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Around line 45-54: The PR creation example is hardcoded with a specific fork
username, which makes the generic workflow misleading. Update the gh pr create
example in AGENTS.md to use a placeholder or generic head format instead of
beast-ofcourse:fix/your-fix, keeping the rest of the command structure intact so
any contributor can substitute their own fork name.
---
Nitpick comments:
In `@MCPForUnity/Editor/Tools/ExecuteCode.cs`:
- Around line 297-349: The compile-and-filter logic in ExecuteCode is correct,
but the BOM/whitespace bogus-error check is too inline and hard to read. Extract
the per-error filter used inside the CompilerError loop into a small helper such
as IsBogusBomError(CompilerError) and use it from the CompileAssemblyFromSource
results handling, so the intent is self-documenting and easier to test without
changing the load-and-cleanup flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4e8a498-a425-44e4-9368-1def2f658451
📒 Files selected for processing (2)
AGENTS.mdMCPForUnity/Editor/Tools/ExecuteCode.cs
| ```bash | ||
| git push origin fix/your-fix | ||
| gh pr create \ | ||
| --repo CoplayDev/unity-mcp \ | ||
| --base beta \ | ||
| --head beast-ofcourse:fix/your-fix \ | ||
| --title "[<area>]: <short description>" \ | ||
| --body "<body>" \ | ||
| --label bug | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hardcoded fork username in the generic PR-creation example.
--head beast-ofcourse:fix/your-fix bakes in a specific contributor's GitHub handle. As a general workflow guide for any AI agent/contributor, this example will mislead anyone else following it verbatim.
✏️ Suggested fix
gh pr create \
--repo CoplayDev/unity-mcp \
--base beta \
- --head beast-ofcourse:fix/your-fix \
+ --head <your-github-username>:fix/your-fix \
--title "[<area>]: <short description>" \
--body "<body>" \
--label bug📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```bash | |
| git push origin fix/your-fix | |
| gh pr create \ | |
| --repo CoplayDev/unity-mcp \ | |
| --base beta \ | |
| --head beast-ofcourse:fix/your-fix \ | |
| --title "[<area>]: <short description>" \ | |
| --body "<body>" \ | |
| --label bug | |
| ``` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AGENTS.md` around lines 45 - 54, The PR creation example is hardcoded with a
specific fork username, which makes the generic workflow misleading. Update the
gh pr create example in AGENTS.md to use a placeholder or generic head format
instead of beast-ofcourse:fix/your-fix, keeping the rest of the command
structure intact so any contributor can substitute their own fork name.
bfb743c to
1f396f9
Compare
Mono's mcs compiler emits a stray U+FEFF BOM character on stdout during compilation. CodeDom's CSharpCodeProvider misinterprets this as a compiler error (no ErrorNumber, ErrorText is just the BOM), causing HasErrors to be true and results.CompiledAssembly to be null — even though mcs exits 0 and wrote the DLL successfully. Fix: - Compile to a temp DLL path (GenerateInMemory=false + OutputAssembly) instead of relying on results.CompiledAssembly. - Skip phantom errors where ErrorNumber is empty and ErrorText is only BOM/whitespace. - Load the produced DLL via Assembly.Load(File.ReadAllBytes(...)) when no real errors exist. - Clean up the temp DLL in a finally block. Fixes CoplayDev#1186
1f396f9 to
127315b
Compare
2 participants
Problem
The \execute_code\ tool consistently fails with a phantom compilation error when using the CodeDom compiler on Mono/Windows. The error reports a lone U+FEFF (BOM) character on line 1, even though compilation actually succeeds (mcs exits 0 and writes the output DLL).
Fixes #1186
Root Cause
Mono's \mcs\ compiler prints a stray U+FEFF BOM character on stdout during compilation. Mono's CodeDom implementation (\CSharpCodeProvider) can't parse this as a standard compiler error format (\ ile(line,col): error CSxxxx), so it fabricates a \CompilerError\ with:
This causes
esults.Errors.HasErrors\ to return \ rue\ and
esults.CompiledAssembly\ to be
ull\ — even though mcs compiled successfully. The tool reports a failure for a build that was actually fine.
Solution
Three changes to the \CodeDomCompile\ method:
Compile to a temp DLL path instead of in-memory (\GenerateInMemory=false\ + \OutputAssembly). This bypasses CodeDom's refusal to surface the assembly when it thinks there are errors.
Skip phantom BOM errors — errors where \ErrorNumber\ is empty and \ErrorText\ is only BOM/whitespace are benign and should not block execution.
Load the DLL manually — when no real errors exist, load the produced DLL via \Assembly.Load(File.ReadAllBytes(...))\ and clean up the temp file in a \ inally\ block.
Verification
Notes
esponseFilePath\ cleanup (existing pattern) is unaffected — both temp files (RSP and DLL) are cleaned up in their respective \ inally\ blocks.
Summary by CodeRabbit