Repository navigation
fix: Invalid tool parameters now return a readable error result instead of an RPC failure - #1522
Merged
Merged
Conversation
User-input validation errors (out-of-range action enums, invalid pause-point ids and timeouts, unsupported test filter types) were thrown as exceptions, reaching the CLI as generic JSON-RPC errors on stderr with exit code 1. Convert them to Success=false responses so they arrive as normal tool results the caller can read. Contract guards for null parameters stay as exceptions.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughSeveral editor tool use cases now return validation-style failure responses for invalid or unsupported input instead of throwing exceptions, and the affected tests were updated or added to assert the new behavior. ChangesValidation failure refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant RunTestsUseCase
participant TestFilterCreationService
participant RunTestsUseCaseTests
RunTestsUseCase->>TestFilterCreationService: TryCreateFilter(filterType, filterValue)
TestFilterCreationService-->>RunTestsUseCase: (filter, errorMessage)
alt errorMessage present
RunTestsUseCase-->>RunTestsUseCaseTests: Response(Success=false, Message)
else filter created
RunTestsUseCase-->>RunTestsUseCaseTests: Continue execution with filter
end
sequenceDiagram
participant SimulateMouseUiActionValidationTests
participant SimulateMouseUiUseCase
participant MouseUiSimulationCommand
SimulateMouseUiActionValidationTests->>SimulateMouseUiUseCase: ExecuteAsync(invalid action)
SimulateMouseUiUseCase->>MouseUiSimulationCommand: TryFromSchema(schema)
MouseUiSimulationCommand-->>SimulateMouseUiUseCase: (null, errorMessage)
SimulateMouseUiUseCase-->>SimulateMouseUiActionValidationTests: Response(Success=false, Action)
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Packages/src/Editor/FirstPartyTools/SimulateMouseUi/SimulateMouseUiUseCase.cs (1)
224-231: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
CreateFailurefor the defensive default case.This branch duplicates the exact shape already built by
CreateFailure(parameters, message)(Line 155). Reusing it keeps failure-response construction consistent.♻️ Proposed refactor
default: // Unreachable when TryFromSchema succeeds; kept as a defensive Success=false response // instead of a throw so any future MouseAction addition surfaces as a validation failure. - return new SimulateMouseUiResponse - { - Success = false, - Message = $"Unknown mouse action: {parameters.Action}", - Action = parameters.Action.ToString() - }; + return CreateFailure(parameters, $"Unknown mouse action: {parameters.Action}");🤖 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 `@Packages/src/Editor/FirstPartyTools/SimulateMouseUi/SimulateMouseUiUseCase.cs` around lines 224 - 231, The defensive default branch in SimulateMouseUiUseCase should reuse the existing CreateFailure(parameters, message) helper instead of manually constructing a SimulateMouseUiResponse with the same failure shape. Update the unreachable fallback in the SimulateMouseUiUseCase logic to call CreateFailure with the unknown-action message so response construction stays consistent with the other failure paths.
🤖 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
`@Packages/src/Editor/FirstPartyTools/SimulateMouseUi/SimulateMouseUiUseCase.cs`:
- Around line 224-231: The defensive default branch in SimulateMouseUiUseCase
should reuse the existing CreateFailure(parameters, message) helper instead of
manually constructing a SimulateMouseUiResponse with the same failure shape.
Update the unreachable fallback in the SimulateMouseUiUseCase logic to call
CreateFailure with the unknown-action message so response construction stays
consistent with the other failure paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 66bc19b9-9b1a-45e1-9ec5-ae22baa695ed
⛔ Files ignored due to path filters (2)
Assets/Tests/Editor/InputActionValidationTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/SimulateMouseUiActionValidationTests.cs.metais excluded by none and included by none
📒 Files selected for processing (12)
Assets/Tests/Editor/InputActionValidationTests.csAssets/Tests/Editor/PausePointTests.csAssets/Tests/Editor/RunTestsToolTests.csAssets/Tests/Editor/RunTestsUseCaseTests.csAssets/Tests/Editor/SimulateMouseUiActionValidationTests.csPackages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.csPackages/src/Editor/FirstPartyTools/RecordInput/RecordInputUseCase.csPackages/src/Editor/FirstPartyTools/ReplayInput/ReplayInputUseCase.csPackages/src/Editor/FirstPartyTools/RunTests/RunTestsUseCase.csPackages/src/Editor/FirstPartyTools/RunTests/TestFilterCreationService.csPackages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardUseCase.csPackages/src/Editor/FirstPartyTools/SimulateMouseUi/SimulateMouseUiUseCase.cs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Route the unknown-mouse-action branch through the existing CreateFailure helper, assert TryFromSchema's error contract instead of masking it with a fallback message, return only the error from pause-point id validation, and use a nullable enum for the mouse action conversion so the invalid state is unrepresentable.
This was referenced Jul 8, 2026
Merged
RyanXie123
pushed a commit
to RyanXie123/unity-cli-loop
that referenced
this pull request
Sep 22, 2026
…ad of an RPC failure (hatayama#1522)
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
User Impact
Changes
Verification