Guard manage_asset search against broad scans#1185
Open
liuzqk wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR hardens ManageAsset search by introducing validation at both server and editor levels to prevent malformed requests, normalizes folder-scope detection to correctly interpret search criteria, and optimizes result fetching to defer asset data loading until after pagination applies, avoiding excessive processing when projects contain many assets. ChangesSearch Validation and Performance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
liuzqk
force-pushed
the
fix/manage-asset-search-guard
branch
from
f7e5c08 to
0f5a81e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
MCPForUnity/Editor/Tools/ManageAsset.cs (1)
697-728: 💤 Low valuePerformance optimization successfully defers asset data loading.
The search pipeline now collects matching paths first, applies pagination to the path list, and only calls
GetAssetData()for the paginated subset. This prevents materializing full asset metadata for all matches when only a page is needed.Minor edge case: If an asset is deleted between
FindAssetsandGetAssetData, the results list may contain a null entry (line 1050 returns null for non-existent assets). This is a rare race condition in practice, and the JSON response would just include a null entry that clients should handle gracefully.Optional: Filter out null entries after GetAssetData
If you want to ensure no null entries in the response:
- var pagedResults = matchingPaths - .Skip(startIndex) - .Take(pageSize) - .Select(assetPath => GetAssetData(assetPath, generatePreview)) - .ToList(); + var pagedResults = matchingPaths + .Skip(startIndex) + .Take(pageSize) + .Select(assetPath => GetAssetData(assetPath, generatePreview)) + .Where(data => data != null) + .ToList();Note: This would cause
pagedResults.Countto potentially be less thanpageSizeif some assets were deleted.🤖 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/ManageAsset.cs` around lines 697 - 728, The pagedResults list can contain nulls if assets are deleted between AssetDatabase.FindAssets and GetAssetData; after creating pagedResults via matchingPaths.Skip(...).Take(...).Select(...).ToList(), filter out null entries (e.g., remove or Where(x => x != null)) and adjust total/result counts accordingly so callers don't receive null entries; update references to GetAssetData, matchingPaths and pagedResults in ManageAsset.cs to perform this null-filtering and ensure pagination/counts reflect the filtered results.Server/tests/integration/test_manage_asset_search_guard.py (2)
37-46: ⚡ Quick winTest validates rejection but doesn't verify the semantic constraint.
The test correctly confirms that a date-only search without scope or type filter is rejected. However, the assertion on line 45 only checks that
"filter_type"appears in the generic error message. Consider adding a positive test case that verifies addingfilter_typemakes the date search valid, which would more clearly document the intended behavior that date filters should be combined with type filters or folder scopes.🧪 Suggested enhancement
Add a positive test after this one:
`@pytest.mark.asyncio` async def test_search_date_filter_with_type_succeeds(monkeypatch): captured = {} async def fake_send(send_fn, unity_instance, command_type, params, **kwargs): captured["params"] = params return {"success": True, "data": {"assets": []}} monkeypatch.setattr( "services.tools.manage_asset.send_with_unity_instance", fake_send, ) result = await manage_asset( ctx=DummyContext(), action="search", path="Assets", filter_type="Prefab", filter_date_after="2026-01-01T00:00:00Z", ) assert result["success"] is True assert captured["params"]["filterDateAfter"] == "2026-01-01T00:00:00Z" assert captured["params"]["filterType"] == "Prefab"🤖 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 `@Server/tests/integration/test_manage_asset_search_guard.py` around lines 37 - 46, The test ensures a date-only search is rejected but doesn't verify the complementary positive behavior; add a new async test (e.g., test_search_date_filter_with_type_succeeds) that monkeypatches services.tools.manage_asset.send_with_unity_instance to capture params, calls manage_asset(ctx=DummyContext(), action="search", path="Assets", filter_type="Prefab", filter_date_after="2026-01-01T00:00:00Z"), asserts result["success"] is True, and verifies the captured params include filterDateAfter == "2026-01-01T00:00:00Z" and filterType == "Prefab" so the semantic constraint (date filter must be combined with type or scope) is validated.
23-24: 💤 Low valueConsider a more specific assertion.
The assertion only checks for
"search_pattern"in the error message, but the actual message contains both"search_pattern"and"filter_type". A more robust assertion would verify the complete intent (e.g., check for"requires"and both search criteria, or verify the full message structure) to prevent false positives if the error message changes.📋 Suggested refinement
assert result["success"] is False - assert "search_pattern" in result["message"] + assert "requires" in result["message"] + assert "search_pattern" in result["message"] or "filter" in result["message"]🤖 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 `@Server/tests/integration/test_manage_asset_search_guard.py` around lines 23 - 24, Update the test assertion to be more specific: instead of only checking that "search_pattern" appears in result["message"], assert that the message mentions both required fields (e.g., contains "search_pattern" and "filter_type") and the intent (for example contains the word "requires" or match the full expected error string). Locate the assertions in the test (the two lines asserting result["success"] is False and checking result["message"]) and replace the loose substring check with either two substring assertions for "search_pattern" and "filter_type" or a single equality/assertion against the full expected error message to ensure the test fails if the message changes.
🤖 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.
Nitpick comments:
In `@MCPForUnity/Editor/Tools/ManageAsset.cs`:
- Around line 697-728: The pagedResults list can contain nulls if assets are
deleted between AssetDatabase.FindAssets and GetAssetData; after creating
pagedResults via matchingPaths.Skip(...).Take(...).Select(...).ToList(), filter
out null entries (e.g., remove or Where(x => x != null)) and adjust total/result
counts accordingly so callers don't receive null entries; update references to
GetAssetData, matchingPaths and pagedResults in ManageAsset.cs to perform this
null-filtering and ensure pagination/counts reflect the filtered results.
In `@Server/tests/integration/test_manage_asset_search_guard.py`:
- Around line 37-46: The test ensures a date-only search is rejected but doesn't
verify the complementary positive behavior; add a new async test (e.g.,
test_search_date_filter_with_type_succeeds) that monkeypatches
services.tools.manage_asset.send_with_unity_instance to capture params, calls
manage_asset(ctx=DummyContext(), action="search", path="Assets",
filter_type="Prefab", filter_date_after="2026-01-01T00:00:00Z"), asserts
result["success"] is True, and verifies the captured params include
filterDateAfter == "2026-01-01T00:00:00Z" and filterType == "Prefab" so the
semantic constraint (date filter must be combined with type or scope) is
validated.
- Around line 23-24: Update the test assertion to be more specific: instead of
only checking that "search_pattern" appears in result["message"], assert that
the message mentions both required fields (e.g., contains "search_pattern" and
"filter_type") and the intent (for example contains the word "requires" or match
the full expected error string). Locate the assertions in the test (the two
lines asserting result["success"] is False and checking result["message"]) and
replace the loose substring check with either two substring assertions for
"search_pattern" and "filter_type" or a single equality/assertion against the
full expected error message to ensure the test fails if the message changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 503abe1b-df5a-4831-9369-9dc77f3963ae
📒 Files selected for processing (5)
MCPForUnity/Editor/Tools/ManageAsset.csServer/src/services/tools/manage_asset.pyServer/tests/integration/test_manage_asset_search_guard.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageAssetSearchGuardTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageAssetSearchGuardTests.cs.meta
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
1 participant
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.
Summary
Fixes #1177.
manage_asset(action="search")calls that put the query inpathGetAssetDataso search results do not materialize full asset metadata before slicingWhy
An empty or invalid search scope can call
AssetDatabase.FindAssets("")and then process metadata for the entire project. Large Unity projects can stall whileGetAssetDataruns over every asset.Validation
uv run --extra dev python -m pytest tests/integration/test_manage_asset_search_guard.py tests/integration/test_manage_asset_json_parsing.py -qpython -m py_compile Server/src/services/tools/manage_asset.pygit diff --checkI also attempted the new Unity EditMode test locally, but this checkout's
UnityMCPTestsproject is pinned to Unity 2021.3.45f2 and the available local editor was 2022.3.62f3, which only triggered package/project upgrade work and did not produce a test result. Those generated changes were discarded.Summary by CodeRabbit
New Features
Bug Fixes
Performance