Conversation
WalkthroughAdds a new MCP server command exposing a “room_get” tool that lists Hue rooms, plus a simple “SayHi” tool. Wires the MCP server into the root CLI under an AI command group. Updates go.mod to include the MCP SDK and refreshes several indirect dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as Cobra CLI
participant MCP as MCP Server
participant Tool as Tool: room_get
participant OH as OpenHUE Context
rect rgba(230,245,255,0.6)
note over CLI: New AI group and mcp-server command
User->>CLI: openhue mcp-server
CLI->>MCP: Start over stdio (with OH in context)
end
rect rgba(235,255,235,0.6)
note over MCP: Tool invocation flow
User->>MCP: Call tool "room_get"
MCP->>Tool: Invoke ListRooms(ctx)
Tool->>OH: Get rooms
alt Rooms found
OH-->>Tool: [room list]
Tool-->>MCP: Formatted room names
MCP-->>User: Content: list of rooms
else No rooms
OH-->>Tool: []
Tool-->>MCP: "No rooms found"
MCP-->>User: Message: none
else Context error
Tool-->>MCP: error (no OpenHUE context)
MCP-->>User: Error response
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
go.mod (2)
41-48: Tidy and lock X packages to the module’s Go version.After upgrading x/* libs, run
go mod tidy -compat=1.23to ensure graph minimality and avoid accidental reliance on 1.24-only stdlib APIs whengo 1.23.0is declared.
12-12: Bump MCP SDK to v0.4.0- github.com/modelcontextprotocol/go-sdk v0.2.0 + github.com/modelcontextprotocol/go-sdk v0.4.0v0.4.0 (released 2025-09-05) includes the latest stdio transport and AddTool enhancements; upgrade to pull in these non-breaking improvements.
cmd/mcp/server.go (3)
3-11: Alias the SDK import to avoid confusion with local package name.The file’s package is
mcpand it imports.../mcp. Aliasing improves readability and avoids mental collisions.Apply:
-import ( +import ( "context" "fmt" "openhue-cli/openhue" "strings" - "github.com/modelcontextprotocol/go-sdk/mcp" + mcpsdk "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/spf13/cobra" )Then update type refs accordingly (see next diffs).
13-21: Register the “SayHi” tool or remove dead code.
SayHiis defined but never registered; either wire it or delete. Wiring is useful for smoke-testing MCP.Also convert SDK types to the aliased import:
type HiParams struct { Name string `json:"name" jsonschema:"the name of the person to greet"` } -func SayHi(ctx context.Context, cc *mcp.ServerSession, params *mcp.CallToolParamsFor[HiParams]) (*mcp.CallToolResultFor[any], error) { - return &mcp.CallToolResultFor[any]{ - Content: []mcp.Content{&mcp.TextContent{Text: "Hi " + params.Arguments.Name}}, +func SayHi(ctx context.Context, cc *mcpsdk.ServerSession, params *mcpsdk.CallToolParamsFor[HiParams]) (*mcpsdk.CallToolResultFor[any], error) { + return &mcpsdk.CallToolResultFor[any]{ + Content: []mcpsdk.Content{&mcpsdk.TextContent{Text: "Hi " + params.Arguments.Name}}, }, nil }And in
Run, add the tool (see later diff).
17-47: Use a typed, unexported context key instead of a string.Prevents collisions and makes retrieval compile-time safe.
Apply:
type HiParams struct { Name string `json:"name" jsonschema:"the name of the person to greet"` } +// context key for *openhue.Context (typed to avoid collisions) +type ctxKey int +const ctxKeyOpenHue ctxKey = iota + -func ListRooms(c context.Context, cc *mcp.ServerSession, params *mcp.CallToolParamsFor[any]) (*mcp.CallToolResultFor[any], error) { +func ListRooms(c context.Context, cc *mcpsdk.ServerSession, params *mcpsdk.CallToolParamsFor[any]) (*mcpsdk.CallToolResultFor[any], error) { - ctx, ok := c.Value("openhue").(*openhue.Context) + ctx, ok := c.Value(ctxKeyOpenHue).(*openhue.Context) @@ - return &mcp.CallToolResultFor[any]{ - Content: []mcp.Content{&mcp.TextContent{Text: "I couldn't find any rooms in your Hue home."}}, + return &mcpsdk.CallToolResultFor[any]{ + Content: []mcpsdk.Content{&mcpsdk.TextContent{Text: "I couldn't find any rooms in your Hue home."}}, }, nil } @@ - return &mcp.CallToolResultFor[any]{ - Content: []mcp.Content{&mcp.TextContent{ + return &mcpsdk.CallToolResultFor[any]{ + Content: []mcpsdk.Content{&mcpsdk.TextContent{ Text: fmt.Sprintf("Here are the rooms I found: %s.", strings.Join(roomNames, ", ")), }}, }, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
cmd/mcp/server.go(1 hunks)cmd/root.go(4 hunks)go.mod(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/mcp/server.go (3)
openhue/context.go (1)
Context(9-15)openhue/home_service.go (1)
SearchRooms(67-83)openhue/buildinfo.go (1)
BuildInfo(3-7)
cmd/root.go (2)
cmd/mcp/server.go (1)
NewCmdMcpServer(50-73)cmd/setup/setup.go (1)
NewCmdSetup(21-43)
🔇 Additional comments (2)
cmd/root.go (2)
24-26: Group ID addition looks good.AI group constant is clear and consistent with the new command’s GroupID.
54-57: LGTM: AI command group registration.This enables discoverability without impacting Hue/config groups.
| func ListRooms(c context.Context, cc *mcp.ServerSession, params *mcp.CallToolParamsFor[any]) (*mcp.CallToolResultFor[any], error) { | ||
|
|
||
| ctx, ok := c.Value("openhue").(*openhue.Context) | ||
| if !ok || ctx == nil { | ||
| return nil, fmt.Errorf("could not retrieve openhue context") | ||
| } | ||
|
|
||
| rooms := openhue.SearchRooms(ctx.Home, nil) | ||
| if len(rooms) == 0 { |
There was a problem hiding this comment.
Avoid nil deref when Home isn’t preloaded (ctx.Home can be nil).
SearchRooms(ctx.Home, ...) will panic if the HomeModel wasn’t loaded (the AI group doesn’t trigger LoadHomeIfNeeded). Lazily load it here.
Apply:
func ListRooms(c context.Context, cc *mcp.ServerSession, params *mcp.CallToolParamsFor[any]) (*mcp.CallToolResultFor[any], error) {
- ctx, ok := c.Value("openhue").(*openhue.Context)
+ ctx, ok := c.Value(ctxKeyOpenHue).(*openhue.Context)
if !ok || ctx == nil {
return nil, fmt.Errorf("could not retrieve openhue context")
}
- rooms := openhue.SearchRooms(ctx.Home, nil)
+ // Lazy-load Home if needed
+ if ctx.Home == nil {
+ home, err := openhue.LoadHome(ctx.H)
+ if err != nil {
+ return nil, fmt.Errorf("failed to load Hue home: %w", err)
+ }
+ ctx.Home = home
+ }
+ rooms := openhue.SearchRooms(ctx.Home, nil)Note: This diff references a typed context key added in a separate suggestion below.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cmd/mcp/server.go around lines 23 to 31, the call SearchRooms(ctx.Home, ...)
can panic if ctx.Home is nil because Home may not be preloaded; before calling
SearchRooms, check if ctx.Home == nil and lazily load it (invoke the context's
LoadHomeIfNeeded or equivalent method), handle and return any load error, and
only then call SearchRooms with the now-non-nil Home to avoid a nil dereference.
| // NewCmdMcpServer returns an initialized Command instance for 'mcp-server' sub command | ||
| func NewCmdMcpServer(ctx *openhue.Context) *cobra.Command { | ||
|
|
||
| cmd := &cobra.Command{ | ||
| Use: "mcp-server", | ||
| Aliases: []string{"mcp"}, | ||
| Short: "Starts the MCP server", | ||
| GroupID: "ai", | ||
| Run: func(cmd *cobra.Command, args []string) { | ||
|
|
||
| // Create a server instance | ||
| server := mcp.NewServer(&mcp.Implementation{Name: "openhue", Version: ctx.BuildInfo.Version}, nil) | ||
|
|
||
| // Add a tool to the server | ||
| mcp.AddTool(server, &mcp.Tool{Name: "room_get", Description: "List all rooms from home"}, ListRooms) | ||
|
|
||
| // Run the server over stdin/stdout, until the client disconnects | ||
| if err := server.Run(context.WithValue(context.Background(), "openhue", ctx), mcp.NewStdioTransport()); err != nil { | ||
| cobra.CheckErr(err) | ||
| } | ||
| }, | ||
| } | ||
|
|
||
| return cmd | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden the command for stdio: silence Cobra output, set stderr, add SayHi tool, and use typed context key.
Prevents protocol corruption and completes the tool set.
Apply:
func NewCmdMcpServer(ctx *openhue.Context) *cobra.Command {
cmd := &cobra.Command{
Use: "mcp-server",
Aliases: []string{"mcp"},
Short: "Starts the MCP server",
GroupID: "ai",
+ SilenceUsage: true,
+ SilenceErrors: true,
Run: func(cmd *cobra.Command, args []string) {
// Create a server instance
- server := mcp.NewServer(&mcp.Implementation{Name: "openhue", Version: ctx.BuildInfo.Version}, nil)
+ version := "dev"
+ if ctx != nil && ctx.BuildInfo != nil && ctx.BuildInfo.Version != "" {
+ version = ctx.BuildInfo.Version
+ }
+ server := mcpsdk.NewServer(&mcpsdk.Implementation{Name: "openhue", Version: version}, nil)
// Add a tool to the server
- mcp.AddTool(server, &mcp.Tool{Name: "room_get", Description: "List all rooms from home"}, ListRooms)
+ mcpsdk.AddTool(server, &mcpsdk.Tool{Name: "room_get", Description: "List all rooms from home"}, ListRooms)
+ mcpsdk.AddTool(server, &mcpsdk.Tool{Name: "say_hi", Description: "Greet a user"}, SayHi)
// Run the server over stdin/stdout, until the client disconnects
- if err := server.Run(context.WithValue(context.Background(), "openhue", ctx), mcp.NewStdioTransport()); err != nil {
+ if err := server.Run(context.WithValue(context.Background(), ctxKeyOpenHue, ctx), mcpsdk.NewStdioTransport()); err != nil {
cobra.CheckErr(err)
}
},
}
+ // Ensure Cobra writes help/usage to stderr (not stdout)
+ cmd.SetOut(cmd.ErrOrStderr())
return cmd
}Committable suggestion skipped: line range outside the PR's diff.
| root.AddCommand(mcp.NewCmdMcpServer(ctx)) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Protect MCP stdio from accidental stdout noise.
Because the MCP server speaks over stdout, any Cobra/print output there can corrupt the stream. Ensure all logs/usage/errors go to stderr and silence Cobra’s auto-usage.
Apply in the MCP command constructor (see server.go suggestions): set SilenceUsage and SilenceErrors to true and set command output to ErrOrStderr(). Also double‑check no code prints to stdout along this code path.
🤖 Prompt for AI Agents
In cmd/root.go around lines 96-97, the MCP subcommand is added but Cobra may
write usage/errors to stdout which will corrupt the MCP stdout protocol; in the
MCP command constructor (server.go) set cmd.SilenceUsage = true and
cmd.SilenceErrors = true, set the command’s output to
cmd.SetOut(cmd.ErrOrStderr()), and audit the MCP command code path to replace
any fmt.Print/Println or other stdout writes with stderr equivalents or
cmd.ErrOrStderr() so nothing writes to stdout.
1 participant
Summary by CodeRabbit
New Features
Chores