Skip to content

Fix create_script validator false-positive inside method bodies#1076

Merged
Scriptwonder merged 1 commit intoCoplayDev:betafrom
justinpbarnett:fix/manage-script-local-function-duplicates
Apr 26, 2026
Merged

Fix create_script validator false-positive inside method bodies#1076
Scriptwonder merged 1 commit intoCoplayDev:betafrom
justinpbarnett:fix/manage-script-local-function-duplicates

Conversation

@justinpbarnett
Copy link
Copy Markdown
Contributor

@justinpbarnett justinpbarnett commented Apr 25, 2026

Summary

  • Fix ManageScript duplicate-method validation so it only counts method declarations that are direct members of a type.
  • Prevent repeated method calls such as return Compute(); from being misclassified as duplicate method declarations.
  • Add regression coverage for repeated invocations and same-signature local functions in separate method bodies.

Test plan

  • Reproduced on upstream/beta with a temporary Unity -executeMethod harness: repeated return Compute(); calls produced duplicate Compute diagnostics.
  • Verified the same harness passes on this branch with REPRO_OK.
  • git diff --check

Notes

Summary by CodeRabbit

  • Bug Fixes

    • Improved duplicate method signature detection to more accurately identify actual duplicate declarations while avoiding false positives from local functions in different scopes and method invocations within method bodies.
  • Tests

    • Added test coverage for edge cases in duplicate method signature detection.

The duplicate-method validator was still matching invocation-like text inside method bodies after the constructor-call fix in CoplayDev#1045. Tracking both the current brace depth and the direct member depth for the containing type keeps duplicate detection scoped to actual type members while preserving the existing lightweight parser.

Constraint: The existing validator is regex-based and runs even without Roslyn available

Rejected: Add more fake return-type keyword skips | that would miss local functions and keep the parser fragile

Confidence: high

Scope-risk: narrow

Directive: Keep duplicate-method matching scoped to direct type members unless replacing this validator with a real syntax walk

Tested: Unity -executeMethod repro failed on upstream/beta and passed on this branch with REPRO_OK

Tested: git diff --check

Not-tested: Full Unity 2021.3 EditMode suite locally; only Unity 6000 is installed in this environment and did not emit runner XML for the 2021.3-pinned project
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 25, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 62b05615-b5f8-4986-a823-8d46b6d5c611

📥 Commits

Reviewing files that changed from the base of the PR and between 48169fc and 0c76786.

📒 Files selected for processing (2)
  • MCPForUnity/Editor/Tools/ManageScript.cs
  • TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptValidationTests.cs

📝 Walkthrough

Walkthrough

The duplicate-method-signature detection algorithm in ManageScript.cs is enhanced with brace-depth filtering to prevent false positives. Method signatures are now validated only when they match the containing type's member scope depth. Two test cases expand coverage to validate local functions and method invocations.

Changes

Cohort / File(s) Summary
Duplicate Method Detection Refinement
MCPForUnity/Editor/Tools/ManageScript.cs
Added per-index brace-depth tracking during containing-type member scan. Filters reported duplicates to require non-empty containing type and matching brace depth between detection location and containing-type member scope.
Test Coverage Expansion
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptValidationTests.cs
New test cases verify that same-signature local functions in different methods are valid C# (no error) and that repeated method invocations within bodies don't trigger duplicate-member errors.

Possibly related PRs

Poem

A rabbit hops through braces deep,
Now tracking depths where methods creep,
Local functions live their lives so free,
No false alarms—just harmony! 🐰✨

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: fixing a false-positive in the create_script validator that was triggered by method bodies.
Description check ✅ Passed The description covers the key sections with substantial content: summary of changes, test plan with validation steps, and notes about related issues and documentation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Scriptwonder
Copy link
Copy Markdown
Collaborator

Great to see you Justin! Thanks for the fix :)

@Scriptwonder Scriptwonder merged commit 74ad91c into CoplayDev:beta Apr 26, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants