fix: Settings no longer shows an unnecessary third-party tools permission#1020
Conversation
Third-party tool access no longer needs a separate permission toggle, so remove the editor control and normalize persisted settings to keep custom tools available. Update docs and tests to match the always-on behavior.
Use the concise CLI prefix in the settings window so the configuration status row matches the requested wording.
Third-party tools are now always available, so remove the leftover UI state that could mark that group as disabled. This keeps the settings list behavior aligned with the always-on permission model.
Third-party tools no longer have a separate permission gate, so remove the persisted setting, security checker branch, and related docs. Keep only legacy JSON cleanup so old allowThirdPartyTools entries are stripped when settings are rewritten.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRemoves the "Allow Third Party Tools" feature and all related code paths, including settings accessors, UI controls, security restrictions, and view data structures. Custom tools remain always available; visibility is now controlled exclusively through per-tool toggles. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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. Review rate limit: 0/1 reviews remaining, refill in 44 minutes and 48 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/src/Editor/Config/ULoopSettings.cs (1)
164-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the legacy tool-toggle fields in the precedence probe.
LegacyFileHasSecurityFields()now ignoresenableTestsExecutionandallowMenuItemExecution, butMigrateFromLegacySettings()still relies on that same legacy JSON to migrate those values into per-tool toggles. On direct upgrades whereUserSettings/UnityMcpSettings.jsonstill has those fields and.uloop/settings.security.jsonexists with stale defaults, this change lets the stale.uloopfile win and drops the user's oldrun-tests/ menu-item disablement.Proposed fix
private static bool LegacyFileHasSecurityFields() { if (!File.Exists(LegacySettingsFilePath)) { return false; } string json = File.ReadAllText(LegacySettingsFilePath); return json.Contains($"\"{LEGACY_ALLOW_THIRD_PARTY_TOOLS_FIELD}\"") + || json.Contains($"\"{nameof(LegacySecuritySettingsProbe.enableTestsExecution)}\"") + || json.Contains($"\"{nameof(LegacySecuritySettingsProbe.allowMenuItemExecution)}\"") || json.Contains($"\"{nameof(LegacySecuritySettingsProbe.dynamicCodeSecurityLevel)}\""); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Packages/src/Editor/Config/ULoopSettings.cs` around lines 164 - 173, LegacyFileHasSecurityFields() currently only checks for LEGACY_ALLOW_THIRD_PARTY_TOOLS_FIELD and LegacySecuritySettingsProbe.dynamicCodeSecurityLevel, which causes MigrateFromLegacySettings() to miss legacy enableTestsExecution and allowMenuItemExecution toggles; update LegacyFileHasSecurityFields() to also detect the legacy JSON keys for enableTestsExecution and allowMenuItemExecution (e.g. check json.Contains($"\"enableTestsExecution\"") and json.Contains($"\"allowMenuItemExecution\"") or use the actual constant/nameof values if present), so MigrateFromLegacySettings() can still read and migrate those per-tool settings using LegacySettingsFilePath and the existing migration logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Packages/src/Editor/Config/ULoopSettings.cs`:
- Around line 164-173: LegacyFileHasSecurityFields() currently only checks for
LEGACY_ALLOW_THIRD_PARTY_TOOLS_FIELD and
LegacySecuritySettingsProbe.dynamicCodeSecurityLevel, which causes
MigrateFromLegacySettings() to miss legacy enableTestsExecution and
allowMenuItemExecution toggles; update LegacyFileHasSecurityFields() to also
detect the legacy JSON keys for enableTestsExecution and allowMenuItemExecution
(e.g. check json.Contains($"\"enableTestsExecution\"") and
json.Contains($"\"allowMenuItemExecution\"") or use the actual constant/nameof
values if present), so MigrateFromLegacySettings() can still read and migrate
those per-tool settings using LegacySettingsFilePath and the existing migration
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a8282ced-b3a3-4938-a222-4fbc2064b637
📒 Files selected for processing (18)
Assets/Tests/Editor/McpEditorWindowRefreshPolicyTests.csAssets/Tests/Editor/ToolSettingsSectionTests.csAssets/Tests/Editor/ULoopSettingsTests.csPackages/src/Editor/Config/ULoopSettings.csPackages/src/Editor/Config/ULoopSettingsData.csPackages/src/Editor/Security/McpSecurityChecker.csPackages/src/Editor/UI/McpEditorModel.csPackages/src/Editor/UI/McpEditorWindow.csPackages/src/Editor/UI/McpEditorWindowViewData.csPackages/src/Editor/UI/UIToolkit/Components/CliSetupSection.csPackages/src/Editor/UI/UIToolkit/Components/ToolSettingsSection.csPackages/src/Editor/UI/UIToolkit/McpEditorWindow.ussPackages/src/Editor/UI/UIToolkit/McpEditorWindow.uxmlPackages/src/Editor/UI/UIToolkit/McpEditorWindowUI.csPackages/src/README.mdPackages/src/README_ja.mdREADME.mdREADME_ja.md
💤 Files with no reviewable changes (8)
- Assets/Tests/Editor/McpEditorWindowRefreshPolicyTests.cs
- Packages/src/Editor/Config/ULoopSettingsData.cs
- Packages/src/Editor/UI/UIToolkit/McpEditorWindow.uss
- Packages/src/Editor/UI/UIToolkit/McpEditorWindowUI.cs
- Packages/src/Editor/UI/McpEditorWindowViewData.cs
- Assets/Tests/Editor/ToolSettingsSectionTests.cs
- Packages/src/Editor/UI/McpEditorModel.cs
- Packages/src/Editor/UI/McpEditorWindow.cs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Keep legacy run-tests and menu-item fields in the migration trigger so direct upgrades do not let stale extracted security defaults override the user's old tool-toggle settings.
|
Addressed the CodeRabbit outside-diff finding in 30b3d6d. LegacyFileHasSecurityFields now treats enableTestsExecution and allowMenuItemExecution as migration triggers, and the regression test covers stale settings.security.json plus a legacy run-tests disablement. |
1 participant
Summary
User Impact
CLIlabel instead ofuloop-cli, making the Configuration section easier to scan.Changes
allowThirdPartyToolsentries are removed when settings are rewritten.Verification
uloop compile --project-path /Users/a12115/ghq/hatayama/unity-cli-loopuloop run-tests --project-path /Users/a12115/ghq/hatayama/unity-cli-loop --test-mode EditMode --filter-type regex --filter-value "io\.github\.hatayama\.uLoopMCP\.(ULoopSettingsTests|ToolSettingsSectionTests)"uloop run-tests --project-path /Users/a12115/ghq/hatayama/unity-cli-loop --test-mode EditMode --filter-type regex --filter-value "io\.github\.hatayama\.uLoopMCP\.(ToolSettingsSectionTests|McpEditorWindowRefreshPolicyTests|ULoopSettingsTests)"uloop run-tests --project-path /Users/a12115/ghq/hatayama/unity-cli-loop --test-mode EditMode --filter-type regex --filter-value "io\.github\.hatayama\.uLoopMCP\.(ULoopSettingsTests|ToolSettingsSectionTests|McpEditorWindowRefreshPolicyTests)"Overview
This PR removes the obsolete "Allow Third Party Tools" permission system from the settings and UI, transitioning custom tools to always-on availability by default. The change simplifies the settings interface while retaining per-tool visibility controls through individual tool toggles. Additionally, the CLI status label was shortened from "uloop-cli" to "CLI" for improved UI readability.
Key Changes
Settings Architecture
allowThirdPartyToolsfromULoopSettingsDatarecordGetAllowThirdPartyTools()andSetAllowThirdPartyTools()fromULoopSettingsLoadSettings()to detect and purge legacyallowThirdPartyToolsentries from persisted JSON, forcing re-save when detectedSecurity & Access Control
McpSecurityChecker.IsToolAllowedByAttribute()now always allows third-party tools instead of conditionally gating themGetAllowThirdPartyTools()User Interface Removal
OnAllowThirdPartyChangedevent subscription andUpdateAllowThirdPartyTools()method fromMcpEditorWindowAllowThirdPartyToolsproperty and constructor parameter fromToolSettingsSectionDataToolSettingsSectionOnAllowThirdPartyChangedevent fromMcpEditorWindowUI.mcp-tool-list-row--disabledCSS class that previously dimmed restricted toolsMcpEditorWindow.uxmltemplateUI Polish
"uloop-cli:"to"CLI:"in three CLI status states (checking, installed with version, not installed)Test Updates
allowThirdPartyToolsinitialization from test fixture helpers acrossMcpEditorWindowRefreshPolicyTests,ToolSettingsSectionTests, andULoopSettingsTestsDocumentation
README_ja.mdandPackages/src/README_ja.mdto reflect always-on behavior and removed permission requirement guidance.uloop/settings.permissions.jsondocumentation to describe only dynamic code execution security levelImpact