Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
Code Metrics Report
Details | | main (9bfb149) | #9 (b48d37d) | +/- |
|---------------------|----------------|--------------|-------|
+ | Coverage | 67.1% | 69.6% | +2.5% |
| Files | 3 | 3 | 0 |
| Lines | 73 | 99 | +26 |
+ | Covered | 49 | 69 | +20 |
- | Code to Test Ratio | 1:1.9 | 1:1.7 | -0.2 |
| Code | 170 | 206 | +36 |
+ | Test | 325 | 353 | +28 |
+ | Test Execution Time | 15s | 1s | -14s |Code coverage of files in pull request scope (67.1% → 69.6%)
Reported by octocov |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the existing Turn On/Off Device tool with a generic Execute Command tool, enriches device listings with JSON schemas for supported commands, and updates tests and dependencies to support the new functionality.
- Introduce
GetExecuteCommandTool, replacingGetTurnOnOffDeviceTooland routing all commands throughExecCommand - Augment
GetDeviceListToolto includecommandParameterJSONSchemafor devices implementingExecutableCommandDevice - Update tests (
device_control_test.go,devices_test.go) and bump dependencies ingo.mod
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/switch-bot-mcp-server/main.go | Register GetExecuteCommandTool instead of the TurnOn/Off tool |
| tools/device_control.go | Implement generic command execution via ExecCommand |
| tools/device_control_test.go | Rename and extend tests for the new Execute Command tool |
| tools/devices.go | Populate commandParameterJSONSchema in device listing payloads |
| tools/devices_test.go | Adjust expected maps to include JSON schemas |
| go.mod | Bump mcp-go, switch-bot-api-go and add new indirect modules |
Comments suppressed due to low confidence (1)
tools/device_control_test.go:14
- Consider adding a test case to verify that the handler returns a "Device not found" error (via
NewToolResultError) when no matching device exists.
func Test_GetExecuteCommandTool(t *testing.T) {
Comment on lines
+26
to
+71
| for _, device := range response.Body.DeviceList { | ||
| var deviceMap map[string]interface{} | ||
|
|
||
| deviceJsonText, err := json.Marshal(device) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| err = json.Unmarshal(deviceJsonText, &deviceMap) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| switch device.(type) { | ||
| case switchbot.ExecutableCommandDevice: | ||
| jsonSchema, err := device.(switchbot.ExecutableCommandDevice).GetCommandParameterJSONSchema() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| deviceMap["commandParameterJSONSchema"] = jsonSchema | ||
| } | ||
|
|
||
| devicesResponse.DeviceList = append(devicesResponse.DeviceList, deviceMap) | ||
| } | ||
|
|
||
| for _, device := range response.Body.InfraredRemoteList { | ||
| var deviceMap map[string]interface{} | ||
|
|
||
| deviceJsonText, err := json.Marshal(device) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| err = json.Unmarshal(deviceJsonText, &deviceMap) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| switch device.(type) { | ||
| case switchbot.ExecutableCommandDevice: | ||
| jsonSchema, err := device.(switchbot.ExecutableCommandDevice).GetCommandParameterJSONSchema() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| deviceMap["commandParameterJSONSchema"] = jsonSchema | ||
| } | ||
|
|
||
| devicesResponse.InfraredRemoteList = append(devicesResponse.InfraredRemoteList, deviceMap) |
There was a problem hiding this comment.
[nitpick] There is duplicated logic for mapping devices and adding schemas; extract into a helper function to reduce code repetition.
Suggested change
| for _, device := range response.Body.DeviceList { | |
| var deviceMap map[string]interface{} | |
| deviceJsonText, err := json.Marshal(device) | |
| if err != nil { | |
| return nil, err | |
| } | |
| err = json.Unmarshal(deviceJsonText, &deviceMap) | |
| if err != nil { | |
| return nil, err | |
| } | |
| switch device.(type) { | |
| case switchbot.ExecutableCommandDevice: | |
| jsonSchema, err := device.(switchbot.ExecutableCommandDevice).GetCommandParameterJSONSchema() | |
| if err != nil { | |
| return nil, err | |
| } | |
| deviceMap["commandParameterJSONSchema"] = jsonSchema | |
| } | |
| devicesResponse.DeviceList = append(devicesResponse.DeviceList, deviceMap) | |
| } | |
| for _, device := range response.Body.InfraredRemoteList { | |
| var deviceMap map[string]interface{} | |
| deviceJsonText, err := json.Marshal(device) | |
| if err != nil { | |
| return nil, err | |
| } | |
| err = json.Unmarshal(deviceJsonText, &deviceMap) | |
| if err != nil { | |
| return nil, err | |
| } | |
| switch device.(type) { | |
| case switchbot.ExecutableCommandDevice: | |
| jsonSchema, err := device.(switchbot.ExecutableCommandDevice).GetCommandParameterJSONSchema() | |
| if err != nil { | |
| return nil, err | |
| } | |
| deviceMap["commandParameterJSONSchema"] = jsonSchema | |
| } | |
| devicesResponse.InfraredRemoteList = append(devicesResponse.InfraredRemoteList, deviceMap) | |
| if err := processDevices(response.Body.DeviceList, &devicesResponse.DeviceList); err != nil { | |
| return nil, err | |
| } | |
| if err := processDevices(response.Body.InfraredRemoteList, &devicesResponse.InfraredRemoteList); err != nil { | |
| return nil, err |
| for _, device := range response.Body.DeviceList { | ||
| var deviceMap map[string]interface{} | ||
|
|
||
| deviceJsonText, err := json.Marshal(device) |
There was a problem hiding this comment.
[nitpick] Marshaling then unmarshaling each device can be expensive; consider constructing the map directly or using a lightweight DTO to avoid double JSON processing.
| "command_parameter_json", | ||
| mcp.Required(), | ||
| mcp.Description("Command to send (true:on, false:off)"), | ||
| mcp.Description("Command to send"), |
There was a problem hiding this comment.
[nitpick] Update this description to clarify that the field expects a JSON string of command parameters (e.g. '{"command":"TurnOn"}').
Suggested change
| mcp.Description("Command to send"), | |
| mcp.Description("JSON string of command parameters (e.g., '{\"command\":\"TurnOn\"}')"), |
Comment on lines
+38
to
+60
| expectedBody: `{"commandType": "command","command": "turnOn","parameter": "default"}`, | ||
| expectedDeviceID: physicalBotDeviceID, | ||
| args: map[string]interface{}{ | ||
| "device_id": physicalDeviceID, | ||
| "is_turn_on": true, | ||
| "device_id": physicalBotDeviceID, | ||
| "command_parameter_json": `{"command":"TurnOn"}`, | ||
| }, | ||
| }, | ||
| { | ||
| name: "Test handler physical device TurnOff", | ||
| expectedBody: "{\"commandType\": \"command\",\"command\": \"turnOff\",\"parameter\": \"default\"}", | ||
| expectedDeviceID: physicalDeviceID, | ||
| name: "Test handler physical BotDevice TurnOff", | ||
| expectedBody: `{"commandType": "command","command": "turnOff","parameter": "default"}`, | ||
| expectedDeviceID: physicalBotDeviceID, | ||
| args: map[string]interface{}{ | ||
| "device_id": physicalDeviceID, | ||
| "is_turn_on": false, | ||
| "device_id": physicalBotDeviceID, | ||
| "command_parameter_json": `{"command":"TurnOff"}`, | ||
| }, | ||
| }, | ||
| { | ||
| name: "Test handler physical BotDevice SetBrightness", | ||
| expectedBody: `{"commandType": "command","command": "setBrightness","parameter": "50"}`, | ||
| expectedDeviceID: physicalCeilingLightDeviceID, | ||
| args: map[string]interface{}{ | ||
| "device_id": physicalCeilingLightDeviceID, | ||
| "command_parameter_json": `{"command":"SetBrightness", "brightness": 50}`, |
There was a problem hiding this comment.
[nitpick] Comparing raw JSON strings can be brittle due to formatting/ordering; consider unmarshaling the response into a map and asserting individual fields instead.
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.
This pull request introduces significant changes to enhance the functionality of the SwitchBot MCP server, primarily focusing on replacing the "Turn On/Off Device" tool with a more flexible "Execute Command" tool. Additionally, it updates dependencies and improves the handling of device schemas to support a wider range of commands. Below is a summary of the most important changes:
Tool Replacement and Refactoring:
GetTurnOnOffDeviceToolwithGetExecuteCommandTool, enabling the execution of arbitrary commands on SwitchBot devices. This includes updating parameters to accept JSON-formatted command data and refactoring the logic to support the new functionality. (cmd/switch-bot-mcp-server/main.go[1]tools/device_control.go[2]tools/device_control.go[3]Dependency Updates:
go.modto include new versions of dependencies (mcp-go v0.27.1,switch-bot-api-go v0.4.0) and added several new indirect dependencies to support enhanced functionality, such asgo-json,go-yaml, andjsonschema-go. (go.modgo.modL6-R25)Test Enhancements:
GetExecuteCommandToolto validate its functionality, including tests for additional commands like "SetBrightness" and "SetAll" for different device types. (tools/device_control_test.go[1] [2] [3] [4]Device Schema Improvements:
GetDeviceListToolto include acommandParameterJSONSchemafield for devices that support executable commands, providing better introspection of device capabilities. (tools/devices.gotools/devices.goL24-R74)Test Updates for Device Schema:
GetDeviceListToolto verify the inclusion ofcommandParameterJSONSchemain the device response, ensuring compatibility with the new schema handling. (tools/devices_test.gotools/devices_test.goR62-R89)