[bug-fix]Duplicate signature fix#906
Merged
Scriptwonder merged 2 commits intoCoplayDev:betafrom Mar 10, 2026
Merged
Conversation
Contributor
Reviewer's GuideAdds a structural duplicate-method-signature detection pass to Unity script validation and introduces focused unit tests that verify correct handling of duplicates versus legitimate variations, including comments, inner classes, overloads, and generic parameters. Sequence diagram for updated script validation with duplicate signature checksequenceDiagram
actor Developer
participant ManageScript
participant CSharpLexer
participant RegexEngine
participant ErrorList
Developer->>ManageScript: ValidateScriptSyntax(contents, level, errorList)
alt syntax invalid
ManageScript-->>Developer: false
else syntax structurally valid
Note over ManageScript: New step: duplicate signature validation
ManageScript->>ManageScript: CheckDuplicateMethodSignatures(contents, errorList)
ManageScript->>CSharpLexer: new CSharpLexer(contents)
loop tokenize non code
CSharpLexer-->>ManageScript: tokens with InNonCode
ManageScript->>ManageScript: mask comments and strings in codeChars
end
ManageScript->>ManageScript: build depthArr from codeOnly
ManageScript->>RegexEngine: match methodSigPattern on codeOnly
loop each matched method
RegexEngine-->>ManageScript: method name and params
ManageScript->>ManageScript: CountTopLevelParams(paramStr)
ManageScript->>ManageScript: ExtractParamTypes(paramStr)
ManageScript->>ErrorList: add duplicate signature error if key seen
end
alt level >= Standard and USE_ROSLYN
ManageScript->>ManageScript: ValidateScriptSyntaxRoslyn(contents, level, errorList)
end
ManageScript-->>Developer: validation result
end
Class diagram for new duplicate signature validation helpersclassDiagram
class ManageScript {
+ValidateScriptSyntax(string contents, ValidationLevel level, List~string~ errorList)
+ValidateScriptSyntaxRoslyn(string contents, ValidationLevel level, List~string~ errorList)
+ValidateScriptSyntaxUnity(string contents, List~string~ errors)
-CheckDuplicateMethodSignatures(string contents, List~string~ errors)
-CountTopLevelParams(string paramStr) int
-ExtractParamTypes(string paramStr) string
}
class CSharpLexer {
+CSharpLexer(string source)
+Position int
+InNonCode bool
+Advance(Token token) bool
}
class Regex {
+Regex(string pattern, RegexOptions options, TimeSpan timeout)
+Matches(string input) MatchCollection
}
class Match {
+Index int
+Groups GroupCollection
}
class GroupCollection {
+Item[int index] Group
}
class Group {
+Value string
}
class Dictionary_StringInt {
+Dictionary_StringInt(StringComparer comparer)
+TryGetValue(string key, int value) bool
+Item[string key] int
}
ManageScript ..> CSharpLexer : uses
ManageScript ..> Regex : uses
ManageScript ..> Match : uses
ManageScript ..> Dictionary_StringInt : uses as seen
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughAdds duplicate method-signature detection to the script validation flow by introducing parsing helpers for parameter lists and invoking the checker during basic (non-Roslyn) validation; tests covering multiple duplicate and non-duplicate scenarios were added, and a small docstring edit occurred in a server script. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
CheckDuplicateMethodSignatures, consider makingmethodSigPatterna static, precompiledRegex(e.g.,static readonly Regex) so it isn’t re-instantiated on every validation call, which can be relatively expensive in large projects. - The
ExtractParamTypesloop currently uses a sentinel comma ati == paramStr.Lengthto handle the final parameter; refactoring to explicitly process the final segment (instead of relying on that sentinel) would make the logic easier to follow and less error-prone for future maintenance.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `CheckDuplicateMethodSignatures`, consider making `methodSigPattern` a static, precompiled `Regex` (e.g., `static readonly Regex`) so it isn’t re-instantiated on every validation call, which can be relatively expensive in large projects.
- The `ExtractParamTypes` loop currently uses a sentinel comma at `i == paramStr.Length` to handle the final parameter; refactoring to explicitly process the final segment (instead of relying on that sentinel) would make the logic easier to follow and less error-prone for future maintenance.
## Individual Comments
### Comment 1
<location path="MCPForUnity/Editor/Tools/ManageScript.cs" line_range="2678-2680" />
<code_context>
+ }
+
+ // Step 3: Match method signatures on code-only text (includes => for expression-bodied)
+ var methodSigPattern = new Regex(
+ @"(?:public|private|protected|internal)(?:\s+(?:static|virtual|override|abstract|sealed|async|new))*\s+\S+\s+(\w+)\s*\(([^)]*)\)\s*(?:where\s+\S+\s*:\s*\S+\s*)?(?:[{;]|=>)",
+ RegexOptions.Multiline | RegexOptions.CultureInvariant, TimeSpan.FromSeconds(2));
+ var sigMatches = methodSigPattern.Matches(codeOnly);
+ var seen = new System.Collections.Generic.Dictionary<string, int>(System.StringComparer.Ordinal);
</code_context>
<issue_to_address>
**issue (bug_risk):** Using only brace depth to distinguish scopes can produce false positives across different types/namespaces.
The deduplication key currently scopes only by `@{depth}`, so two methods with the same signature in different classes/namespaces at the same brace depth will be treated as duplicates (e.g., `class A { void Foo(int x) { } }` and `class B { void Foo(int x) { } }`). To avoid these false positives, consider enriching the key with surrounding type/namespace context (e.g., a stack of `class/struct/interface` names) or by including the full signature plus a nearby `namespace`/`class` identifier.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a structural validation guard to manage_script edits to detect duplicate method signatures (aimed at preventing anchor_replace-style corruption as described in issue #898), along with new in-memory validation tests.
Changes:
- Run a new duplicate-method-signature scan during
ValidateScriptSyntaxto reject corrupted outputs early. - Implement helpers to normalize parameter type extraction and brace-depth-based scoping for signature comparison.
- Add new NUnit tests intended to verify duplicate detection behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptValidationTests.cs | Adds reflection-based tests for duplicate method detection. |
| MCPForUnity/Editor/Tools/ManageScript.cs | Adds duplicate method signature detection to core script validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+142
to
+150
| /// Calls ValidateScriptSyntaxUnity via reflection and returns the error list. | ||
| /// </summary> | ||
| private List<string> CallValidateScriptSyntaxUnity(string contents) | ||
| { | ||
| var errors = new List<string>(); | ||
| var method = typeof(ManageScript).GetMethod("ValidateScriptSyntaxUnity", | ||
| BindingFlags.NonPublic | BindingFlags.Static); | ||
| Assert.IsNotNull(method, "ValidateScriptSyntaxUnity method must exist"); | ||
| method.Invoke(null, new object[] { contents, errors }); |
There was a problem hiding this comment.
These new duplicate-signature tests call ValidateScriptSyntaxUnity via reflection, but the duplicate signature check is implemented in ValidateScriptSyntax (it calls CheckDuplicateMethodSignatures) and ValidateScriptSyntaxUnity does not invoke that check. As a result, the "*_Flagged" tests will not exercise the new behavior and will fail/produce false confidence. Update the helper to reflectively call ValidateScriptSyntax(contents, ValidationLevel.Basic, out string[] errors) (or call CheckDuplicateMethodSignatures directly) and assert against that error output instead.
Suggested change
| /// Calls ValidateScriptSyntaxUnity via reflection and returns the error list. | |
| /// </summary> | |
| private List<string> CallValidateScriptSyntaxUnity(string contents) | |
| { | |
| var errors = new List<string>(); | |
| var method = typeof(ManageScript).GetMethod("ValidateScriptSyntaxUnity", | |
| BindingFlags.NonPublic | BindingFlags.Static); | |
| Assert.IsNotNull(method, "ValidateScriptSyntaxUnity method must exist"); | |
| method.Invoke(null, new object[] { contents, errors }); | |
| /// Calls core ValidateScriptSyntax via reflection (ValidationLevel.Basic) and returns the error list. | |
| /// </summary> | |
| private List<string> CallValidateScriptSyntaxUnity(string contents) | |
| { | |
| var method = typeof(ManageScript).GetMethod("ValidateScriptSyntax", | |
| BindingFlags.NonPublic | BindingFlags.Static); | |
| Assert.IsNotNull(method, "ValidateScriptSyntax method must exist"); | |
| var parameters = method.GetParameters(); | |
| Assert.AreEqual(3, parameters.Length, "ValidateScriptSyntax signature has changed"); | |
| var validationLevelType = parameters[1].ParameterType; | |
| object validationLevelBasic; | |
| if (validationLevelType.IsEnum) | |
| { | |
| validationLevelBasic = Enum.Parse(validationLevelType, "Basic"); | |
| } | |
| else | |
| { | |
| // Fallback in case the parameter type changes from an enum. | |
| validationLevelBasic = Activator.CreateInstance(validationLevelType); | |
| } | |
| object[] args = new object[] { contents, validationLevelBasic, null }; | |
| method.Invoke(null, args); | |
| var errorsArray = args[2] as string[]; | |
| var errors = errorsArray != null ? new List<string>(errorsArray) : new List<string>(); |
Comment on lines
+2682
to
+2691
| var seen = new System.Collections.Generic.Dictionary<string, int>(System.StringComparer.Ordinal); | ||
| foreach (Match sm in sigMatches) | ||
| { | ||
| string methodName = sm.Groups[1].Value; | ||
| int paramCount = CountTopLevelParams(sm.Groups[2].Value); | ||
| string paramTypes = ExtractParamTypes(sm.Groups[2].Value); | ||
| int depth = depthArr[sm.Index]; | ||
| string key = $"{methodName}/{paramCount}/{paramTypes}@{depth}"; | ||
| if (seen.TryGetValue(key, out _)) | ||
| errors.Add($"ERROR: Duplicate method signature detected: '{methodName}' with {paramCount} parameter(s). This may indicate a corrupted edit."); |
There was a problem hiding this comment.
The duplicate-method signature key uses only brace depth (@{depth}) to represent scope. This will incorrectly flag identical method signatures that exist in two different sibling types/namespaces at the same nesting depth (e.g., class A { public void Foo(int x){} } class B { public void Foo(int x){} } in the same file). Consider including the containing type (and namespace) in the key by tracking a type stack while scanning braces, rather than using depth alone.
Suggested change
| var seen = new System.Collections.Generic.Dictionary<string, int>(System.StringComparer.Ordinal); | |
| foreach (Match sm in sigMatches) | |
| { | |
| string methodName = sm.Groups[1].Value; | |
| int paramCount = CountTopLevelParams(sm.Groups[2].Value); | |
| string paramTypes = ExtractParamTypes(sm.Groups[2].Value); | |
| int depth = depthArr[sm.Index]; | |
| string key = $"{methodName}/{paramCount}/{paramTypes}@{depth}"; | |
| if (seen.TryGetValue(key, out _)) | |
| errors.Add($"ERROR: Duplicate method signature detected: '{methodName}' with {paramCount} parameter(s). This may indicate a corrupted edit."); | |
| // Track enclosing scope (namespace/type) so identical signatures in different sibling scopes are not conflated | |
| var scopePattern = new Regex( | |
| @"\b(?:namespace|class|struct|interface|record)\s+([A-Za-z_][A-Za-z0-9_\.]*)\b", | |
| RegexOptions.Multiline | RegexOptions.CultureInvariant); | |
| var scopeMatches = scopePattern.Matches(codeOnly); | |
| int currentScopeIndex = -1; | |
| var seen = new System.Collections.Generic.Dictionary<string, int>(System.StringComparer.Ordinal); | |
| foreach (Match sm in sigMatches) | |
| { | |
| // Advance scope index to the last scope declaration starting at or before this method | |
| while (currentScopeIndex + 1 < scopeMatches.Count && | |
| scopeMatches[currentScopeIndex + 1].Index <= sm.Index) | |
| { | |
| currentScopeIndex++; | |
| } | |
| string scopeName = currentScopeIndex >= 0 | |
| ? scopeMatches[currentScopeIndex].Groups[1].Value | |
| : "global"; | |
| string methodName = sm.Groups[1].Value; | |
| int paramCount = CountTopLevelParams(sm.Groups[2].Value); | |
| string paramTypes = ExtractParamTypes(sm.Groups[2].Value); | |
| string key = $"{scopeName}.{methodName}/{paramCount}/{paramTypes}"; | |
| if (seen.TryGetValue(key, out _)) | |
| errors.Add($"ERROR: Duplicate method signature detected: '{methodName}' with {paramCount} parameter(s) in scope '{scopeName}'. This may indicate a corrupted edit."); |
|
|
||
| // Step 3: Match method signatures on code-only text (includes => for expression-bodied) | ||
| var methodSigPattern = new Regex( | ||
| @"(?:public|private|protected|internal)(?:\s+(?:static|virtual|override|abstract|sealed|async|new))*\s+\S+\s+(\w+)\s*\(([^)]*)\)\s*(?:where\s+\S+\s*:\s*\S+\s*)?(?:[{;]|=>)", |
There was a problem hiding this comment.
methodSigPattern uses \S+ for the return type. This fails to match common generic return types that contain spaces (e.g. Dictionary<string, int> Get()), so duplicates in those methods won’t be detected. Consider loosening the return-type part of the regex (or tokenizing types) so generic types with whitespace still match.
Suggested change
| @"(?:public|private|protected|internal)(?:\s+(?:static|virtual|override|abstract|sealed|async|new))*\s+\S+\s+(\w+)\s*\(([^)]*)\)\s*(?:where\s+\S+\s*:\s*\S+\s*)?(?:[{;]|=>)", | |
| @"(?:public|private|protected|internal)(?:\s+(?:static|virtual|override|abstract|sealed|async|new))*\s+(?:[^{(;=>]+?)\s+(\w+)\s*\(([^)]*)\)\s*(?:where\s+\S+\s*:\s*\S+\s*)?(?:[{;]|=>)", |
Comment on lines
+2554
to
+2569
| string param = paramStr.Substring(start, i - start).Trim(); | ||
| if (types.Length > 0) types.Append(", "); | ||
| // The type is everything except the last token (the name). | ||
| // But if the last token ends with '>' or ']', it's all type (e.g. "List<int>"). | ||
| // Find the last whitespace that is NOT inside <> brackets. | ||
| int lastSplit = -1; | ||
| int d2 = 0; | ||
| for (int j = 0; j < param.Length; j++) | ||
| { | ||
| char pc = param[j]; | ||
| if (pc == '<' || pc == '(' || pc == '[') d2++; | ||
| else if (pc == '>' || pc == ')' || pc == ']') d2--; | ||
| else if (d2 == 0 && char.IsWhiteSpace(pc)) lastSplit = j; | ||
| } | ||
| types.Append(lastSplit > 0 ? param.Substring(0, lastSplit).Trim() : param); | ||
| start = i + 1; |
There was a problem hiding this comment.
ExtractParamTypes determines the “type part” by stripping everything after the last whitespace outside generics. This breaks for parameters with default values (e.g. int x = 1), where it will keep int x = as the “type”. That can cause false negatives when comparing Foo(int x) vs Foo(int x = 1) (same signature in C#, but keys won’t match). Consider explicitly removing default value assignments and parameter names/modifiers via a more robust parse (or at least truncate at '=' at top level before the whitespace split).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MCPForUnity/Editor/Tools/ManageScript.cs`:
- Around line 2688-2693: The current duplicate-key uses depthArr[sm.Index]
(brace depth) so the key string
$"{methodName}/{paramCount}/{paramTypes}@{depth}" can collide across different
types; change the key to include the enclosing type identity instead of depth —
obtain the containing type name for the symbol represented by sm (e.g., a
full/qualified type name or namespace+type via the AST/symbol API you already
have access to) and build the key like
$"{containingTypeFullName}/{methodName}/{paramCount}/{paramTypes}" (replace the
use of depthArr[sm.Index] and ensure seen is keyed by that containingType-based
string before calling seen.TryGetValue or seen[...]=1).
- Around line 2678-2680: The regex assigned to methodSigPattern currently
requires an explicit accessibility modifier, so update the pattern used in the
methodSigPattern variable to make the leading accessibility token optional
(e.g., change the "(?:public|private|protected|internal)" group to an optional
group) and allow optional whitespace after it; keep the rest of the pattern
(modifiers, return type, name capture group (\w+) and parameter capture group
([^)]*)) and retain the same RegexOptions and timeout settings so methods
declared without an explicit modifier (like "void Start()") are matched.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptValidationTests.cs`:
- Around line 144-151: The test helper CallValidateScriptSyntaxUnity is invoking
the wrong entrypoint (ValidateScriptSyntaxUnity) so duplicate-signature checks
added to ValidateScriptSyntax are never exercised; update the helper to
reflectively call ManageScript.ValidateScriptSyntax (not
ValidateScriptSyntaxUnity), keeping the same BindingFlags/Invoke pattern and the
same parameters (contents, errors) so tests that assert
CheckDuplicateMethodSignatures behavior exercise the new logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6cface6-8b46-45ab-85bb-81cdd6144a55
📒 Files selected for processing (2)
MCPForUnity/Editor/Tools/ManageScript.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptValidationTests.cs
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptValidationTests.cs
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptValidationTests.cs (1)
145-161: Previous issue addressed, but method name is now misleading.The method correctly calls
ValidateScriptSyntax(line 152), which exercisesCheckDuplicateMethodSignatures. However, the method nameCallValidateScriptSyntaxUnityis confusing since it doesn't callValidateScriptSyntaxUnityat all.📝 Consider renaming for clarity
- private List<string> CallValidateScriptSyntaxUnity(string contents) + private List<string> CallValidateScriptSyntax(string contents)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptValidationTests.cs` around lines 145 - 161, Rename the misleading helper method CallValidateScriptSyntaxUnity to a clear name like CallValidateScriptSyntax (or CallManageScriptValidateScriptSyntax) because it invokes ManageScript.ValidateScriptSyntax and not ValidateScriptSyntaxUnity; update the method declaration and all its call sites in tests to the new name and ensure the signature and behavior remain unchanged (references: method CallValidateScriptSyntaxUnity, ManageScript.ValidateScriptSyntax, CheckDuplicateMethodSignatures).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptValidationTests.cs`:
- Around line 145-161: Rename the misleading helper method
CallValidateScriptSyntaxUnity to a clear name like CallValidateScriptSyntax (or
CallManageScriptValidateScriptSyntax) because it invokes
ManageScript.ValidateScriptSyntax and not ValidateScriptSyntaxUnity; update the
method declaration and all its call sites in tests to the new name and ensure
the signature and behavior remain unchanged (references: method
CallValidateScriptSyntaxUnity, ManageScript.ValidateScriptSyntax,
CheckDuplicateMethodSignatures).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05855218-9bfe-4dcd-a4d4-12fa22cdf218
📒 Files selected for processing (3)
MCPForUnity/Editor/Tools/ManageScript.csServer/src/services/tools/script_apply_edits.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptValidationTests.cs
✅ Files skipped from review due to trivial changes (1)
- Server/src/services/tools/script_apply_edits.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a duplicate signature check that should fix issue #898, with additional test file.
Summary by Sourcery
Add basic structural validation to detect duplicate method signatures in Unity scripts and cover it with regression tests.
Bug Fixes:
Tests:
Summary by CodeRabbit